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

Align colors in SDL package to Go specification #581

Merged
merged 6 commits into from
Nov 25, 2023

Conversation

mokiat
Copy link
Contributor

@mokiat mokiat commented Nov 20, 2023

Closes #580

This PR does the following changes:

  • It ensures that Surface.Set does not panic by handling colors correctly.
  • It changes Surface.At to return color.NRGBA instead of color.RGBA which is the correct type for what is being returned (non-alpha-premultiplied color). This can be a breaking change if anyone ever expected a specific color from the method, despite the interface abstraction.
  • It changes the sdl.Color type to color.NRGBA which is the correct representation of what is held in sdl.Color (non-alpha-premultiplied color). This is a breaking change. Preserving the color.RGBA type mapping would require more code rework.
  • It fixes all of the color models to properly return 16bit-component alpha-premultiplied colors.
  • It fixes a broken type reference in the sdl package

Things to note:

  • The color.RGBA type in Go represents an alpha-premultiplied color and SDL2 appears to use standard non-alpha premultiplied colors everywhere. This is why color.NRGBA is the correct pick.
  • The Color.RGBA method is expected to return 16bit (i.e. from 0x0000 to 0xFFFF) alpha-premultiplied color components.
  • The PR makes heavy use of color.NRGBA and color.NRGBAModel, since these are the official ones and they have stable and optimized implementations for color conversions between alpha-premultiplied and non-alpha-premultiplied variants and from 8bit-component to 16bit-component colors.

Side notes:

  • In the past I had written a blog post about Go's colors which can shed some light on the above considerations. Feel free to remove this section if such type of links are unacceptable here.
  • While downscaling of bytes is trivial, upscaling is not so if you want to fill the whole scale. I have tried to come up with the proper bit-wise operation expressions but to double-check I have used the following Go program to empirically measure it: https://play.golang.com/p/fYrvt4TXFv2

@mokiat mokiat changed the title Draft: Align colors in SDL package to Go specification Align colors in SDL package to Go specification Nov 21, 2023
@mokiat
Copy link
Contributor Author

mokiat commented Nov 21, 2023

This PR should be ready for a more general public verification. The more people that have color scenarios in their apps to verify the better.

Copy link
Contributor

@Lundis Lundis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

I ran into the same issue today. This PR solves all of my issues and does not seem to break anything. NRGBA should be correct indeed 👍

@veeableful
Copy link
Contributor

Thank you very much @mokiat! I will merge this into the v0.4.x branch and create a new patch release as well so more people can test it.

@veeableful veeableful merged commit 0880ba8 into veandco:master Nov 25, 2023
1 check passed
veeableful pushed a commit that referenced this pull request Nov 25, 2023
* sdl/surface: handle colors correctly in Surface.Set method

* sdl/pixels: fix color models

* sdl/surface: handle RGBA correctly in Surface.At method

* sdl/pixels: make sdl.Color be of type color.NRGBA

* sdl/struct_test: fix compilation issue in sdl package

* sdl/pixels: improve binary upscale/downscale precision
@mokiat mokiat deleted the align_colors_to_spec branch November 26, 2023 18:01
@mokiat
Copy link
Contributor Author

mokiat commented Nov 26, 2023

Thanks! Using v0.4.36, colors work well for me now.

On a side note, asking for future reference: should I have opened the PR against v0.4.x or was master ok, since there seem to be other API changes in master (e.g. non-ptr events) that have not gone into v0.4.x?

@veeableful
Copy link
Contributor

Yes, master is preferred in most cases. I usually cherry-pick the commit into v.4.x branch afterwards. (Speaking of, I really should get v0.5.x out already 😓, just stuck on the static libraries currently).

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.

Surface panics on color conversions due to nonidiomatic color models
3 participants