👩‍🌾 Prevent -0 with out stream operator

[chapulina]

🦟 Bug fix

Fixes #161

Summary

Implemented @azeey 's suggestion to check for zeroes and explicitly output 0 to avoid -0.

I also had to add a const version of Color::operator[] so it could be used by the operator.

Checklist

• Signed all commits for DCO
• Added tests (all existing tests should still pass)
• 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

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

https://github.com/osrf/buildfarmer/issues/181

[codecov]

Codecov Report

Merging #206 (b87c45b) into main (efb5db3) will increase coverage by 0.67%.
The diff coverage is 100.00%.

❗ Current head b87c45b differs from pull request most recent head b007963. Consider uploading reports for the commit b007963 to get more accurate results

@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
+ Coverage   98.53%   99.21%   +0.67%     
==========================================
  Files          61       65       +4     
  Lines        6001     6094      +93     
==========================================
+ Hits         5913     6046     +133     
+ Misses         88       48      -40     

Impacted Files	Coverage Δ
include/ignition/math/Angle.hh	100.00% <ø> (ø)
include/ignition/math/Box.hh	100.00% <ø> (ø)
include/ignition/math/MassMatrix3.hh	99.40% <ø> (ø)
src/Angle.cc	100.00% <ø> (+10.34%)	⬆️
include/ignition/math/Color.hh	100.00% <100.00%> (ø)
include/ignition/math/Ellipsoid.hh	100.00% <100.00%> (ø)
include/ignition/math/Helpers.hh	98.55% <100.00%> (+0.05%)	⬆️
include/ignition/math/Matrix3.hh	100.00% <100.00%> (ø)
include/ignition/math/Matrix4.hh	100.00% <100.00%> (ø)
include/ignition/math/Quaternion.hh	100.00% <100.00%> (+4.10%)	⬆️
... and 25 more

---

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 efb5db3...b007963. Read the comment docs.

[scpeters]

I added a test case in e3c7b81 that fails on ign-math6: printing a Pose3::Zero object results in 0 0 0 0 -0 0. With this branch we can expect all 0 values

there are some behavior changes by setting precision to 6 for all our output streams, though it does make them more consistent

[chapulina]

I added a test case in e3c7b81 that fails on ign-math6

Thank you for the test case. I'm on he fence here about whether it's a good idea to get this PR in. Consistency is good, but behavior change, even if it's almost harmless, can be disruptive for users who have been handling the current behaviour. I can target this at main if you prefer.

[azeey]

I just want to point out that the use of std::fpclassify in libsdformat is limited to the URDF parser within libsdformat (https://github.com/osrf/sdformat/blob/abc6615d4ad3dc22a2756a2fbcbcb7e8662c2fb5/src/parser_urdf.cc#L1113). libsdformat still relies on the operator<< of ignition::math::Pose3d when generating SDFormat output strings. There are tests in libsdformat that expect -0 to be output. I am okay with updating them to 0 immediately after this is merged, but I just wanted to point out the behavior change.

[chapulina]

There are tests in libsdformat that expect -0 to be output. I am okay with updating them to 0 immediately after this is merged, but I just wanted to point out the behavior change.

Ouch that's good to know. I think we should retarget this to main. Ignition Math 6 is used across various projects and has been around for over 2 years. I think it's safe to assume that this will break users while bringing little benefit.

[chapulina]

humm I don't know what's happening on CI:

stderr: error: unable to create file tools/check_test_ran.py (Permission denied)
error: unable to create file tools/code_check.sh (Permission denied)
fatal: cannot create directory at 'tools/cppcheck_rules': Permission denied

Seems related to #211. Any ideas, @j-rivero ?

[j-rivero]

Seems related to #211. Any ideas, @j-rivero ?

For some reason the tools/ directory was under root permissions (could be a build finished unsuccessful). I've change it to jenkins and things are working as expected.

[j-rivero]

Minor suggestion for the Migration document. Looks good to me. I was trying to measure the impact on performance of changes by playing with time command but my results are pretty inconsistent. Given that it will only affect to the output I'm not that worried.

[chapulina]

I was trying to measure the impact on performance of changes by playing with time command but my results are pretty inconsistent. Given that it will only affect to the output I'm not that worried.

That's a good idea. This may actually impact Gazebo, because the stream operators are used for serialization, and that happens often. So let's keep an eye on that 👁️