Fix Color::HSV() incorrect hue output

[AzulRadio]

🦟 Bug fix

Summary

• The implementation of Hue of Color::HSV() does not seem right, where HSV stands for hue, saturation, value.
• OS Version: Ubuntu 20.04
• Source build from b399035, branch ign-math6
• Also update the expected behavior in the color tutorial accordingly.
• Change test cases for Color

Why also change the test cases:

• Here is a document of Ignition Math that said the range of Hue should be [0, 360].
• The current Hue implementation does not seem to fit in any HSV standard as far as I know.
• Repeatedly converting back and forth between RGB and HSV will change the value of the color drastically.

Description

• Color::HSV() in Color.cc has an incorrect output hue (index 0 of return Vector3f object)

Example 1: Converting the RGB (0.5, 0.6, 0.7) to HSV.

• Expected output: (210, 0.286, 0.7), can verify with RGB to HSV
• Actual output: (3.5, 0.286, 0.7)

Example 2: Converting the RGB (0.0, 0.0, 0.0) to HSV.

• Expected output: (0, 0, 0)
• Actual output: (-1, 0, 0)

Examples can be verified by putting the following file in /examples

#include <iostream>
#include <ignition/math/Color.hh>

int main() 
{
  ignition::math::Color b = ignition::math::Color(0.0, 0.0, 0.0);
  std::cout << "The RGBA value of b: " << b << std::endl;
  std::cout << "The HSV value of b: " << b.HSV() << std::endl;

  ignition::math::Color a = ignition::math::Color(0.5, 0.6, 0.7);
  std::cout << "The RGBA value of a: " << a << std::endl;
  std::cout << "The HSV value of a: " << a.HSV() << std::endl;
}

Incorrect Output on terminal

Correct Output on terminal

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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.

[codecov]

Codecov Report

Merging #320 (6a0a57c) into ign-math6 (b399035) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           ign-math6     #320   +/-   ##
==========================================
  Coverage      99.65%   99.65%           
==========================================
  Files             67       67           
  Lines           6359     6360    +1     
==========================================
+ Hits            6337     6338    +1     
  Misses            22       22           

Impacted Files	Coverage Δ
src/Color.cc	98.58% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b399035...6a0a57c. Read the comment docs.

[chapulina]

Thanks for the fix!

In general, we avoid changes in behavior in a stable version even if they're bug fixes, because downstream users may be compensating for it and may be broken by the change. But in this case, I think it's safe to assume that users aren't using this function since it returns wrong values, so fixing it should be good. I think we should add a note to the migration guide just in case.

[AzulRadio]

@chapulina Thanks for the feedback! I updated the migration.md but I am not entirely sure how to write this guide (which version are we migrating from & to, etc). Could you help me verify what I added in the guide is correct? Thanks for your time!

[chapulina]

I updated the migration.md but I am not entirely sure how to write this guide (which version are we migrating from & to, etc). Could you help me verify what I added in the guide is correct?

Thanks, the text looks good to me. But since the fix will go into the next release, mind putting it under a new section ## Ignition Math 6.9.2 to 6.10.0 on the top?

Thanks for your time!

Thank you too!

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1