Support printing sdf poses in degrees and allow snapping to commonly used angles by aaronchongth · Pull Request #689 · gazebosim/sdformat

aaronchongth · 2021-09-07T06:13:48Z

🎉 New feature

Related to #646

Summary

Support options in printing,

ign sdf -p PATH
ign sdf -p PATH --degrees
ign sdf -p PATH --snap-to-degrees 15
ign sdf -p PATH --snap-to-degrees 15 --snap-tolerance 0.01

snap-to-degress must be larger than 0, less than or equal to 360.

snap-tolerance must be larger than 0, less than 360, and less than snap-to-degrees.

For example,

<pose>1 2 3   0.523756 0.785555 1.0473546</pose>

Printing options,

ign sdf -p test.sdf --degrees
# <pose degrees='true' rotation_format='euler_rpy'>1 2 3   30.009 45.009 60.009</pose>

ign sdf -p test.sdf --snap-to-degrees 5
# <pose degrees='true' rotation_format='euler_rpy'>1 2 3   30 45 60</pose>

ign sdf -p test.sdf --snap-to-degrees 20
# <pose degrees='true' rotation_format='euler_rpy'>1 2 3   30.009 45.009 60</pose>

ign sdf -p test.sdf --snap-to-degrees 5 --snap-tolerance 0.008
# <pose degrees='true' rotation_format='euler_rpy'>1 2 3   30.009 45.009 60.009</pose>

ign sdf -p test.sdf --snap-to-degrees 5 --snap-tolerance 0.01
# <pose degrees='true' rotation_format='euler_rpy'>1 2 3   30 45 60</pose>

Behavior changes

The process of getting strings from Param will be type dependent, at the moment type pose uses a separate process than the rest of the types, and in the future, other types can adopt similar patterns to handle specific parsing behaviors. See https://github.com/ignitionrobotics/sdformat/blob/aaron/snapping-rotation-printouts/src/Param.cc#L980-L1010
Getting default value strings will obey directly what was provided in the Param constructor, for example this test that needed to be modified, 8b4f471#diff-441337ac422044f4e471bad3d5d6d322f315bf1dd94cace33cd62e4c50072b3eR233

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
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

azeey · 2021-09-21T22:17:00Z

The API changes from this PR have been captured by #708.

codecov-commenter · 2021-10-18T09:46:26Z

Codecov Report

Merging #689 (1871f32) into sdf12 (de356cd) will increase coverage by 0.13%.
The diff coverage is 98.48%.

❗ Current head 1871f32 differs from pull request most recent head 27a6148. Consider uploading reports for the commit 27a6148 to get more accurate results

@@            Coverage Diff             @@
##            sdf12     #689      +/-   ##
==========================================
+ Coverage   90.57%   90.70%   +0.13%     
==========================================
  Files          78       78              
  Lines       12336    12439     +103     
==========================================
+ Hits        11173    11283     +110     
+ Misses       1163     1156       -7

Impacted Files	Coverage Δ
include/sdf/Element.hh	`100.00% <ø> (ø)`
include/sdf/Param.hh	`78.57% <ø> (ø)`
src/Param.cc	`91.28% <97.05%> (+2.14%)`	⬆️
src/Element.cc	`96.96% <100.00%> (+0.06%)`	⬆️
src/PrintConfig.cc	`100.00% <100.00%> (ø)`
src/ign.cc	`64.15% <100.00%> (+1.77%)`	⬆️
src/parser.cc	`87.55% <100.00%> (+0.06%)`	⬆️

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 de356cd...27a6148. Read the comment docs.

src/Element.cc

include/sdf/Element.hh

src/Param.cc

src/ign_TEST.cc

test/integration/pose_1_9_sdf.cc

src/ign_TEST.cc

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

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

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

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

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

aaronchongth · 2021-12-14T16:05:31Z

1400948 reverts all unnecessary changes made to old tests due to the experimental nature of handling parsing since the start of sdf12, specifically by https://github.com/ignitionrobotics/sdformat/pull/589/files

As of now, the behavior of getting strings from values will all reside in ParamPrivate::StringFromValueImpl, which

The default value to string process uses ParamStreamer (which gives us the old behavior)
bool and pose has their own type specific methods of extracting the strings

Apologies for all the back and forth regarding modifying old tests. As of 1400948 we should be good to go and no longer have to worry about changes in behavior, unless another type requires its own value-to-string method.

azeey

I was struggling to understand what ToString behavior is with floating point numbers since we are now changing behavior again. Here's what I've come up so far to summarize the current behavior.

If the element is set by the user, the string value used by the user is printed verbatim when ToString is called (except for Poses whose behavior depends on the PrintConfig setting).
Sometimes, if the element is set by the user, the value is read by the libsdformat and converted to a binary representation. When ToString is called, the binary representation is converted back to a string.
If the element is not present, but a default element is added by libsdformat, the string value is read from the default attribute of the element's description, converted to a binary representation, and converted back to a string.

I then used the following file to test the behavior.

<sdf version="1.9">
  <world name="default">
    <model name="M1">
      <link name="L1">
        <visual name="v1">
          <material>
            <diffuse>0.15 0.6 0.8</diffuse><!-- (1) String printed verbatim. Type: color -->
            <render_order>0.15</render_order><!-- (2) Converted to binary then to string. Type: float -->
          </material>
          <transparency>0.15</transparency><!-- (2) Converted to binary then to string. Type: double -->
        </visual>
        <sensor name='camera' type='camera'>
          <pose>-1.06 0.090912 0 0 0 3.14</pose> <!-- (1) String printed verbatim. Type: pose -->
          <camera>
            <horizontal_fov>1.40</horizontal_fov> <!-- (2) Converted to binary then to string. Type: double -->
            <clip>
              <near>0.15</near><!-- (2) Converted to binary then to string. Type: double -->
            </clip>
            <distortion>
              <center>0.15 1.4</center> <!-- (1) String printed verbatim. Type: vector2 -->
            </distortion>
          </camera>
        </sensor>
      </link>
      <joint name="j1" type="revolute">
        <parent>world</parent>
        <child>L1</child>
        <axis>
          <xyz>0.12 0.34 0.56</xyz> <!-- (1) String printed verbatim. Type: vector3 -->
        </axis>
      </joint>
    </model>
    <!--<magnetig_field> (3) Default string converted to binary then to string. Type: vector3 -->
  </world>
</sdf>

Output from ign sdf -p

<sdf version='1.9'>
  <world name='default'>
    <model name='M1'>
      <link name='L1'>
        <visual name='v1'>
          <material>
            <diffuse>0.15 0.6 0.8 1</diffuse>
            <render_order>0.150000006</render_order>
          </material>
          <transparency>0.14999999999999999</transparency>
          <geometry/>
        </visual>
        <sensor name='camera' type='camera'>
          <pose>-1.06 0.090912 0 0 0 3.14</pose>
          <camera>
            <horizontal_fov>1.3999999999999999</horizontal_fov>
            <clip>
              <near>0.14999999999999999</near>
              <far>100</far>
            </clip>
            <distortion>
              <center>0.15 1.4</center>
            </distortion>
            <image>
              <width>320</width>
              <height>240</height>
            </image>
          </camera>
        </sensor>
      </link>
      <joint name='j1' type='revolute'>
        <parent>world</parent>
        <child>L1</child>
        <axis>
          <xyz>0.12 0.34 0.56</xyz>
          <limit>
            <lower>-10000000000000000</lower>
            <upper>10000000000000000</upper>
          </limit>
        </axis>
      </joint>
    </model>
    <gravity>0 0 -9.8</gravity>
    <magnetic_field>6e-06 2.3e-05 -4.2e-05</magnetic_field>
    <atmosphere type='adiabatic'/>
    <physics type='ode'>
      <max_step_size>0.001</max_step_size>
      <real_time_factor>1</real_time_factor>
      <real_time_update_rate>1000</real_time_update_rate>
    </physics>
    <scene>
      <ambient>0.4 0.4 0.4 1</ambient>
      <background>0.7 0.7 0.7 1</background>
      <shadows>true</shadows>
    </scene>
  </world>
</sdf>

My observation is the behavior is inconsistent between double types and vector* types. Would it be possible to eliminate this inconsistency?

azeey · 2021-12-15T17:26:24Z

src/PrintConfig.cc

-      this->GetRotationSnapTolerance() == _config.GetRotationSnapTolerance())
+  if (this->RotationInDegrees() == _config.RotationInDegrees() &&
+      this->RotationSnapToDegrees() == _config.RotationSnapToDegrees() &&
+      this->RotationSnapTolerance() == _config.RotationSnapTolerance())


Should we check for PreserveIncludes now that that's merged?

Added in b541f34, thanks for spotting that!

aaronchongth · 2021-12-17T10:36:50Z

Many thanks for testing it extensively, @azeey! 🙇

I was struggling to understand what ToString behavior is with floating point numbers since we are now changing behavior again.
I apologize for the switching behavior around so often since the introduction of parsing-using-parent-element-attributes. I think a clarification question I should have asked earlier is, would we ideally like to revert all the changes made to tests/test-files back to how it was in sdf11, with only changes related to pose and bool?

This is related to #689 (comment), in order to avoid that change (returning "0"), we would need the returned string to be retrieved from parsing the binary value, instead of the string/default-string that was used to set the value. Hence 1400948.

However if we decide to keep the changes in behavior (returning string/default-string that was set), we would need the aforementioned test to return "0.0", as that was the default string value.

This will affect this observation,

If the element is set by the user, the string value used by the user is printed verbatim when ToString is called (except for Poses whose behavior depends on the PrintConfig setting).

aaronchongth · 2021-12-17T10:58:14Z

My observation is the behavior is inconsistent between double types and vector* types. Would it be possible to eliminate this inconsistency?

This is due to the different implementations of operator<< for

double and float
- https://github.com/ignitionrobotics/sdformat/blob/sdf12/include/sdf/Param.hh#L89-L101
- specifically using std::setprecision(std::numeric_limits<double>::max_digits10), 28124b0
ignition::math::Vector2, ignition::math::Vector3

if we remove the template specialization for double and float, it returns

<sdf version='1.9'>
  <world name='default'>
    <model name='M1'>
      <link name='L1'>
        <visual name='v1'>
          <material>
            <diffuse>0.15 0.6 0.8</diffuse>
            <render_order>0.15</render_order>
          </material>
          <transparency>0.15</transparency>
          ...

azeey · 2021-12-17T22:28:29Z

Thanks for the explanation. Eric has created #790 to address the inconsistency. I have tested this PR against all other ignition libraries and had no failures, so I'll go ahead and approve.

azeey

There are a couple of unresolved conversations. Otherwise LGTM. Thanks for your patience.

azeey · 2021-12-17T22:34:13Z

src/ign.cc


 //////////////////////////////////////////////////
-extern "C" SDFORMAT_VISIBLE int cmdPrint(const char *_path)
+extern "C" SDFORMAT_VISIBLE int cmdPrint(const char *_path,


Any thoughts?

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

…arse booleans into true/false, instead of 1/0 Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

aaronchongth · 2021-12-18T16:36:32Z

I noticed the recent merge for test/integration/print_config.cc, https://github.com/ignitionrobotics/sdformat/blob/sdf12/test/integration/print_config.cc#L193, where the pose is expected to have a -0 in the pitch.

Ideally we would expect the sanitization for -0 to do it's work here, however after tracing the code path, I noticed that this is due to the use of Param::SetFromString on the result of ignition::math::Pose3d::operator<<, where down the line the string was returned directly, as there were no parent element attributes or PrintConfig modifications happening.

I couldn't find any persuasive solutions to this without breaking more stuff. I would suggest we move on with the merge, however keep an eye out for the next release of ign-math so that we can utilize gazebosim/gz-math#206, and make changes to this test.

Let me know what you think.

…ng instead of default PrintConfig Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

scpeters · 2021-12-20T21:06:06Z

I couldn't find any persuasive solutions to this without breaking more stuff. I would suggest we move on with the merge, however keep an eye out for the next release of ign-math so that we can utilize ignitionrobotics/ign-math#206, and make changes to this test.

Let me know what you think.

that sounds good to me

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

scpeters · 2021-12-30T17:59:36Z

src/parser.cc

-        element->AddAttribute(attribute->Name(), "string", "", 1, "");
-        element->GetAttribute(attribute->Name())->SetFromString(
+        const std::string attributeName(attribute->Name());
+        if (attributeName == "type")


what if a plugin uses the type attribute?

we are observing some ign-gazebo test failures of the triggered_publisher test, and that plugin uses attributes with name type:

Bumps in garden: use ign-math7 and dependents gz-sim#1264 (comment)

https://github.com/ignitionrobotics/ign-gazebo/blob/ign-gazebo6/test/worlds/triggered_publisher.sdf#L5

😄 I had just started writing a similar response here:

This broke plugins downstream on ign-gazebo. Loading a plugin like this:

<plugin filename="ignition-gazebo-triggered-publisher-system" name="ignition::gazebo::systems::TriggeredPublisher"> <input type="ignition.msgs.Empty" topic="/in_1"/> <output type="ignition.msgs.Boolean" topic="/out_1"> data: true </output> </plugin>

Results in an assertion when the SDF is loaded:

173: Error [Param.cc:768] Unknown parameter type[ignition.msgs.Boolean] 173: Exception [Param.cc:81] SDF ASSERTION 173: Invalid parameter 173: In function : Param 173: Assert expression : this->dataPtr->ValueFromStringImpl( this->dataPtr->typeName, _default, this->dataPtr->defaultValue)

I'm testing a temporary fix

This is what I'm trying: #809

osrf-triage · 2022-04-13T21:31:01Z

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

github-actions bot added the 🏯 fortress Ignition Fortress label Sep 7, 2021

chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021

azeey mentioned this pull request Sep 9, 2021

proposal: Should allow //pose rotation to be specified in degrees? #252

Closed

4 tasks

chapulina removed the beta Targeting beta release of upcoming collection label Sep 20, 2021

jennuine mentioned this pull request Sep 21, 2021

API changes for PrintConfig #708

Merged

8 tasks

aaronchongth changed the base branch from main to sdf12 October 4, 2021 09:57

aaronchongth force-pushed the aaron/snapping-rotation-printouts branch 2 times, most recently from 2a2b58c to 983362f Compare October 10, 2021 18:07

aaronchongth marked this pull request as ready for review October 20, 2021 15:04

aaronchongth requested a review from azeey October 20, 2021 15:04

azeey requested changes Oct 27, 2021

View reviewed changes

aaronchongth added 17 commits November 8, 2021 16:13

Ruby option to print in_degrees or snap_to_degrees

dffcb8d

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

Basic PrintConfig added

a548788

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

PrintConfig gets passed into printing implementations of Element and …

7bff621

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

Adding basic test for print options

648e2c1

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

Reverting to PrintConfig with basic API

942a180

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

Moved creation of PrintConfig into ign functions

8dbd8a3

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

Param value GetPoseAsString and tests

d1e9dd1

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

Moved attribute painting to its own function, fixed test strings

347d92d

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

Added basic tests for pose rotation input as quaternions

9de0da6

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

Using different flags for ign sdf -p, allow snapping to different values

91fb532

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

Disabling test on windows, fixing comment

085a372

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

Remove stale function, fixed linting

ac0df14

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

Adding tolerance as a argument, added tests

8dd6e5e

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

Use 3 spaces when changing rotation formats or snapping to degrees

bff39f4

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

Added check for tolerance larger than snapping interval

c126e19

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

Moving PrintAttributes to ElementPrivate to remain ABI stability

f600b74

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

Using true/false instead of 1/0

0c0f296

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

aaronchongth added 6 commits December 13, 2021 18:58

Removed Get from PrintConfig getter functions

b426498

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

Using std optional's converting constructor

b69f448

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

Modified snapToInterval implementation, added test

236e858

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

Added bool type specific value parser, values are parsed using ParamS…

4667c4b

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

Merge branch 'sdf12' into aaron/snapping-rotation-printouts

a7dfafb

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

Reverting all unnecessary changes made in sdf12 to old tests

1400948

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

aaronchongth requested a review from azeey December 14, 2021 16:07

azeey reviewed Dec 17, 2021

View reviewed changes

EricCousineau-TRI mentioned this pull request Dec 17, 2021

Use consistent float / double precision by using configuring the ostream object precision? #790

Closed

azeey approved these changes Dec 17, 2021

View reviewed changes

aaronchongth added 3 commits December 18, 2021 12:24

Merge branch 'sdf12' into aaron/snapping-rotation-printouts

b3c4a5b

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

Added comparison for PreserveIncludes

b541f34

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

Check for 'type' attribute in unknown elements as well, in order to p…

0675571

…arse 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 stri…

3c7d932

…ng instead of default PrintConfig Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

aaronchongth added 2 commits December 21, 2021 17:10

Added comment regarding sanitizing -0 in test outputs

54b24a5

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

Merge branch 'sdf12' into aaron/snapping-rotation-printouts

27a6148

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

aaronchongth merged commit 5e91938 into sdf12 Dec 21, 2021

aaronchongth deleted the aaron/snapping-rotation-printouts branch December 21, 2021 11:37

scpeters mentioned this pull request Dec 30, 2021

Bumps in garden: use ign-math7 and dependents gazebosim/gz-sim#1264

Merged

scpeters reviewed Dec 30, 2021

View reviewed changes

chapulina mentioned this pull request Dec 30, 2021

Fix parsing 'type' attibutes in plugins #809

Merged

7 tasks

EricCousineau-TRI mentioned this pull request Jan 10, 2022

Implement snap-to feature for //pose/@rotation_type conversion #646

Closed

jennuine mentioned this pull request Mar 9, 2022

Changed default floating point precision to max #872

Merged

8 tasks

Conversation

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

Uh oh!

🎉 New feature

Summary

Behavior changes

Checklist

Uh oh!

azeey commented Sep 21, 2021

Uh oh!

codecov-commenter commented Oct 18, 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!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

azeey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azeey Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

aaronchongth Dec 18, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

azeey commented Dec 17, 2021

Uh oh!

azeey left a comment

Choose a reason for hiding this comment

Uh oh!

azeey Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

aaronchongth commented Dec 18, 2021

Uh oh!

scpeters commented Dec 20, 2021

Uh oh!

scpeters Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

chapulina Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

chapulina Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

osrf-triage commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aaronchongth commented Sep 7, 2021 •

edited

Loading

codecov-commenter commented Oct 18, 2021 •

edited

Loading

aaronchongth commented Dec 14, 2021 •

edited

Loading

azeey left a comment •

edited

Loading

aaronchongth commented Dec 17, 2021 •

edited

Loading

aaronchongth commented Dec 17, 2021 •

edited

Loading