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

Normalize joint axis xyz vector when parsing from SDFormat #312

Merged
merged 10 commits into from
Aug 7, 2020

Conversation

azeey
Copy link
Collaborator

@azeey azeey commented Jul 2, 2020

Closes #307. Note that normalization is done only when parsing from SDFormat, i.e, values set via JointAxis::SetXyz are not normalized.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf10     #312   +/-   ##
=======================================
  Coverage   87.50%   87.51%           
=======================================
  Files          60       60           
  Lines        9085     9090    +5     
=======================================
+ Hits         7950     7955    +5     
  Misses       1135     1135           
Impacted Files Coverage Δ
src/JointAxis.cc 94.15% <100.00%> (+0.11%) ⬆️
include/sdf/Types.hh 100.00% <0.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 99b5bbe...c89eea8. Read the comment docs.

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

looks good, just needs entries in the changelog and migration guide

@scpeters
Copy link
Member

scpeters commented Jul 5, 2020

ready for review from @EricCousineau-TRI

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Copy link
Collaborator Author

azeey commented Jul 8, 2020

@osrf-jenkins run tests please

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.

Looks awesome, thanks!!!

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Copy link
Collaborator Author

azeey commented Jul 15, 2020

I've looked through the libsdformat code and we do already use SDF_ASSERT in a few places (Element.cc:970, Param.cc:71), so I've added it in JointAxis::SetXyz. But I would note that all of the existing uses of SDF_ASSERT are in code that provides the sdf::Element API, so this the first use of exceptions in the DOM API.

@EricCousineau-TRI
Copy link
Collaborator

Per f2f, if the return type is changed from void to Error, then the function should gain a [[nodiscard]] attribute so that warnings can be thrown.

include/sdf/JointAxis.hh Outdated Show resolved Hide resolved
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

looks good once the compiler error is fixed

@azeey
Copy link
Collaborator Author

azeey commented Jul 28, 2020

@osrf-jenkins run tests please

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@EricCousineau-TRI
Copy link
Collaborator

Per f2f, this is most likely broken due to some oddities with github actions (e.g. fetching opaquely from pull/XYZ/merge rather than pull/XYZ/head).

@azeey azeey merged commit 5b53e44 into gazebosim:sdf10 Aug 7, 2020
scpeters added a commit to scpeters/sdformat that referenced this pull request Aug 17, 2020
…#312)

* Normalize joint axis xyz vector when parsing from SDFormat

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
scpeters added a commit that referenced this pull request Aug 18, 2020
* Normalize joint axis xyz vector when parsing from SDFormat

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
@dirk-thomas
Copy link
Contributor

@azeey This pull request seem to have introduced the following compiler warnings: https://build.osrfoundation.org/job/ignition_launch-ci-master-bionic-amd64/10/

[Discovered during osrf/buildfarmer#84]

@scpeters
Copy link
Member

@azeey This pull request seem to have introduced the following compiler warnings: https://build.osrfoundation.org/job/ignition_launch-ci-master-bionic-amd64/10/

[Discovered during osrf/buildfarmer#84]

yes, this has caused some test failures, now that it has been included in the prerelease 10.0.0pre210.0.0pre2

the test failures will be fixed by gazebosim/gz-sim#288

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.

None yet

5 participants