-
-
Notifications
You must be signed in to change notification settings - Fork 852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Convert.To after rounding in Pack() to avoid different behavior on ARM vs x86/x64 #1790
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1790 +/- ##
==========================================
- Coverage 87.10% 87.03% -0.08%
==========================================
Files 936 936
Lines 47649 47631 -18
Branches 6015 6015
==========================================
- Hits 41506 41456 -50
- Misses 5150 5181 +31
- Partials 993 994 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -523,7 +520,7 @@ private static string ToRgbaHex(string hex) | |||
return hex + "FF"; | |||
} | |||
|
|||
if (hex.Length < 3 || hex.Length > 4) | |||
if (hex.Length is < 3 or > 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the expense of less readable code one can save a comparison here:
if (hex.Length is < 3 or > 4) | |
if ((uint)(hex.Length - 3) > (4 - 3)) |
BTW: is this method hot enough worth optimizing? One could save same allocations here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gfoidl thanks for your feedback, but this is not a critical path. I rather keep this readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat trick... Not hot. It's just used for parsing hex values which are generally defined outside loops.
I wonder if we should replicate the behavior of the https://source.dot.net/#System.Private.CoreLib/Convert.cs,10cd63a0dc4a7c4c |
I dont know if its really worth for such niche pixel formats, like short2 and short4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One question.
public readonly Vector4 ToVector4() => new Vector4(0, 0, 0, this.PackedValue / 255F); | ||
public readonly Vector4 ToVector4() => new(0, 0, 0, this.PackedValue / 255F); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So type interference is something we prefer in the codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the shorter new(...)
syntax very much. From your question im sensing you are not totally convinced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference, given up on code style battles long time ago 😄
Just want see us making a conscious decision on this, and have a preferred style established. @JimBobSquarePants your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all in. I absolutely LOVE being able to declare on the left. 😍 Foo foo = new(bar);
😍
If we really want to spend time optimizing these methods, I'd rather explore how to SIMD them on x86. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! I think we can leave the Convert as-is and focus on potential bulk optimization for V3.
Prerequisites
Description
I noticed some pixelformat tests failing on ARM64. Namly short2, short4, normalizedbyte4 tests.
The reason for that is a cast from float to int in
Pack
. on ARM this gets rounded to 0, if its negative, while on x86/x64 its not.For example:
Result on ARM64: 0
Result on x64: 4294967196 (uint.Max + f + 1)
To avoid the difference, i have added a
Convert.ToInt32
after the rounding.The rest of the changes are some minor cleanup.