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

Replace System.Drawing primitives with our own implementations. #15973

Merged
merged 8 commits into from Mar 4, 2019

Conversation

@pchote
Copy link
Member

commented Dec 31, 2018

This PR implements the bulk of the work towards #15955 by replacing System.Drawing.(Color|Rectangle|Size) with our own implementations. The new implementations drop all of the features we don't need and are / work towards being immutable.

Depends on #15930.
Depends on #16218.

I strongly recommend reviewing commit-by-commit to avoid all the using changes drowning context.

@pchote pchote added this to the Next + 1 milestone Dec 31, 2018

@pchote pchote force-pushed the pchote:drawing-struct-replacement branch 2 times, most recently from 9d93133 to d814225 Dec 31, 2018

return R.ToString("X2") + G.ToString("X2") + B.ToString("X2") + A.ToString("X2");
}

public static Color Transparent { get { return FromArgb(0x00FFFFFF); } }

This comment has been minimized.

Copy link
@pchote

pchote Dec 31, 2018

Author Member

These definitions were generated programmatically:

foreach (var colorValue in Enum.GetValues(typeof(System.Drawing.KnownColor)))
{
	var color = System.Drawing.Color.FromKnownColor((System.Drawing.KnownColor)colorValue);
	Console.WriteLine("public static Color {0} {{ get {{ return FromArgb(0x{1:X8}); }} }}", color.Name, color.ToArgb());
}

and then all the junk non-color colors (scrollbar, button, etc) were removed.
I'm not expecting anybody to try and verify these individually.

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

hm... shouldnt we make transparent 0x00000000 ? This would allow comparison with empty pixels on images. Like when creating a new pixel array, pixels should be == Color.Transparent.

return !(left == right);
}

public float GetBrightness()

This comment has been minimized.

Copy link
@pchote

pchote Dec 31, 2018

Author Member

GetBrightness/GetSaturation/GetHue implementations are based on "Go To Definition" on the mono types, then cleaning up the generated code. Compare with https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Primitives/src/System/Drawing/Color.cs#L489 for the .NET source.

return FromArgb((byte)(argb >> 24), (byte)(argb >> 16), (byte)(argb >> 8), (byte)argb);
}

public static bool TryParse(string value, out Color color)

This comment has been minimized.

Copy link
@pchote

pchote Dec 31, 2018

Author Member

This was copied directly from HSLColor.TryParseRGB.

return new Color(((byte)alpha << 24) + ((byte)red << 16) + ((byte)green << 8) + (byte)blue);
}

public static Color FromAhsl(int alpha, float h, float s, float l)

This comment has been minimized.

Copy link
@pchote

pchote Dec 31, 2018

Author Member

This was copied directly from HSLColor.RGBFromHSL(plus addition of alpha channel).

return FromAhsl(255, h, s, l);
}

public static Color FromAhsv(int alpha, float h, float s, float v)

This comment has been minimized.

Copy link
@pchote

pchote Dec 31, 2018

Author Member

This was copied directly from HSLColor.FromHSV(plus addition of alpha channel).

return FromAhsv(255, h, s, v);
}

public void ToAhsv(out int a, out float h, out float s, out float v)

This comment has been minimized.

Copy link
@pchote

pchote Dec 31, 2018

Author Member

This was copied directly from HSLColor.ToHSV (plus addition of alpha channel).

@pchote pchote force-pushed the pchote:drawing-struct-replacement branch from d814225 to 59fc9a7 Dec 31, 2018

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

Looks good to me.

@pchote pchote force-pushed the pchote:drawing-struct-replacement branch from 59fc9a7 to e143c5d Jan 8, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

Rebased.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

Needs another rebase.

@pchote pchote force-pushed the pchote:drawing-struct-replacement branch from e143c5d to 35e8703 Feb 3, 2019

@pchote pchote removed the PR: Rebase me! label Feb 3, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2019

Rebased.

@pchote pchote force-pushed the pchote:drawing-struct-replacement branch from 35e8703 to 64d577b Feb 18, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

Rebased on #16218 to remove the image saving workarounds.

@chrisforbes
Copy link
Member

left a comment

This looks mostly good; just some jarring naming issues, and there needs to be an explanation of what the embedded metadata is for.

OpenRA.Game/FileFormats/Png.cs Outdated Show resolved Hide resolved
var c1 = new HSLColor(c.H, c.S, Math.Max(rampRange, c.L)).RGB;
var c2 = new HSLColor(c.H, c.S, (byte)Math.Max(0, c.L - rampRange)).RGB;
var rampRange = (byte)((1 - rampFraction) * l);
var c1 = Color.FromAhsl(h, s, Math.Max(rampRange, l));

This comment has been minimized.

Copy link
@chrisforbes

chrisforbes Feb 23, 2019

Member

FromAhsl/FromAhsv should take an alpha value, or be named FromHsl/FromHsv

This comment has been minimized.

Copy link
@pchote

pchote Feb 24, 2019

Author Member

This is modeled after System.Drawing.Color.FromArgb, which provides non-alpha overloads. I can change this and our clone of FromArgb if you insist, but i'd prefer not to.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

Needs rebase now.

@pchote pchote force-pushed the pchote:drawing-struct-replacement branch from 64d577b to a295335 Feb 24, 2019

@pchote pchote removed the PR: Rebase me! label Feb 24, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

Rebased and updated.

@pchote pchote requested a review from chrisforbes Feb 24, 2019

{
readonly long argb;

public static Color FromArgb(int red, int green, int blue)

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

Isnt it a better idea to call this one FromRgb? There is no alpha handled here.

This comment has been minimized.

Copy link
@pchote

pchote Mar 1, 2019

Author Member

I commented about this above - these signatures are either copied directly from or modelled after System.Drawing.Color to keep compatibility.

If there is a consensus that we should aim for correctness instead then I can do that, but it will mean expanding the diff in this PR even further as all the places that use these will obviously need to change too.

return FromArgb(alpha, (int)(rgb[0] * 255), (int)(rgb[1] * 255), (int)(rgb[2] * 255));
}

public static Color FromAhsl(float h, float s, float l)

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

Isnt it a better idea to call this one FromHsl? There is no alpha handled here. Also to unify this with the above Rgb variant, shouldnt we move this method over FromAhsl?

return FromAhsl(alpha, h, ss, ll);
}

public static Color FromAhsv(float h, float s, float v)

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

Isnt it a better idea to call this one FromHsv? There is no alpha handled here.

return (int)argb;
}

public static Color FromArgb(int alpha, Color baseColor)

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

While in different libraries like sass, you can use Argb to add alpha to a color too, this might confuse on places where color is purely created from HSL or HSV. Wouldnt WithAlpha be a better name?

This comment has been minimized.

Copy link
@pchote

pchote Mar 1, 2019

Author Member

As above, this is inherited from the System.Drawing implementation. Renaming this means also renaming the 39 uses of it across the engine and mod code, which will quickly add up if we want to rename the other methods too.

return R.ToString("X2") + G.ToString("X2") + B.ToString("X2") + A.ToString("X2");
}

public static Color Transparent { get { return FromArgb(0x00FFFFFF); } }

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

hm... shouldnt we make transparent 0x00000000 ? This would allow comparison with empty pixels on images. Like when creating a new pixel array, pixels should be == Color.Transparent.

@@ -231,22 +230,22 @@ static bool IsPaletted(byte bitDepth, PngColorType colorType)

public byte[] Save()
{
var pixelFormat = Palette != null ? PixelFormat.Format8bppIndexed : PixelFormat.Format32bppArgb;
var pixelFormat = Palette != null ? System.Drawing.Imaging.PixelFormat.Format8bppIndexed : System.Drawing.Imaging.PixelFormat.Format32bppArgb;

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

I guess removal of System.Drawing here is for another PR...?

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

Applies to all changes in this file ofcourse :D

This comment has been minimized.

Copy link
@pchote

pchote Mar 1, 2019

Author Member

I still don't have the code or the understanding to write png directly, so this is deferred to the future.

@@ -102,7 +102,7 @@ endif
game_SRCS := $(shell find OpenRA.Game/ -iname '*.cs')
game_TARGET = OpenRA.Game.exe
game_KIND = winexe
game_LIBS = $(COMMON_LIBS) $(game_DEPS) thirdparty/download/SharpFont.dll thirdparty/download/Open.Nat.dll
game_LIBS = $(COMMON_LIBS) $(game_DEPS) System.Drawing.dll thirdparty/download/SharpFont.dll thirdparty/download/Open.Nat.dll

This comment has been minimized.

Copy link
@IceReaper

IceReaper Mar 1, 2019

Contributor

I guess having this here is part of the Png.cs stuff for a future PR...?

This comment has been minimized.

Copy link
@pchote

pchote Mar 1, 2019

Author Member

Correct

@reaperrr
Copy link
Contributor

left a comment

Code looks good as far as I'm able to tell, no in-game regressions spotted

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

I don't really care either way whether the FromArgb -> FromRgb etc. renaming happens, but in my opinion in order to get this merged soon and not blow up the diff further, that should be left for a follow-up.

@pchote pchote force-pushed the pchote:drawing-struct-replacement branch from a295335 to 3592f0e Mar 4, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Rebased again, and i'm going to merge this now - lets keep this one limited to a drop-in replacement, then we can consider improving the API in a future PR.

@pchote pchote merged commit dad21fe into OpenRA:bleed Mar 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote deleted the pchote:drawing-struct-replacement branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.