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

Clamping color values to within [0, 1] #613

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Jul 26, 2024

🦟 Bug fix

Toward #200

Summary

This removes the clamping of color values, but it doesn't yet enforce the invariant for channels being within [0, 1]. The best way to do that would be to throw an exception, but we don't do that anywhere in the gz-math codebase.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Addisu Z. Taddese <[email protected]>
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Jul 26, 2024
@azeey azeey added the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Jul 26, 2024
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@azeey azeey changed the title Remove clamping of color values Clamping color values to within [0, 1] Aug 15, 2024
@azeey
Copy link
Contributor Author

azeey commented Aug 15, 2024

I've added enforcement of the color values range [0, 1] in 5f0ae54. Some of the test values needed to be updated because the clamping is different now: instead of dividing by 255 when a value is greater than 1, it's clamped to 1.

While I was trying to fix tests, I noticed that the functions that convert between RGB and YUV are not well defined. It's not clear what the input range is for YUV values. I also couldn't find a good reference for the equations used for the conversion. In fact, checking wikipedia, it looks like the math might be incorrect. As far as I can tell, these functions are not being used anywhere in Gazebo, so I propse we deprecate them.

@iche033
Copy link
Contributor

iche033 commented Aug 16, 2024

the YUV conversion code matches one of the formula in this post: https://www.vbforums.com/showthread.php?728455-What-is-the-CORRECT-conversion-for-YUV-to-RGB.

the HSV conversion code seems to come from: https://stackoverflow.com/a/6930407

I tried a few online color converters using the values from our test and the results don't really match. I agree we should either fix the code or deprecate / remove it.

@azeey
Copy link
Contributor Author

azeey commented Aug 16, 2024

Yes, there seems to be many conventions around this and I don't think any of us are experts in this area. I'll deprecate it in a follow-up PR.

@azeey azeey merged commit 98608b6 into gazebosim:main Aug 16, 2024
9 checks passed
@iche033 iche033 mentioned this pull request Aug 21, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection Breaking change Breaks API, ABI or behavior. Must target unstable version. 🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants