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

Colour API proposal #6301

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

FreezyLemon
Copy link
Contributor

The changes in this PR are the additions needed to implement the proposal as a usable API. More things might need to be added later. Breaking changes are described below but not included in the PR so that reading the code is easier.

Keep in mind that this is a proposal and while the commits in this PR could be merged, they exist more to demonstrate the API as code. I don't mind changing stuff.

API overview

  • ColourInfo: Semantically unchanged. Describes the colours of the four vertices of a quad.
  • SRGBColour: Main single-colour type that framework users interact with. Contains a gamma-corrected sRGB colour. Thin wrapper around Colour4. The type discourages invalid things like gamma-incorrect colour math1.
  • LinearColour (new): Secondary colour type for framework users. Contains a linear-light sRGB colour2. Thin wrapper around Colour4. Basically only exists to do colour math.
  • Colour4: Should rarely be used directly. Low level, raw colour values, no inherent colour space. This is where the real math (and optimization) should happen so that all abstractions can benefit.
  • Color4: Semantically the same as Colour4. Colour4 (no osuTK dependency, uses System.Numerics.Vector4 which should optimise better) should always be preferred over Color4 unless interacting with an osuTK API that requires it.

Implementation notes

  • Turning MultiplyAlpha into a pure method is a hidden trap for existing usage ([Pure] should help with this, but not sure it's enough). Renaming it is an alternative, but making it pure is a requirement for a readonly SRGBColour.
  • The colour constants Color4.Red etc. are copies of the CSS <named-color> values and are defined to be in the sRGB colour space, so they should live in SRGBColour. This is also necessary to remove the implicit conversions Color4/Colour4 <-> SRGBColour later on.
  • The change from Color4 to Colour4 inside SRGBColour is most likely going to have performance implications. I am not sure how to test this because colours are used everywhere, and I do not have enough insight into the renderer implementations to know how this impacts them.

Next steps

This list is mostly unordered.

  • Update Colour4 and ColourInfo docs
  • Make SRGBColour readonly and extend API to cover common usage
  • Implement LinearColour
  • Remove SRGBColour.Linear (breaking with low impact)
    • Returns a Colour4 which loses colour space information
    • ToLinear() should be used instead
  • Remove SRGBColour operator overloads (*, /, +) (breaking with low impact)
    • Use ToLinear() and LinearColour instead
    • Make the expensive sRGB<->linear conversions explicit
    • Chaining the SRGBColour operators is slow and imprecise and turns the operators into a footgun3
  • Replace Color4 and Colour4 in public API with SRGBColour or maybe Colour4 instead (breaking with very high impact)
    • can be done in steps
    • Notable: OsuColour, Color4Extensions
  • Remove implicit conversions between Color4/Colour4 and SRGBColour (see Implicit conversions in SRGBColour should not exist #5714) (breaking with high impact)
  • Remove implicit conversions between Color4/Colour4 and ColourInfo (breaking with very high impact)
    • This is going to be a huge change (drawable.Colour = Color4.Red must be replaced with drawable.Colour = SRGBColour.Red), but can often be done with search-and-replace.
    • Benefit: Removes osuTK from a lot of places.
  • Figure out what to do with colour-related Bindable types
  • Figure out what to do with colour pickers
  • Maybe move hexcode parsing and related code to SRGBColour (these are also semantically sRGB)
  • Maybe remove colour constants in Colour4

Footnotes

  1. Gamma-incorrect operations could be done with helper functions.

  2. As described here (search for "srgb-linear"). Can also be called "linear sRGB" or "gamma-expanded sRGB". This is usually called "linear colour"/"linear colour space" in and around o!f, which is why the struct is named LinearColour.

  3. For example, a * b * c will: Convert a and b to linear. Multiply those two. Convert the result back to sRGB. Convert the first result and c to linear. Multiply those two. Convert that result back to sRGB.

- Make it a `readonly record struct`
- Update docs to indicate that it has no inherent colour space
- Add Deconstruct(r, g, b, a) method
- Wrapper around Colour4
- readonly record struct
- add some convenience methods
This should be their final destination.
They can be obsoleted/removed from Colour4 later.
@bdach
Copy link
Collaborator

bdach commented May 26, 2024

The type discourages invalid things like gamma-incorrect colour math
(...)
Gamma-incorrect operations could be done with helper functions.

I immediately disagree with these premises. They sound like ivory tower thinking to me.

As discussed multiple times to death in discord, like it or not, in 99% of use cases we want gamma "incorrect" to match every other graphics program that is gamma "incorrect". So we're going to have to sprinkle these "helpers" everywhere just to get what we want to match everything else, in 99% of use cases.

The seminal article on this: https://erikmcclure.com/blog/everyone-does-srgb-wrong-because/

@smoogipoo
Copy link
Contributor

smoogipoo commented May 29, 2024

Without spending too much time on this for now, my preliminary thought is that SRGB doesn't make sense to think about and use directly. It's only really useful when interpolating/blending colours. For anything else, I'd prefer the user to not have to think about SRGB unless it needs to be resolved into a colour space for the purpose of doing some sort of maths.

It's unintuitive because we never think of colours in SRGB - when we translate an RGB value from figma or want "red", we're basically dealing with the "linear" colour space, though colour spaces are actually irrelevant at this instant in time.

Eventually, the question will arise (from a new o!f consumer): "which colour should I use, SRGBColour or LinearColour?" To which the correct response will/should be "use either as they're identical, unless you're doing interpolation". But really, the user shouldn't have to think about or ask this at all.

So... I would actually be in favour of using Color4 more, and SRGBColour less. The considerations, imo, only need to be around:

  • Interpolation.ValueAt(): Perhaps this needs Color4 and SRGBColour variants, with the user required to cast to perform gamma-correct interpolation.
  • Drawable.FadeColour(): This hides the underlying interpolation and needs some sort of special support - perhaps a parameter bool gammaCorrect is fine but it will also need its own transform.

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

Successfully merging this pull request may close these issues.

None yet

3 participants