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

Fixes precision loss when adding nested models. #314

Merged
merged 4 commits into from
Feb 4, 2021

Conversation

hauke76
Copy link
Contributor

@hauke76 hauke76 commented Jul 6, 2020

The precision loss is resolved by explicitly calling std::setprecision
for float and double parameters when these are streamed/requested as
strings.

@hauke76 hauke76 force-pushed the fix_precision_loss branch 2 times, most recently from 85be3d4 to dadc373 Compare July 6, 2020 11:13
@hauke76 hauke76 force-pushed the fix_precision_loss branch from dadc373 to 346e688 Compare July 6, 2020 13:17
@scpeters
Copy link
Member

scpeters commented Jul 6, 2020

addresses #313

@scpeters
Copy link
Member

scpeters commented Jul 7, 2020

I think we would need a test before we could merge this. Fortunately, there are some existing tests for the text output after including nested models (see test/integration/nested_model.cc) that could be modified to increase precision. For example, scpeters@dce1c4d increases the precision of a pose value in a nested model and expects the same precision in the output. Is that the type of thing this is supposed to fix? I tried that test with your branch, and it didn't seem to fix it. With which data types have you tested your branch? I think Vector3 and Quaternion values might have precision limited by the ign-math class:

@hauke76
Copy link
Contributor Author

hauke76 commented Jul 7, 2020

The proposed fix only addresses double and float types stored in the parameter variant. I ran into this when using camera intrinsics such as fx, fy, etc. I already thought about the vector and matrix types. We need special branches to fix that too as the default streaming operators you pointed to only serialize with 6 decimals precision.

I will extend this CL to also fix those types and add a test.

@hauke76 hauke76 force-pushed the fix_precision_loss branch 3 times, most recently from 1fc2107 to c2e6c16 Compare July 7, 2020 13:02
The precision loss is resolved by explicitly calling std::setprecision
for float and double parameters when these are streamed/requested as
strings.

Signed-off-by: Hauke Heibel <[email protected]>
@hauke76 hauke76 force-pushed the fix_precision_loss branch from c2e6c16 to bc247b1 Compare July 7, 2020 13:03
@hauke76
Copy link
Contributor Author

hauke76 commented Jul 7, 2020

I have modified the existing test to show that the correct code path is taken and to show that the logic is correct. Since I have not found an SDF property with 32bit float precision, I only tested for double precision. I used the sphere's radius property which is a double.

All floating point ignition math types are affected as well. I could easily provide full specializations for them as and re-implement the serialization while taking care to exactly copy the math libraries' behavior but I am wondering if the better fix were to remove the precision function invocation in the ignition math library. It were then possible to simply write os << std::setprecision(std::numeric_limits<T>::max_digits10) << v.val for the ignition types. Right now, that won't work.

@EricCousineau-TRI
Copy link
Collaborator

For the std::setprecision(), would it be possible to make sure you reverse any global effects? e.g. using some RAII pattern?

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Jul 30, 2020

@jennuine
Copy link
Collaborator

jennuine commented Feb 3, 2021

For the std::setprecision(), would it be possible to make sure you reverse any global effects?

There are no global effects because the ostream used in the operator<< functions is a locally created stream object

Signed-off-by: Jenn Nguyen <[email protected]>
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Sweet, thank you for checking!

@jennuine jennuine merged commit 28124b0 into gazebosim:master Feb 4, 2021
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.

5 participants