arcade icon indicating copy to clipboard operation
arcade copied to clipboard

Updated the color setter so that it does not change the current alpha

Open FriendlyGecko opened this issue 1 year ago • 5 comments
trafficstars

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.

FriendlyGecko avatar Feb 11 '24 20:02 FriendlyGecko

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.

FriendlyGecko avatar Feb 11 '24 20:02 FriendlyGecko

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.

einarf avatar Feb 11 '24 23:02 einarf

That makes sense, it is a little late for me now, but I will try to draft something like this when I get up.

FriendlyGecko avatar Feb 12 '24 01:02 FriendlyGecko

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 avatar Feb 12 '24 09:02 FriendlyGecko

@FriendlyGecko Are you still interested in working on this?

TL;DR

  1. An .rgb property with getter+setter is definitely good
  2. We should defer an RGB class till a later PR.

How?

This is my opinion, but I'd take the following approach:

  1. Make a backup branch with these changes and push it to your fork

  2. Switch back to this PR's branch

  3. Revert the following:

    • Changes to BasicSprite.color
    • The RGB class (We'll wait till later, see bottom of comment)
  4. Add a Color.rgb property which returns self[:3]

  5. Modify BasicSprite.rgb's setter to use an inlined style like #2029

  6. Strongly consider reverting the _rgb slot

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.

  1. #1965
  2. A big doc build system refactor to help with the camera PR
  3. Various GL issues with blend state and texture managers
  4. Various doc issues
  5. Time / update issues
  6. Input problems we should fix now
  7. 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.

pushfoo avatar Mar 21 '24 07:03 pushfoo

Marking as closed due to features being implemented in #2060

FriendlyGecko avatar Apr 29 '24 21:04 FriendlyGecko