arcade
arcade copied to clipboard
Updated the color setter so that it does not change the current alpha
Will allow the closure of #1838.
I just swapped the last value self.color[3] with self._alpha which will the alpha getter to maintain the current.
Recognized that that wasn't enough to solve the open issue so I modified the code to check for use of the arcade.Color type. Now it should be ready for testing/merge.
TO CLARIFY: I only modified the len = 3 check. Not len = 4, so users can still set the alpha through this if they wish.
We probably should have separate .rgb and .alpha properties and let the color one just take whatever length is passed in. This is more in line with arcade 2.6 and solves two issues.
That makes sense, it is a little late for me now, but I will try to draft something like this when I get up.
I created a workable draft, larger part will be cleaning up the RGB class which for the moment is just a copy of the Color class. As of right now, it is functional just not clean.
@FriendlyGecko Are you still interested in working on this?
TL;DR
- An
.rgbproperty with getter+setter is definitely good - We should defer an
RGBclass till a later PR.
How?
This is my opinion, but I'd take the following approach:
-
Make a backup branch with these changes and push it to your fork
-
Switch back to this PR's branch
-
Revert the following:
- Changes to
BasicSprite.color - The
RGBclass (We'll wait till later, see bottom of comment)
- Changes to
-
Add a
Color.rgbproperty which returnsself[:3] -
Modify
BasicSprite.rgb's setter to use an inlined style like #2029 -
Strongly consider reverting the
_rgbslot
Why revert BasicSprite.color changes?
TL;DR: Computes rarely used data + has a merge conflict
We noticed visibility behavior is broken at the moment. There's already a PR to fix it over at #2029. The implementation there seems like good reference for a BasicSprite.rgb property since it seems to make efficiency improvements to the color setter. However, they also conflict with the current implementation of this branch.
Calculating an rgb value each time we set also doesn't seem worthwhile if:
- Color property writes are common
- Color property reads are rare
- The GPU is sent RGBA each time anyway
Why revert arcade.types.RGB for now?
TL;DR: Not a hard no forever, but we need to ship 3.0 + there are ongoing perf questions about tuple subclassing
New Features Discouraged until 3.1
To be clear, this isn't a hard no forever. However, the current priority is shipping Arcade 3.0. New features are somewhat discouraged for the moment unless:
- It's clear they won't break things somehow
- They need to be done now to prevent huge future pain
In this case, expanding the already-overfilled arcade.types module:
- risks breaking things
- doesn't seem necessary to prevent future issues
Issues with these are why #1772 is on hold.
Why revert the _rgb slot addition?
TL;DR: Duplicates rarely read data
If we read .rgb from sprites rarely, it's not worth:
- Duplicating data already found in
BasicSprite.color - Recomputing duplicated value with an expensive function call
- The complexity of a lazily updated property as an alternative
Perf Questions about Tuples / Etc
Another reason to wait is the perf profiling the pyglet project has been doing. They've been benchmarking variations on implementing vector types somewhat similar to Color.
Although their findings need further investigation, they suggest Arcade's colors might need a closer look. This applies to both the current Color and any future color types.
When do we add RGB as a type?
TL;DR: After we finish things critical to 3.0
Right now, the RGB class conflicts with the RGB type alias we have. There might be a way to change these aliases and their names to be nicer, but we first need to finish addressing a lot of other things.
- #1965
- A big doc build system refactor to help with the camera PR
- Various GL issues with blend state and texture managers
- Various doc issues
- Time / update issues
- Input problems we should fix now
- pyglet math type updates
I'm putting some thought into our nomenclature for colors again. There might be a way to use your concept. However, it might require cleaning up types. You can see that started in #2030, but nothing uses that alias yet, so it doesn't break anything. Avoiding major unintended breakage + the big todo list above both take priority.
Marking as closed due to features being implemented in #2060