pythonarcade / arcade

Easy to use Python library for creating 2D arcade games.
http://arcade.academy
Other
1.71k stars 324 forks source link

Request for Feedback / Enhancement: Color API #1843

Closed pushfoo closed 7 months ago

pushfoo commented 1 year ago

Enhancement / Request for Feedback: Color API

tl;dr

  1. Our current color handling and Color type have multiple flaws as shown by #1838
  2. Much of item 1 is due to trying to nudge users away from using RGB colors
  3. Some of our color property and argument names are inconsistent with both our prior code and pyglet's
  4. This is a GitHub issue rather than a Discord thread because the users most concerned with Color (@bunny-therapist) do not appear to use Discord

My current (and loosely held) idea for fixing this while preserving our "keep the alpha unless replaced" behavior:

  1. Fulfill #1838 as follows (mostly per @bunny-therapist's original suggestions):
    • Use RGBOrA255 instead of RGB255 for color arguments throughout the codebase
    • Make the Color type 3-length except when an alpha value is provided
    • Add a Color.rgb property as beginner-friendly shorthand for color_instance[:3]
    • Do not provide alpha values for our current color constants, except for TRANSPARENT_BLACK
    • TBD: When someone uses alpha on a 3-length Color, which happens: raise IndexError() or return None?
  2. Add an OPAQUE_WHITE color constant and use it
  3. Add replace(self, r = None, g = None, b = None, alpha = None) -> Color and opaque() -> Color instance methods to Color
  4. Add opacity properties throughout the codebase to match pyglet's API
  5. Add an alpha property to Color to be consistent with our own properties

Please comment on known issues or mention ones not yet covered.

Once we reach rough consensus, we can turn the items above into a checkboxes and start making PRs.

Known Problems

As pointed out in #1838, there are three main sources of confusion:

  1. Although we still support RGB arguments, we use RGBA255 as a type hint
  2. Users may expect arcade's color constants to leave the alpha of objects alone
  3. Color doesn't provide a beginner-friendly way to use only the RGB components

These are not minor issues. Many users will think of color and opacity as separate because:

While reviewing arcade and pyglet's code, I also noticed pyglet uses opacity for its alpha-related property, while we use alpha and a.

What we have now

To my understanding, Arcade's color handling includes alpha because:

  1. In almost all places, pyglet accepts RGBA colors (pyglet.Sprite is the only exception, and it will be changed shortly)
  2. It does this because OpenGL represents colors as RGBA internally

Since this is supposed to be a beginner-friendly library, it was a mistake to try to force users to use RGBA. Additionally, pyglet does not seem to be planning on dropping RGB support any time soon, so we have no reason to drop it.

Type + Link to Source Intended Usage Current Usage Comments
ChannelType = TypeVar('ChannelType') Allow defining generic color type aliases Only used in types.py
RGB = Tuple[ChannelType, ChannelType, ChannelType] Generic RGB type allowing specifying channel type Only used directly in types.py
RGBA = Tuple[ChannelType, ChannelType, ChannelType, ChannelType] Generic RGBA type allowing specifying channel type Only used directly in types.py
RGBOrA = Union[RGB[ChannelType], RGBA[ChannelType]] A generic for either 3 or 4 tuples of a channel type Only used directly in types.py
RGBOrA255 = RGBOrA[int] Specify either 3 or 4 length 0-255 color Only in arcade.text We should probably have used this instead of trying to encourage RGBA.
RGBOrANormalized = RGBOrA[float] Specify either 3 or 4 length float colors Only in arcade.types
RGBA255OrNormalized = Union[RGBA255, RGBANormalized] An RGBA color of either int or float channel type. Only in Window.clear() and View.clear() Per @einarf's comments on Discord, clear shouldn't use this.
class Color[RGBA255]: Backward-compatible RGBA color definition type arcade.color and arcade.csscolor

There is also a paused PR for adding a float version of the Color class (#1772). It would also help with #1794, among other issues. I've paused for multiple reasons, some of which include uncertainty about the color API.

Top Color Questions

2. How Should Color Handle Length & Alpha?

Length

I see the following as worth considering for our Color type and constants:

  1. 4-length only, with a helper .rgb property to get the rgb channels, as suggested by #1838
  2. Either 3 or 4 length

In the latter case, we could still have constants like arcade.color.TRANSPARENT_BLACK. However, we'd have to make the following changes:

  1. Make most constants 3-length
  2. Add with_alpha(self, alpha: int) -> Color and Color.opaque(self) -> Color instance methods
  3. Update Color.from_hex_string()

We can also mix it with an .rgb property since users may find it helpful if they write / use color mixing methods.

At the moment, the 3 or 4 length option seems best to me.

Alpha

How should accessing alpha on a 3-length color be handled?

  1. Raise a clearly worded IndexError
  2. Return None

3. Should We Support Non-RGB(A)? If so, how?

@cspotcode has brought up non-RGBA support. It has also been brought up on Discord at least once.

I've argued in favor of RGB/RGBA only for the following reasons:

If we want to enable support for non-RGBA, we have the following options:

Helper methods seem like the best option since they can be added at any time without performance or complexity penalties.

4. Should We Support Subclassing? If so, how?

Doing so may preclude optimizations such as #1841. However, we may be able to side-step this issue entirely if we define color as a Protocol type as outlined below.

Wish List Ideas

These are potentially thorny issues and probably best avoided.

I've mentioned them in case someone else can think of ways around their problems.

1. Color as a Protocol Type?

Using Protocol types could open the door to additional benefits.

Pros:

Cons:

1. Assignment to Slices of Color Properties?

As of now, I haven't found a good way to support syntax for the following:

sprite_instance.color[:3] = (1, 2, 3)

By "good", I mean the following:

  1. Performant
  2. Easy to implement

If we build this on the Protocol concept above, we might be able get the following:

However, it would have the following downsides:

bunny-therapist commented 1 year ago

(I am in fact on Discord, I'm just named differently. I changed my display name now to match.)

I like making most of the constants 3-length (rgb) and I also like the suggested instance methods.

I wonder if it would be user-unfriendly to have Color be 3 or 4 length, since that means that it is not obvious whether setting the color property of a Sprite will affect the alpha or not - it would depend on the value of the Color I think it would be better than currently, where setting color to a constant always changes alpha.

I have not followed the whole "RGBA discussion" - I understand that, somewhere internally, colors make sense as RGBA, but it has never really made much sense to me to use RGBA at the top-level user-facing API of arcade, especially since properties like "color" and "alpha" imply that RGB and A are separate. For me as a user, it would make more sense if Color was length 3 RGB and alpha something completely separate. Internally, in the Sprite and below that, one could handle it as RGBA if one wanted to (but it sounds as if pyglet uses RGB...?).

Ultimately, from a game-making perspective, it feels like there are more times that I want to adjust RGB and A separately than I want to adjust them both at the same time (but maybe that's just me). Sprite having shorthand properties like "visible" which adjust alpha seems to suggest that this is known (?).

Basically, I do not have the advantages of RGBA clear to me, I just see the drawbacks in terms of user-friendliness. I have occasionally defined colors with alpha values and reused those (I think my menu windows are some transparent grey), but those are very niche cases, not the most common ones.

cspotcode commented 1 year ago

@pushfoo wanted to compare how other engines handle color. Here are links and summaries for a few popular engines. I didn't include Unreal because I've never used it and the docs confuse me.

Prior Art

GameMaker / GML

https://manual.yoyogames.com/GameMaker_Language/GML_Reference/Drawing/Colour_And_Alpha/Colour_And_Alpha.htm

Pyglet

Godot

https://docs.godotengine.org/en/stable/classes/class_color.html

Unity

https://docs.unity3d.com/ScriptReference/Color.html https://docs.unity3d.com/ScriptReference/Material.html

Pygame

https://www.pygame.org/docs/ref/color.html https://www.pygame.org/docs/ref/draw.html#pygame.draw.rect

Construct

https://www.construct.net/en/make-games/manuals/construct-3/scripting/scripting-reference/object-interfaces/iworldinstance

Misc

Not a game engine, but came across this color library: https://pkg.go.dev/image/color Python color libraries: https://github.com/waveform80/colorzero -- uses namedtuple, closest to what we want to do. But only rgb, no alpha https://grapefruit.readthedocs.io/en/latest/ https://python-colormath.readthedocs.io/en/latest/

cspotcode commented 1 year ago

I need to read your proposal again -- have skimmed -- but I like your proposed changes so far.

One thing I did not see mentioned:

Will users of arcade always be able to assume that colors are instances of Color and have all the helpful properties and methods? Even when a color is assigned a non-Color tuple, will we convert to Color internally?

Here is an example of where this is convenient for users:

# Assign color of my_sprite_b
my_sprite.color = (255, 255, 0, 128)

# Elsewhere in the codebase, copy the color, not the alpha, from one sprite to another
other_sprite.color = my_sprite.color.rgb
# This assumes that `my_sprite.color` is an instance of `Color` despite the assignment above

Is this guarantee worth making? Is performance acceptable?

cspotcode commented 1 year ago

In case it helps, I tried to list all the things I imagine a beginner wanting to do easily in their games. I left the syntax column blank to avoid committing to a particular implementation, but I can fill it in with python snippets if that helps.

Use-case Possible syntax
Get color and alpha of a thing
Get color of a thing without alpha
Get alpha of a thing without color
Set color and alpha of a thing
Set color of a thing, don't change alpha
Set alpha of a thing, don't change color
Tweak single color channel of a thing's color, e.g. bump up the red
Declare color as CSS hexcode
Declare color as numbers 0-255 copied from image editor
Transform colors, e.g. blend 2 colors, adjust saturation, fade in/out
Ditto, but w/normalized values
Get normalized (float) color from int, and vice versa
Use normalized (float) color to update thing w/out gotchas sprite.color = normalized_color
Use the same color for a pyglet thing as an arcade thing (e.g. a sprite and a pyglet shape of the same color)
cspotcode commented 1 year ago

@pushfoo my 2cents for your questions

> How Should Color Handle Length & Alpha?

I prefer option 1. because I agree with @bunny-therapist's concern that variable length might be user-unfriendly.

It seems easier for every Color to be guaranteed 4-length with 4 ints (never None) so that anything hinted Color can assume an alpha int. Maybe I'm too biased towards static typing.

I like with_alpha(self, alpha: int) -> Color and Color.opaque(self) -> Color.

Color.from_hex_string() can return a Color with alpha defaulting to 255, then you can chain .rgb as needed.

> Should We Support Non-RGB(A)? If so, how?

Agreed that helper methods are best, not additional classes. I like your example def from_hsv255(self, h: int, s: int: v: int) -> Self:)

> Should We Support Subclassing? If so, how?

Doesn't seem useful. IMO we subclass NamedTuple. If it allows subclassing, great, but it's not necessary.

> Assignment to Slices of Color Properties?

As nifty as this might be, it seems too complex. It sounds like a clever, fun idea, but too complex.

We already have a bunch of tuple attributes with multiple setters to modify them: position/center_x/center_y, velocity/change_x/change_y, scale/scale_xy

pushfoo commented 1 year ago

Update tl;dr I'm not yet sure on some the questions, but i'm still working on this by making pyglet's color handling consistent

pushfoo commented 1 year ago

We don't need to implement HSV conversion ourselves. There's a module in the standard library which takes care of this: https://docs.python.org/3.8/library/colorsys.html

FriendlyGecko commented 9 months ago

Didn't see this prior to making #1970

Is there any room for my pull or should I drop it?

pushfoo commented 8 months ago

Is there any room for my pull or should I drop it?

In my opinion, scale back that specific PR for now. I'll comment more on the PR itself. This issue thread is full of ambitious and complicated stuff which got us bogged down. I'm tempted to leave most of it for the future so we can get 3.0 out the door.

pushfoo commented 7 months ago

TL;DR: Closing this ticket for the moment, will revisit as needed.

Fixing Cameras and doc build takes priority for 3.0. We'll definitely do the following:

  1. Add a .rgb property to Color type instances which returns Tuple[int, int, int]
  2. Skip things like HSV / etc

If time allows:

  1. Nicer naming scheme for Color-related things (probably requires search and replace)
  2. (Doubtful) Maybe add a float type / move color stuff to their own arcade.types submodule or something