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

Return positive infinity instead of -1 for boundless positive values #357

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Sep 4, 2020

Targeted at sdf10, this is a resubmission of #249, which itself was a revival of bitbucket PR 458 per #231. I've added tests in 148d782. cc @mxgrey

Original description:

SDFormat has a convention of using -1 to represent positive infinity. If users of the DOM API are not careful and cognizant of this convention, it can result in confusing bugs that are hard to track down. My dartsim plugin was exhibiting subtle non-physical behavior after loading structures from SDF, because it had effort and velocity limits of +1 (minimum) and -1 (maximum).

This PR is a suggested tweak to the API behavior so that the API returns positive infinity instead of -1 to represent boundlessly positive values. Hopefully this might save other SDF users from long, confusing debugging sessions.

However, this does deviate from the exact convention of the SDF document specification, so I’d understand if this PR is declined.

@scpeters scpeters added this to the libsdformat10 milestone Sep 4, 2020
@scpeters scpeters requested a review from azeey September 4, 2020 00:56
@scpeters
Copy link
Member Author

scpeters commented Sep 4, 2020

@mxgrey if you want to amend with a --signoff your old commit migrated from bitbucket, it will make the DCO bot happy

@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2020

Codecov Report

Merging #357 into sdf10 will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf10     #357   +/-   ##
=======================================
  Coverage   87.32%   87.33%           
=======================================
  Files          60       60           
  Lines        9216     9220    +4     
=======================================
+ Hits         8048     8052    +4     
  Misses       1168     1168           
Impacted Files Coverage Δ
src/Utils.hh 100.00% <ø> (ø)
src/JointAxis.cc 94.15% <100.00%> (ø)
src/Utils.cc 100.00% <100.00%> (ø)

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 784eb19...0227719. Read the comment docs.

@chapulina chapulina added the 🔮 dome Ignition Dome label Sep 4, 2020
@scpeters
Copy link
Member Author

scpeters commented Sep 4, 2020

@osrf-jenkins run tests please

@scpeters scpeters added the beta Targeting beta release of upcoming collection label Sep 9, 2020
@scpeters
Copy link
Member Author

scpeters commented Sep 9, 2020

@osrf-jenkins run tests please

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM, I just have a minor comment about documentation.

include/sdf/JointAxis.hh Outdated Show resolved Hide resolved
mxgrey and others added 3 commits September 10, 2020 16:41
Add example file with default, finite and infinite
joint limit values, using both "inf" and "-1" to
specify infinite values of the effort and velocity
limit.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters merged commit 5b02ffe into sdf10 Sep 14, 2020
@scpeters scpeters deleted the infinite_if_negative_10 branch September 14, 2020 18:15
@scpeters
Copy link
Member Author

I think this doesn't require any downstream changes in ion-physics, since it is already using a similar conversion:

azeey pushed a commit to azeey/sdformat that referenced this pull request Dec 9, 2020
…azebosim#357)

SDFormat has a convention of using -1 to represent positive infinity
for symmetric joint limits on velocity and effort.
If users of the DOM API are not careful and cognizant of this
convention, it can result in confusing bugs that are hard to track down.

This PR changes the API behavior so that the API returns positive
infinity instead of -1 to represent boundlessly positive values.
Hopefully this might save other SDF users from long, confusing
debugging sessions.

A test is added with an example file with default, finite and infinite
joint limit values, using both "inf" and "-1" to specify infinite values
of the effort and velocity limit.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

Co-authored-by: Michael X. Grey <grey@openrobotics.org>
brawner pushed a commit that referenced this pull request Dec 11, 2020
…357)

SDFormat has a convention of using -1 to represent positive infinity
for symmetric joint limits on velocity and effort.
If users of the DOM API are not careful and cognizant of this
convention, it can result in confusing bugs that are hard to track down.

This PR changes the API behavior so that the API returns positive
infinity instead of -1 to represent boundlessly positive values.
Hopefully this might save other SDF users from long, confusing
debugging sessions.

A test is added with an example file with default, finite and infinite
joint limit values, using both "inf" and "-1" to specify infinite values
of the effort and velocity limit.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

Co-authored-by: Michael X. Grey <grey@openrobotics.org>
brawner pushed a commit that referenced this pull request Jan 9, 2021
…357)

SDFormat has a convention of using -1 to represent positive infinity
for symmetric joint limits on velocity and effort.
If users of the DOM API are not careful and cognizant of this
convention, it can result in confusing bugs that are hard to track down.

This PR changes the API behavior so that the API returns positive
infinity instead of -1 to represent boundlessly positive values.
Hopefully this might save other SDF users from long, confusing
debugging sessions.

A test is added with an example file with default, finite and infinite
joint limit values, using both "inf" and "-1" to specify infinite values
of the effort and velocity limit.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

Co-authored-by: Michael X. Grey <grey@openrobotics.org>
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 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants