NavSat::ToElement() method by ntfshard · Pull Request #1585 · gazebosim/sdformat

ntfshard · 2025-09-02T17:41:23Z

🦟 Bug fix

Fixes #

Summary

sdf::NavSat didn't have a ToElement() method which prevent to convert sensor-specific information back to sdf format.

This PR include such changes:

Introduce sdf::NavSat::ToElement() method
Added this method call to dispatcher in Sensor::ToElement()
Also same dispatcher had misleading error msg: if user tried to convert sensor w/o sensor-specific information it will get error message about lack of this feature, but actually it's not correct. Dispatcher logic changed to avoid such misleading (split apart type check and pointer check)
And message was w/o trailing new_line symbol which makes console output harder to read
Added test for new ToElement() method; valgrind is happy
Couple of typos fixed

This PR was developed on top of sdf15 and retargeted to sdf16 due to sdf16 do not have dev packages.

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
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
Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by and Generated-by messages.

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>

ntfshard · 2025-09-02T17:44:13Z

+    /// Note that parameter passing functionality is not captured with this
+    /// function.


I copied this text from similar method of other class (at least it consistent)

azeey · 2025-09-02T19:37:48Z

Have you seen #1538?

ntfshard · 2025-09-03T01:59:26Z

Have you seen #1538?

No

UPD: I tried to use python bindings to generate sdf files and got error msg, which bring me to missing implementation. Now I see this PR is duplicates implementation of already opened PR

ntfshard · 2025-12-08T17:14:18Z

Have you seen #1538?

What would you suggest to do in this case? Fortunately we rewrote sdf generation on a pure python and don't constrained by this PR, but I'd like to wrap up this topic in any way.

ntfshard · 2026-02-17T06:40:43Z

@azeey could you please elaborate

azeey

Looks good. I'd just ask that you add the version that takes an sdf::Error. I'll close #1538 since it's no longer active and this PR has tests.

azeey · 2026-02-18T19:57:50Z

 }
+
+/////////////////////////////////////////////////
+sdf::ElementPtr NavSat::ToElement() const


We've been adding an implementation that takes an sdf:Error (see

sdformat/src/Noise.cc

Line 265 in dcd3c2d

sdf::ElementPtr Noise::ToElement(sdf::Errors &_errors) const

). Can you add that please?

Done.

Still I'm quite curious how we can get a error status (except invalid tag as a parameter). I mean, when parsing a user's input, error is not something unexpected, but here we are constructing tag from scratch basically.

It is possible for there to be errors in the spec (e.g., navsat.sdf), especially if the user modified it in their own copy of libsdformat. It's also more consistent if all the ToElement functions have the same signature.

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>

ntfshard · 2026-02-23T17:40:18Z

+  if (this->Type() == sdf::SensorType::AIR_PRESSURE)
  {
-    sdf::ElementPtr airPressureElem = elem->GetElement("air_pressure");
-    airPressureElem->Copy(this->dataPtr->airPressure->ToElement());


Just realized, in this function mix of functions is used. _error passed only in 3 functions above and code below only writing in else branch of dispatcher.

Also GetAttribute seems should have version with error parameter to match GetElement. But maybe I'm not right in a big picture.

I may have missed some in my review. If there are any more functions that accept _error, we should definitely be using them. Please feel free to create a follow up PR.

azeey · 2026-02-23T18:50:36Z

 }
+
+/////////////////////////////////////////////////
+sdf::ElementPtr NavSat::ToElement() const


It is possible for there to be errors in the spec (e.g., navsat.sdf), especially if the user modified it in their own copy of libsdformat. It's also more consistent if all the ToElement functions have the same signature.

azeey · 2026-02-23T18:51:58Z

+sdf::ElementPtr NavSat::ToElement(sdf::Errors &_errors) const
+{
+  sdf::ElementPtr elem(new sdf::Element);
+  sdf::initFile("navsat.sdf", elem);


Suggested change

sdf::initFile("navsat.sdf", elem);

sdf::initFile("navsat.sdf", ParserConfig::GlobalConfig(), elem, _errors);

azeey · 2026-02-23T18:52:07Z

+  {
+    auto el = elem->GetElement("position_sensing", _errors)->
+        GetElement("horizontal", _errors)->GetElement("noise", _errors);
+    el->Copy(this->dataPtr->horizontalPositionNoise.ToElement(), _errors);


Suggested change

el->Copy(this->dataPtr->horizontalPositionNoise.ToElement(), _errors);

el->Copy(this->dataPtr->horizontalPositionNoise.ToElement(_errors), _errors);

Good catch, thank you

azeey · 2026-02-23T18:52:17Z

+  {
+    auto el = elem->GetElement("position_sensing", _errors)->
+        GetElement("vertical", _errors)->GetElement("noise", _errors);
+    el->Copy(this->dataPtr->verticalPositionNoise.ToElement(), _errors);


Suggested change

el->Copy(this->dataPtr->verticalPositionNoise.ToElement(), _errors);

el->Copy(this->dataPtr->verticalPositionNoise.ToElement(_errors), _errors);

azeey · 2026-02-23T18:52:25Z

+  {
+    auto el = elem->GetElement("velocity_sensing", _errors)->
+        GetElement("horizontal", _errors)->GetElement("noise", _errors);
+    el->Copy(this->dataPtr->horizontalVelocityNoise.ToElement(), _errors);


Suggested change

el->Copy(this->dataPtr->horizontalVelocityNoise.ToElement(), _errors);

el->Copy(this->dataPtr->horizontalVelocityNoise.ToElement(_errors), _errors);

azeey · 2026-02-23T18:52:32Z

+  {
+    auto el = elem->GetElement("velocity_sensing", _errors)->
+        GetElement("vertical", _errors)->GetElement("noise", _errors);
+    el->Copy(this->dataPtr->verticalVelocityNoise.ToElement(), _errors);


Suggested change

el->Copy(this->dataPtr->verticalVelocityNoise.ToElement(), _errors);

el->Copy(this->dataPtr->verticalVelocityNoise.ToElement(_errors), _errors);

ntfshard · 2026-02-24T01:41:33Z

  // camera, depth, thermal, segmentation
  else if (this->CameraSensor())
  {
+    // TODO(anyone) use ToElement with sdf::Error when it available


If I already touched structure of this function, added _error where it was obvious and compiler not angry

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>

ntfshard · 2026-03-03T10:02:42Z

@azeey is it OK now?

azeey · 2026-03-03T19:21:22Z

+  if (this->Type() == sdf::SensorType::AIR_PRESSURE)
  {
-    sdf::ElementPtr airPressureElem = elem->GetElement("air_pressure");
-    airPressureElem->Copy(this->dataPtr->airPressure->ToElement());


I may have missed some in my review. If there are any more functions that accept _error, we should definitely be using them. Please feel free to create a follow up PR.

azeey · 2026-03-03T19:36:56Z

@Mergifyio backport main sdf12 sdf14 sdf15

mergify · 2026-03-03T19:37:48Z

backport main sdf12 sdf14 sdf15

✅ Backports have been created

Details

#1636 NavSat::ToElement() method (backport #1585) has been created for branch main
#1637 NavSat::ToElement() method (backport #1585) has been created for branch sdf12 but encountered conflicts

Cherry-pick of 8ccc60c has failed:

On branch mergify/bp/sdf12/pr-1585
Your branch is up to date with 'origin/sdf12'.

You are currently cherry-picking commit 8ccc60cd.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   include/sdf/Error.hh
	modified:   include/sdf/NavSat.hh
	modified:   src/Geometry.cc
	modified:   src/NavSat.cc
	modified:   src/NavSat_TEST.cc
	modified:   src/ScopedGraph.hh

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   src/Sensor.cc
	both modified:   src/Surface.cc

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

#1638 NavSat::ToElement() method (backport #1585) has been created for branch sdf14 but encountered conflicts

Cherry-pick of 8ccc60c has failed:

On branch mergify/bp/sdf14/pr-1585
Your branch is up to date with 'origin/sdf14'.

You are currently cherry-picking commit 8ccc60cd.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   include/sdf/Error.hh
	modified:   include/sdf/NavSat.hh
	modified:   src/Geometry.cc
	modified:   src/NavSat.cc
	modified:   src/NavSat_TEST.cc
	modified:   src/ScopedGraph.hh

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   src/Sensor.cc
	both modified:   src/Surface.cc

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

#1639 NavSat::ToElement() method (backport #1585) has been created for branch sdf15 but encountered conflicts

Cherry-pick of 8ccc60c has failed:

On branch mergify/bp/sdf15/pr-1585
Your branch is up to date with 'origin/sdf15'.

You are currently cherry-picking commit 8ccc60cd.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   include/sdf/Error.hh
	modified:   include/sdf/NavSat.hh
	modified:   src/Geometry.cc
	modified:   src/NavSat.cc
	modified:   src/NavSat_TEST.cc
	modified:   src/ScopedGraph.hh
	modified:   src/Sensor.cc

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   src/Surface.cc

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

`sdf::NavSat` didn't have a `ToElement()` method which prevent to convert sensor-specific information back to sdf format. This PR include such changes: * Introduce `sdf::NavSat::ToElement()` method * Added this method call to dispatcher in `Sensor::ToElement()` * Also same dispatcher had misleading error msg: if user tried to convert sensor w/o sensor-specific information it will get error message about lack of this feature, but actually it's not correct. Dispatcher logic changed to avoid such misleading (split apart type check and pointer check) * And message was w/o trailing new_line symbol which makes console output harder to read * Added test for new ToElement() method; valgrind is happy * Couple of typos fixed --------- Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit 8ccc60c)

`sdf::NavSat` didn't have a `ToElement()` method which prevent to convert sensor-specific information back to sdf format. This PR include such changes: * Introduce `sdf::NavSat::ToElement()` method * Added this method call to dispatcher in `Sensor::ToElement()` * Also same dispatcher had misleading error msg: if user tried to convert sensor w/o sensor-specific information it will get error message about lack of this feature, but actually it's not correct. Dispatcher logic changed to avoid such misleading (split apart type check and pointer check) * And message was w/o trailing new_line symbol which makes console output harder to read * Added test for new ToElement() method; valgrind is happy * Couple of typos fixed --------- Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit 8ccc60c) # Conflicts: # src/Sensor.cc # src/Surface.cc

`sdf::NavSat` didn't have a `ToElement()` method which prevent to convert sensor-specific information back to sdf format. This PR include such changes: * Introduce `sdf::NavSat::ToElement()` method * Added this method call to dispatcher in `Sensor::ToElement()` * Also same dispatcher had misleading error msg: if user tried to convert sensor w/o sensor-specific information it will get error message about lack of this feature, but actually it's not correct. Dispatcher logic changed to avoid such misleading (split apart type check and pointer check) * And message was w/o trailing new_line symbol which makes console output harder to read * Added test for new ToElement() method; valgrind is happy * Couple of typos fixed --------- Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit 8ccc60c) # Conflicts: # src/Surface.cc

`sdf::NavSat` didn't have a `ToElement()` method which prevent to convert sensor-specific information back to sdf format. This PR include such changes: * Introduce `sdf::NavSat::ToElement()` method * Added this method call to dispatcher in `Sensor::ToElement()` * Also same dispatcher had misleading error msg: if user tried to convert sensor w/o sensor-specific information it will get error message about lack of this feature, but actually it's not correct. Dispatcher logic changed to avoid such misleading (split apart type check and pointer check) * And message was w/o trailing new_line symbol which makes console output harder to read * Added test for new ToElement() method; valgrind is happy * Couple of typos fixed --------- Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com> (cherry picked from commit 8ccc60c)

NavSat ToElement() method

6d53c66

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>

ntfshard requested review from azeey and scpeters as code owners September 2, 2025 17:41

github-actions Bot added the 🪵 jetty Gazebo Jetty label Sep 2, 2025

azeey added this to Core development Sep 2, 2025

github-project-automation Bot moved this to Inbox in Core development Sep 2, 2025

ntfshard commented Sep 2, 2025

View reviewed changes

azeey reviewed Feb 18, 2026

View reviewed changes

ntfshard added 2 commits February 24, 2026 01:21

Merge remote-tracking branch 'origin/sdf16' into NavSatToElement-sdf16

b70de34

ToElement(sdf::Errors&)

0442b6e

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>

ntfshard force-pushed the NavSatToElement-sdf16 branch from 9712064 to 0442b6e Compare February 23, 2026 17:00

ntfshard requested a review from azeey February 23, 2026 17:27

ntfshard commented Feb 23, 2026

View reviewed changes

azeey reviewed Feb 23, 2026

View reviewed changes

ntfshard force-pushed the NavSatToElement-sdf16 branch from bbc87d2 to 34e0c17 Compare February 24, 2026 01:25

ntfshard commented Feb 24, 2026

View reviewed changes

ntfshard force-pushed the NavSatToElement-sdf16 branch from 34e0c17 to 9da3507 Compare February 24, 2026 01:45

Better error handling

3371f31

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>

ntfshard force-pushed the NavSatToElement-sdf16 branch from 9da3507 to 3371f31 Compare February 24, 2026 01:53

ntfshard requested a review from azeey February 24, 2026 16:17

Merge branch 'sdf16' into NavSatToElement-sdf16

c040044

Signed-off-by: Maksim Derbasov <ntfs.hard@gmail.com>

azeey approved these changes Mar 3, 2026

View reviewed changes

github-project-automation Bot moved this from Inbox to In review in Core development Mar 3, 2026

azeey merged commit 8ccc60c into gazebosim:sdf16 Mar 3, 2026
14 checks passed

github-project-automation Bot moved this from In review to Done in Core development Mar 3, 2026

mergify Bot mentioned this pull request Mar 3, 2026

NavSat::ToElement() method (backport #1585) #1636

Merged

9 tasks

mergify Bot mentioned this pull request Mar 3, 2026

NavSat::ToElement() method (backport #1585) #1637

Open

9 tasks

mergify Bot mentioned this pull request Mar 3, 2026

NavSat::ToElement() method (backport #1585) #1638

Open

9 tasks

mergify Bot mentioned this pull request Mar 3, 2026

NavSat::ToElement() method (backport #1585) #1639

Merged

9 tasks

ntfshard deleted the NavSatToElement-sdf16 branch March 15, 2026 10:31

		/// Note that parameter passing functionality is not captured with this
		/// function.

	sdf::initFile("navsat.sdf", elem);
	sdf::initFile("navsat.sdf", ParserConfig::GlobalConfig(), elem, _errors);

	el->Copy(this->dataPtr->horizontalPositionNoise.ToElement(), _errors);
	el->Copy(this->dataPtr->horizontalPositionNoise.ToElement(_errors), _errors);

	el->Copy(this->dataPtr->verticalPositionNoise.ToElement(), _errors);
	el->Copy(this->dataPtr->verticalPositionNoise.ToElement(_errors), _errors);

	el->Copy(this->dataPtr->horizontalVelocityNoise.ToElement(), _errors);
	el->Copy(this->dataPtr->horizontalVelocityNoise.ToElement(_errors), _errors);

	el->Copy(this->dataPtr->verticalVelocityNoise.ToElement(), _errors);
	el->Copy(this->dataPtr->verticalVelocityNoise.ToElement(_errors), _errors);

Conversation

ntfshard commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦟 Bug fix

Summary

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azeey commented Sep 2, 2025

Uh oh!

ntfshard commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntfshard commented Dec 8, 2025

Uh oh!

ntfshard commented Feb 17, 2026

Uh oh!

azeey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntfshard Feb 24, 2026 • 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!

ntfshard commented Mar 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azeey commented Mar 3, 2026

Uh oh!

mergify Bot commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ntfshard commented Sep 2, 2025 •

edited

Loading

ntfshard commented Sep 3, 2025 •

edited

Loading

ntfshard Feb 24, 2026 •

edited

Loading

mergify Bot commented Mar 3, 2026 •

edited

Loading