Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Default in Color #31

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Philipp-M
Copy link

@Philipp-M Philipp-M commented Apr 27, 2024

So we had this argument in linebender/xilem#226 (comment)
And I think Default for Color doesn't really make sense, especially if Color::a is 0.

Is there a reason for implementing Default for it?

(CI breaks currently because of this, I'm waiting for initial feedback before fixing it (i.e. impl Default for the types depending on Color::default))

@ratmice
Copy link
Contributor

ratmice commented Apr 27, 2024

I tend to agree that at the very least 0 alpha is a weird default, curiously though it is somewhat in line with structures like kurbo::Rect, and kurbo::Circle which both return zero size shapes. I could I guess imagine it being useful with functional record update syntax with a better default.

I don't have any strong opinion on it though.

@xStrom
Copy link
Member

xStrom commented Apr 27, 2024

The derive seems to have been originally added in vello#168 by @dfrg, but there's no mentioned rationale.

The all-zero value, even though transparent, is still a valid value. It may be worth adding a custom default implementation with an opaque color, but the transparent color does work visually as "no color", revealing whatever is underneath as if there isn't any color.

As for why have Default implemented, it's so that you can easily derive Default for a higher level type without having to write a custom implementation with a specific color. Sometimes you just don't care what the color value is, you just want to write #[derive(Default)] and move on.

I think there is enough value and very little cost to keep Default implemented. Changing the default color could be worth it if we're worried about it being too hidden.

@dfrg
Copy link
Contributor

dfrg commented May 25, 2024

In general, I’m opposed to removing the Default impl but open to changing the value if people feel strongly about it.

@Philipp-M
Copy link
Author

Sure, maybe just (opaque) black or white? But @xStrom made good arguments for it being transparent. I'm indifferent with keeping, removing or changing the default value, this PR just resulted out of a conversation in xilem.

@ratmice
Copy link
Contributor

ratmice commented May 25, 2024

I'm don't have a strong opinion on transparent/opaque, I think if kurbo's shape impls had custom default implementations returning unit shapes, rather than zeros opaque black would be a better default.

because then something like

#[derive(Default)]
struct Foo {
   r: Rect,
   color: Color,
   tfm: Affine,
}

Foo::default();

Would give a black unit rect with an identify transform, which I think would be really convenient.
However that would require coordination and further updates to the Default impls on the kurbo side of things.
While there isn't a single unit value for every kind of shape in kurbo, it still seems to me like it would be nice.
Anyhow if there is any desire to move more towards that kind of direction for default implementations across the libs
I'd lean opaque.

@DJMcNab
Copy link
Member

DJMcNab commented May 26, 2024

Bevy's default here is WHITE:

https://github.com/bevyengine/bevy/blob/383314ef627da6654588a704959fc92c67770a52/crates/bevy_color/src/color.rs#L365

I don't have the reasoning to hand for that

@ratmice
Copy link
Contributor

ratmice commented May 26, 2024

My thinking was just make it contrast with whatever the default unfilled viewport color is.
So my thinking would be that if bevy uses black to fill an empty scene a default color of white would make sense.

I off hand forget what vello uses, I seem to recall transparent (but thought it was either transparent or white). But this also begs the question of whether different backends like bevy_vello and vello proper might use different unfilled viewport background. So there might very well not be any good choice for a contrasting default, since logical colors like foreground/background don't exist at this level of the stack.

@xStrom
Copy link
Member

xStrom commented May 26, 2024

Black/white background will vary between projects, so in that sense maybe some annoying neon color will work better for the intention of being immediately visible.

@DJMcNab
Copy link
Member

DJMcNab commented Jul 3, 2024

I think the conclusion we reached is that we probably will keep it, but change it to something non-transparent?
The candidates are:

  1. Opaque Black
  2. Opaque White
  3. Opaque of some intermediate colour, probably #ff00ff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants