Skip to content
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

Patch for #251 #258

Closed
wants to merge 2 commits into from
Closed

Patch for #251 #258

wants to merge 2 commits into from

Conversation

@SleepyMode
Copy link
Collaborator

SleepyMode commented Nov 9, 2018

This pull request should fix #251, mostly.

One thing that was unable to be done was actually using the in modifier due to the fact that it wasn't added until C# 7.2 (while we're using C# 7.0).

Additionally, the negation operator (public static Color operator -) was left untouched due to the odd behaviour that was observed upon it's modification.

SleepyMode added 2 commits Nov 9, 2018
@SleepyMode SleepyMode changed the title Patch #251 Patch for #251 Nov 9, 2018
@@ -525,7 +525,7 @@ public static void Modulate(ref Color left, ref Color right, out Color result)
/// <returns>The modulated color.</returns>
public static Color Modulate(Color left, Color right)
{
return new Color(left.R * right.R, left.G * right.G, left.B * right.B, left.A * right.A);
return new Color((byte)(left.R * right.R), (byte)(left.G * right.G), (byte)(left.B * right.B), (byte)(left.A * right.A));

This comment has been minimized.

Copy link
@xen2

xen2 Nov 12, 2018

Member

Shouldn't there be a division by 255 here? (i.e. max intensity red would be R = 255 x R = 255 and should result in R = 255)

@@ -499,7 +499,7 @@ public static void Modulate(ref ColorBGRA left, ref ColorBGRA right, out ColorBG
/// <returns>The modulated color.</returns>
public static ColorBGRA Modulate(ColorBGRA left, ColorBGRA right)
{
return new ColorBGRA((left.R * right.R) >> 8, (left.G * right.G) >> 8, (left.B * right.B) >> 8, (left.A * right.A) >> 8);
return new ColorBGRA((byte)((left.R * right.R) >> 8), (byte)((left.G * right.G) >> 8), (byte)((left.B * right.B) >> 8), (byte)((left.A * right.A) >> 8));

This comment has been minimized.

Copy link
@xen2

xen2 Nov 12, 2018

Member

Same

This comment has been minimized.

Copy link
@Kryptos-FR

Kryptos-FR Nov 12, 2018

Collaborator

That's what >> 8 is supposed to do here. But it's wrong because it divides by 256 instead of 255.

This comment has been minimized.

Copy link
@xen2

xen2 Nov 12, 2018

Member

Right, sorry I thought this was same mistake as in Color.cs.
Anywway, should be / 255 as you said.

This comment has been minimized.

Copy link
@SleepyMode

SleepyMode Nov 13, 2018

Author Collaborator

Hm, well, I could add it once I add the in keywords soon.
Let's wait for the C# 7.2 PR first. (Which I'm going to upload any moment now)

@xen2 xen2 force-pushed the xenko3d:master branch from 88dcb74 to 1535057 Nov 19, 2018
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 18, 2019

@SleepyMode I will just go ahead and do the last fixes myself and merge it. Thanks!

@xen2 xen2 self-assigned this Jan 18, 2019
xen2 added a commit that referenced this pull request Jan 19, 2019
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 19, 2019

Merged in a separate commit, thanks!

@xen2 xen2 closed this Jan 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.