meerk40t / svgelements

SVG Parsing for Elements, Paths, and other SVG Objects.
MIT License
132 stars 29 forks source link

RGBA vs ARGB for Color objects #109

Closed Lofgren closed 3 years ago

Lofgren commented 3 years ago

Looking at the code, it seems 8 digit hex values (e.g. #8899AABB) are interpreted as ARGB values, meaning that the alpha component is in the MSB position. This is also the string representaion used for a (non opaque) color. However, the CSS3 specification uses an RGBA representation with the alpha component in the LSB position (making the above example #BB8899AA).

Reading up on this, I understand that Android uses ARGB and this obviously leads to some confusion, but I still think that it would be more correct to use RGBA in accordance with the CSS3 spec. I can fix this and submit a PR if you are interested. A change like this may break some applications, though.

tatarize commented 3 years ago

I checked you're right. Yeah, I'd take that PR. The correct answer is always how SVG/CSS works in the spec, deviations from that are bugs. It would probably just get a minor upgrade 1.4.13 or so. I'm used to ARGB but yeah 8 byte hex is RRGGBBAA

tatarize commented 3 years ago

The 4 character hex code is equally RGBA values there rather than ARGB.

Lofgren commented 3 years ago

I started to write the update and noticed some inconsitencies in the internal handling of the color value. In some places (e.g. the hex parsing of 3 and 6 digit hex numbers) explicit care is taken to keep the result in a 32 bit integer (by ensuring a negative number when alpha is greater than 0x7F), but in other places this is ignored and the value will expand to a bigger Python int. This doesn't change the user behavior in any way, but I think this should either be enforced everywhere, or there is no need to do it anywhere. The readme file further states that the value is stored in a 32 bit int. Since this is not always the case, this is wrong.

My suggestion is to drop the attempt to keep the data in a 32 bit int and just update the manual. This directly affects the way I write this update, so I need to know how you want this handled.

tatarize commented 3 years ago

It was a python 2 code. Support was dropped at python 2, EOL. (I did briefly make it compatible again for a bCNC). It moved things around and manipulated the bits in specific ways to keep the datatype an int rather than letting it transition into being a long. It was kinda effective but generally failed as some OS implementations weren't fooled into keeping things int32, so it would act inconsistently at times. Since longs (as a namespace) don't exist in python 3, I could maintain python 3 & 2 compatibility if and only if, ints were restricted to int32s in both versions. Python 2 support is no longer a requirement or expectation.

Feel free to get rid of that code, there's no need confusing future would-be contributors. That section is walking on eggshells to preserve something that went out the window 16 months ago. Also, there's no reason to keep avoiding the python 3 typing syntax, so if that's something you wanted to add to the Color class, (or all the classes but that's a longer project), it's not an issue.

It still has some python 2 compatibility since the code goes back to when support for python 2 was a thing, anything that is python 2/3 can be safely made python 3 only without objection, that weird 32 bit int code included.

tatarize commented 3 years ago

110 corrects this issue. And 1.5.0 is out. The .value and int(color) of the colors are now also RGBA, 32 bit numbers.

tatarize commented 3 years ago

Hit a couple snags with Color() with my other codebase, so I had to add some stuff in 1.5.1 to soften those edges.

Mostly Color(int) now uses value and shifted the code a lot. So I'm putting that to being RGB and added a bunch of other helpers so you can do Color.rgb Color.argb and Color.rgba. And Color(argb=int), and some other previously ignored options. And mixing and matching some values so Color('red', alpha=128) now works, etc. This at least gives enough tools to correctly deal with the breaking changes.