Fix empty pose parsing fail for rotation_format='quat_xyzw' by aaronchongth · Pull Request #729 · gazebosim/sdformat

aaronchongth · 2021-10-11T11:31:32Z

🦟 Bug fix

Fixes #707

Summary

Added check for input if it matches the default string specified in the pose descriptions.

Input string will be swapped to the correct default values based on the rotation format of the element, 0 0 0 0 0 0 for @rotation_format='euler_rpy' and 0 0 0 0 0 0 1 for @rotation_format='quat_xyzw',

if the input string is empty (this means the Param was created with an empty default value string), or
if the input string matches the default value string specified in the pose description (this means that this pose's value was not set yet)

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

src/Param.cc

test/integration/pose_1_9_sdf.cc

codecov-commenter · 2021-10-25T16:49:31Z

Codecov Report

Merging #729 (70366dd) into sdf12 (d1b096c) will decrease coverage by 0.06%.
The diff coverage is 84.92%.

@@            Coverage Diff             @@
##            sdf12     #729      +/-   ##
==========================================
- Coverage   89.22%   89.15%   -0.07%     
==========================================
  Files          76       76              
  Lines       11905    12001      +96     
==========================================
+ Hits        10622    10700      +78     
- Misses       1283     1301      +18

Impacted Files	Coverage Δ
include/sdf/Param.hh	`78.57% <ø> (ø)`
src/parser.cc	`87.51% <50.00%> (+0.10%)`	⬆️
src/Param.cc	`89.13% <85.48%> (-2.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 d1b096c...70366dd. Read the comment docs.

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

…rmat is defined Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

…ave not been assigned to param Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

include/sdf/Param.hh

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

include/sdf/Param.hh

src/Param.cc

include/sdf/Param.hh

src/Param.cc

azeey · 2021-11-10T19:27:02Z

src/Param.cc

+  }
+  else if (rotationFormat == "quat_xyzw")
+  {
+    ss << pose->Pos() << " "


Use 3 spaces.

Using 3 spaces unless attributes are not set, ef54667

azeey · 2021-11-10T19:27:08Z

src/Param.cc

+  }
+  else if (rotationFormat == "euler_rpy" && inDegrees)
+  {
+    ss << pose->Pos() << " "


Use 3 spaces.

Using 3 spaces unless attributes are not set, ef54667

azeey · 2021-11-10T19:27:13Z

src/Param.cc

+    return true;
+  }
+
+  ss << pose->Pos() << " "


Use 3 spaces.

Using 3 spaces unless attributes are not set, ef54667

src/Param.cc

…ead of pointer Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

… to nullopt Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

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

azeey · 2021-11-11T22:49:15Z

@aaronchongth It would be nice not to introduce or at least minimize the changes in the output of ToString. I have suggested changes in #748. Let me know what you think.

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

…es are set Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

azeey · 2021-11-22T15:33:38Z

Per VC, check that Reparse() doesn't get called in the code path from ign sdf -p where a non-default PrintConfig object is created. This is because Reparse creates a default PrintConfig potentially overriding the one passed in from ign sdf -p.

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

aaronchongth · 2021-11-23T08:00:10Z

@azeey I've just gone through it again, and added a comment here to describe why a default PrintConfig can be used, 594506a. This is ready for another round of review. Thanks!

…used angles (#689) * Ruby option to print in_degrees or snap_to_degrees Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Basic PrintConfig added Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * PrintConfig gets passed into printing implementations of Element and Param Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Adding basic test for print options Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Reverting to PrintConfig with basic API Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Moved creation of PrintConfig into ign functions Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Param value GetPoseAsString and tests Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Moved attribute painting to its own function, fixed test strings Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added basic tests for pose rotation input as quaternions Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Using different flags for ign sdf -p, allow snapping to different values Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Disabling test on windows, fixing comment Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Remove stale function, fixed linting Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Adding tolerance as a argument, added tests Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Use 3 spaces when changing rotation formats or snapping to degrees Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added check for tolerance larger than snapping interval Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Moving PrintAttributes to ElementPrivate to remain ABI stability Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Using true/false instead of 1/0 Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Remove use of SDF_ASSERT in GetAsString Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added tests for //include/pose Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Adding parsing passing test for empty quat_xyzw pose Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added check for default string values to be modified when rotation_format is defined Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added tests Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Reparsing translates default value into string to be used if values have not been assigned to param Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Using StringFromValueImpl for getting strings from all ParamVariants Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Refactor pose string from value into its own function Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Fixing casting erroerror, added documentation and tests for tolerance < interval Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Correcting stale comments Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Fixing snapToInterval math, added more tests Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Removed unneeded visibility macro Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Adding return documentation and using const reference to variant instead of pointer Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Returning string directly, removing stale _config, reverting strValue to nullopt Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Remove use of assertions Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Suggested changes to #729 (#748) Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org> * Using three space delimiter between position and rotation if attributes are set Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added comment regarding use of default PrintConfig in Reparse Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Adding equality comparison for PrintConfig Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Removed stale include Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Uniied string and value parsing behavior, and modified necessary tests Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Overloaded function to maintain ABI stability Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Fixing missing space in test for exec command Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Adding comment regarding attributeExceptions Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Indenting help message, adding test for shuffling command flags Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Modifying cmd flag shuffling test to handling flags with arguments too Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Removed Get from PrintConfig getter functions Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Using std optional's converting constructor Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Modified snapToInterval implementation, added test Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added bool type specific value parser, values are parsed using ParamStreamer by default Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Reverting all unnecessary changes made in sdf12 to old tests Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added comparison for PreserveIncludes Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Check for 'type' attribute in unknown elements as well, in order to parse booleans into true/false, instead of 1/0 Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Only checking for pose related PrintConfig options for returning string instead of default PrintConfig Signed-off-by: Aaron Chong <aaronchongth@gmail.com> * Added comment regarding sanitizing -0 in test outputs Signed-off-by: Aaron Chong <aaronchongth@gmail.com> Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>

aaronchongth requested a review from azeey October 11, 2021 11:31

github-actions bot added 🌱 garden Ignition Garden 🏯 fortress Ignition Fortress labels Oct 11, 2021

aaronchongth changed the title ~~Aaron/pose quat empty value~~ Fix empty pose parsing fail for rotation_format='quat_xyzw' Oct 11, 2021

azeey requested changes Oct 14, 2021

View reviewed changes

src/Param.cc Outdated Show resolved Hide resolved

test/integration/pose_1_9_sdf.cc Show resolved Hide resolved

aaronchongth requested a review from azeey October 25, 2021 18:10

aaronchongth linked an issue Nov 1, 2021 that may be closed by this pull request

Empty pose not interpreted as identity when rotation_format=quat_xyzw is set #707

Closed

aaronchongth added 6 commits November 8, 2021 17:40

Adding parsing passing test for empty quat_xyzw pose

dafe4d1

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Added check for default string values to be modified when rotation_fo…

2299089

…rmat is defined Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Added tests

735042d

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Reparsing translates default value into string to be used if values h…

840c7b4

…ave not been assigned to param Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Using StringFromValueImpl for getting strings from all ParamVariants

c9dd236

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Refactor pose string from value into its own function

832fca1

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

aaronchongth force-pushed the aaron/pose-quat-empty-value branch from 5f559fc to 832fca1 Compare November 8, 2021 09:41

scpeters reviewed Nov 8, 2021

View reviewed changes

include/sdf/Param.hh Outdated Show resolved Hide resolved

Removed unneeded visibility macro

3456422

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

azeey reviewed Nov 10, 2021

View reviewed changes

aaronchongth added 3 commits November 11, 2021 12:47

Adding return documentation and using const reference to variant inst…

4cf502c

…ead of pointer Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Returning string directly, removing stale _config, reverting strValue…

e903b13

… to nullopt Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Remove use of assertions

4eaf06b

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

azeey pushed a commit that referenced this pull request Nov 11, 2021

Suggested changes to #729

73b1294

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

azeey pushed a commit that referenced this pull request Nov 11, 2021

Suggested changes to #729

3f60481

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

azeey mentioned this pull request Nov 11, 2021

Suggested changes to #729 #748

Merged

Addisu Z. Taddese and others added 3 commits November 12, 2021 10:17

Suggested changes to #729 (#748)

7df7a9f

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

Using three space delimiter between position and rotation if attribut…

ef54667

…es are set Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Merge branch 'sdf12' into aaron/pose-quat-empty-value

cb45ae4

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

aaronchongth requested a review from azeey November 12, 2021 05:51

aaronchongth mentioned this pull request Nov 12, 2021

Support printing sdf poses in degrees and allow snapping to commonly used angles #689

Merged

8 tasks

aaronchongth added 2 commits November 23, 2021 14:04

Merge branch 'sdf12' into aaron/pose-quat-empty-value

3e0076e

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Added comment regarding use of default PrintConfig in Reparse

594506a

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

Merge branch 'sdf12' into aaron/pose-quat-empty-value

70366dd

azeey approved these changes Dec 1, 2021

View reviewed changes

azeey merged commit b95f44d into sdf12 Dec 1, 2021

azeey deleted the aaron/pose-quat-empty-value branch December 1, 2021 21:16

Conversation

aaronchongth commented Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦟 Bug fix

Summary

Checklist

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

azeey Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

aaronchongth Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

azeey Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

aaronchongth Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

azeey Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

aaronchongth Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azeey commented Nov 11, 2021

Uh oh!

azeey commented Nov 22, 2021

Uh oh!

aaronchongth commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aaronchongth commented Oct 11, 2021 •

edited

Loading

codecov-commenter commented Oct 25, 2021 •

edited

Loading

aaronchongth commented Nov 23, 2021 •

edited

Loading