Skip to content

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

Merged
azeey merged 3 commits intogazebosim:mainfrom
azeey:remove_color_clamp
Aug 16, 2024
Merged

Clamping color values to within [0, 1]#613
azeey merged 3 commits intogazebosim:mainfrom
azeey:remove_color_clamp

Conversation

@azeey
Copy link
Copy Markdown
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 <addisu@openrobotics.org>
@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
iche033 and others added 2 commits August 13, 2024 17:26
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey changed the title Remove clamping of color values Clamping color values to within [0, 1] Aug 15, 2024
@azeey
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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