Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Color type is now a class instead of struct #56

Open
mattleibow opened this issue Apr 9, 2021 · 6 comments
Open

Color type is now a class instead of struct #56

mattleibow opened this issue Apr 9, 2021 · 6 comments
Labels
enhancement New feature or request ready Items that are planned and ready for work

Comments

@mattleibow
Copy link
Member

mattleibow commented Apr 9, 2021

I have been back and forth on this one, but it is still a large breaking change that will affect users. Maybe. I am not sure exactly how this affects all the code when recompiled, but it is different.

This will matter though when there are new null ref exceptions popping up, but hopefully tooling can aid with that.

Most frameworks are structs - except for iOS - and this may be unexpected.

@jonlipsky did mention that he noticed some perf improvements with TouchDraw, but this might need a check on say Android with the GC. It may be that it is fine and the GC is OK and that copying structs is more expensive.

@Clancey mentions a nicer reading:

var foo = color?.ToNative() ?? defaultColor
// vs
var foo = color == Color.Default ? defaultColor : color.ToNative()

This is true, but it is also the same with Color? as a nullable.

This also relates to #57 where there is no longer a Default "color" and null is used.

Relates to parent issue: #58

@mattleibow mattleibow changed the title Type is now a class instead of struct Color type is now a class instead of struct Apr 9, 2021
@charlesroddie
Copy link
Contributor

+1 should be a struct

@Clancey
Copy link
Contributor

Clancey commented Apr 10, 2021

Even with nullable structs, the code won’t be clean. You still need to check if it is default value. So the nullable struct is even uglier.

@hartez
Copy link
Contributor

hartez commented Apr 10, 2021

Even with nullable structs, the code won’t be clean. You still need to check if it is default value. So the nullable struct is even uglier.

That's only if we decide to use a struct AND keep a Color.Default. If we remove Color.Default, make Color a struct, and go with the convention that cross-platform colors default to null, then we end up with your cleaner-looking code. E.g., if my cross-platform property is

public Color? MyColor { get; set; }

Then my translation to a native color looks something like

var color = Element.MyColor?.ToNative() ?? GetNativeDefaultColor();

Where GetNativeDefaultColor is doing the work of determining what "default" means given the control type, usage, current theme, etc.

@vpenades
Copy link
Contributor

vpenades commented Oct 2, 2021

I also agree Color should be a struct... in fact, it should be a readonly struct, which will provide additional performance benefits.

Also, by being a struct, Color could be casted to Vector4 in memory, opening the door for fast, bulk color operations:

var pixelRow = new Color[256];
var vectorRow = MemoryMashall.Cast<Color,Vector4>(pixelRow);

@jsuarezruiz jsuarezruiz added the enhancement New feature or request label Nov 12, 2021
@ToolmakerSteve
Copy link

ToolmakerSteve commented Dec 11, 2021

@hartez

Color? ... and go with the convention that cross-platform colors default to null

Wouldn't that require most internal methods to take Color? instead of Color? Otherwise, there would no longer be a way to pass around a value that might be Default.

What advantage does Color? have over it being a class? Would have to be quite an advantage, to make people put ? at the end all the time. OR wonder when they should say Color and when they should say Color?.

IMHO, it either stays a struct with Color.Default, or it becomes a class.

@Redth Redth added the ready Items that are planned and ready for work label Jan 19, 2022
@malware-dev
Copy link

malware-dev commented Jun 6, 2023

This will cause a lot of micro allocations... as well as unexpected behaviors. Not to mention that it just instinctively feels unnatural 😛 Without knowing the reasoning behind why this was done, this seems like a poor decision to me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request ready Items that are planned and ready for work
Projects
None yet
Development

No branches or pull requests

9 participants