Ensure PixelOperations.To(TPixel) uses scaling. #2050
Conversation
| /// Gets the MDFileUnits exif tag. | ||
| /// </summary> | ||
| public static ExifTag<string> MDFileUnits => new ExifTag<string>(ExifTagValue.MDFileUnits); | ||
| public static ExifTag<string> MDFileUnits { get; } = new ExifTag<string>(ExifTagValue.MDFileUnits); |
There was a problem hiding this comment.
Pushed this by accident but it was something I found the other day when looking at the EXIF tags.
| <RuleSet Name="ImageSharp.Tests" ToolsVersion="17.0"> | ||
| <Include Path="..\shared-infrastructure\sixlabors.tests.ruleset" Action="Default" /> | ||
| <Rules AnalyzerId="StyleCop.Analyzers" RuleNamespace="StyleCop.Analyzers"> | ||
| <Rule Id="SA1313" Action="None" /> |
There was a problem hiding this comment.
Allows us to replace "dummy" with discard "_"
| PixelConverterTests.ReferenceImplementations.To<TPixel, TDestPixel>(this.Configuration, source, expected); | ||
|
|
||
| TestOperation(source, expected, (s, d) => this.Operations.To(this.Configuration, s, d.GetSpan())); | ||
| TestOperation(source, expected, (s, d) => this.Operations.To(this.Configuration, s, d.GetSpan()), false); |
There was a problem hiding this comment.
We need approximate comparison here now we've added all the types. Comparison is still very accurate.
| public void Issue2048() | ||
| { | ||
| // https://github.com/SixLabors/ImageSharp/issues/2048 | ||
| RgbaVector green = Color.Green.ToPixel<RgbaVector>(); |
There was a problem hiding this comment.
This is actually covered in the PixelOperation tests but I like to be explicit for serious bugs.
|
|
||
| public TestPixel(float red, float green, float blue, float alpha) | ||
| { | ||
| Guard.MustBeBetweenOrEqualTo(red, 0F, 1F, nameof(red)); |
There was a problem hiding this comment.
This was unbounded when it shouldn't be.
| { | ||
| var pix = default(TPixel); | ||
| pix.FromVector4(new System.Numerics.Vector4(this.Red, this.Green, this.Blue, this.Alpha)); | ||
| pix.FromScaledVector4(new Vector4(this.Red, this.Green, this.Blue, this.Alpha)); |
There was a problem hiding this comment.
Best to be explicit despite guard.
| { | ||
| private static readonly Lazy<PixelTypeInfo> LazyInfo = | ||
| new Lazy<PixelTypeInfo>(() => PixelTypeInfo.Create<RgbaVector>(PixelAlphaRepresentation.Unassociated), true); | ||
| new(() => PixelTypeInfo.Create<RgbaVector>(PixelAlphaRepresentation.Unassociated), true); |
There was a problem hiding this comment.
I have to say I am not a fan of this syntax for object creation... I find it a lot harder to read. especially in cases like this:
// in some piece of unchanged code
var list = new List<ObjType>();
.
.
.
// lots of lines later
.
.
.
// in the middle of a PR change block, there is no context available while review what the hell this thing is.
list.Add(new(arg1, "arg2", 34)
{
OptionalProperty = bar
});not going to block any PRs etc over this just wanted to comment on the fact I feel it drastically harms readability in most cases... the only exception I can kind of agree on is property initialization where var is not allowed. Or possibly collection initializers where the Type of clear in the collection type.
There was a problem hiding this comment.
Yeah, definitely something to be aware of. I really like it in some scenarios though
Prerequisites
Description
Fixes #2048