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.

Sign up for GitHub

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

Jump to bottom

Schema accessor #1393

Open
wants to merge 23 commits into
base: sdf14
Choose a base branch
from
Open

Schema accessor #1393

wants to merge 23 commits into from

Conversation

aagrawal05
Copy link

@aagrawal05 aagrawal05 commented Apr 5, 2024 •
edited
Loading

🎉 New feature

Closes #1376

Summary

  1. I've created a script format_sdf.py which reads from the /sdf/1.11 folder and generates/updates the source for all the relevant classes in the src/ and include/ directories as per the suggested implementation.
  2. There were DOM elements navsat.sdf and forcetorque.sdf for which the script does not work and I had to manually edit the script because their naming convention did not match the camel-case conversion (they would need to be nav_sat.sdf and force_torque.sdf respectively to function correctly with the script) This may be undesirable from a code style perspective but may be a breaking change—this is a minor point but I just wanted to point it out.
  3. The code builds successfully, I have not thoroughly tested it but it does seem to work.
  4. I can add tests for the SchemaFile in all the DOM element tests but I thought it might be overkill because the names are hardcoded in the method anyways. I can add it easily with the script if needed.

Test it

Tests can be done manually as mentioned by the OP in #1376 in his code example.
I could also add tests in the DOM element unit tests, but that might be overkill because the values are hard-coded.

Checklist

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

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

@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Apr 5, 2024
azeey
azeey requested changes Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I have a few comments that should be easy to address.

Regarding testing, I don't think we should create new tests for this, but one way we can make sure it's working is by using SchemaFile() instead of hard coded file names in ToElement functions. Perhaps you can update your script to make that change as well?

.DS_Store Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this file

src/Actor.cc Outdated
{
static const std::string* kSchemaFile = new std::string("actor.sdf");
return *kSchemaFile;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a newline after the }? (see https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline)

src/Actor.cc Outdated
Comment on lines 824 to 828
const std::string& Actor::SchemaFile()
{
static const std::string* kSchemaFile = new std::string("actor.sdf");
return *kSchemaFile;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to our style guide, this is sort of a last resort way to create variables with static storage. After reading through https://abseil.io/tips/140#string-view-mistake, I think the recommended way is:

Suggested change
const std::string& Actor::SchemaFile()
{
static const std::string* kSchemaFile = new std::string("actor.sdf");
return *kSchemaFile;
}
inline std::string_view Actor::SchemaFile()
{
static const char kSchemaFile[] = "actor.sdf";
return kSchemaFile;
}

@aagrawal05 aagrawal05 requested a review from azeey May 19, 2024 15:00
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@azeey
Copy link
Collaborator

azeey commented Aug 6, 2024

@aagrawal05 Looks like there are some style issues and a DCO error. Mind fixing them?

@aagrawal05
Copy link
Author

@aagrawal05 Looks like there are some style issues and a DCO error. Mind fixing them?

Hey @azeey sure let me take a look. The style issues look fairly straightforward and I'll shortly submit changes to fix this. Please let me know if anything further is to be done in terms of functionality.

@aagrawal05 aagrawal05 force-pushed the schema_accessor branch 2 times, most recently from 6817d40 to 2215b16 Compare August 16, 2024 03:32
aagrawal05 and others added 19 commits August 16, 2024 11:55
@aagrawal05
added changes
310b6fd 
Signed-off-by: Aditya Agrawal <[email protected]>
@aagrawal05
string to string_view, refactoring
f8e22dc 
Signed-off-by: Aditya Agrawal <[email protected]>
@scpeters @aagrawal05
Param_TEST: Check return values of Param::Get/Set (gazebosim#1394)
339a4dc 
Signed-off-by: Steve Peters <[email protected]>
@iche033 @aagrawal05
backport - Add mesh optimization attribute to <mesh> gazebosim#1382
ceee6ff 
Signed-off-by: Ian Chen <[email protected]>
@iche033 @aagrawal05
backport - Update max_convex_hulls description gazebosim#1386
2028a96 
Signed-off-by: Ian Chen <[email protected]>
@ahcorde @iche033 @aagrawal05
Added Python wrapper to ConvexDecomposition (gazebosim#1398)
2276134 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
@azeey @aagrawal05
Add package.xml, fix gz sdf tests on Windows (gazebosim#1374)
5590a14 
The `gz sdf` tests are fixed by 
* fixing dll path issue with Ruby on windows
* Setting home path

---------

Signed-off-by: Addisu Z. Taddese <[email protected]>
@j-rivero @scpeters @aagrawal05
Fix trivial warning on 24.04 for JointAxis_TEST.cc (gazebosim#1402)
67bf992 
* Fix trivial warning on 24.04 for JointAxis_TEST.cc

---------

Signed-off-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
@iche033 @aagrawal05
Prepare for 14.2.0 release (gazebosim#1405)
8631b3f 
Signed-off-by: Ian Chen <[email protected]>
@Blast545 @scpeters @aagrawal05
(Backport) Enable 24.04 CI, remove distutils dependency (gazebosim#1413)
74aca7e 
* Enable 24.04 CI, remove distutils dependency (gazebosim#1408)
  distutils is no longer required since this branch requires a
 new enough version of cmake.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Jorge Perez <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
@scpeters @aagrawal05
Allow empty strings in plugin and custom attributes (gazebosim#1407)
0a61cc1 
Currently errors are generated when adding an attribute
containing an empty string to a <plugin> block or as a
custom attribute. This adds failing tests to confirm the bug
and fixes the errors in by setting `_required == 0` when
calling Element::AddAttribute. This also changes
Element::ToString to print empty custom attributes.

Fixes gazebosim#725.

Signed-off-by: Steve Peters <[email protected]>
@shameekganguly @aagrawal05
Update default camera instrinsics skew to 0, which matches spec (gaze…
186f7b1 
…bosim#1423)

Signed-off-by: Shameek Ganguly <[email protected]>
@iche033 @aagrawal05
Add note in migration guide on camera skew value change (gazebosim#1425)
2489af1 
Signed-off-by: Ian Chen <[email protected]>
@ahcorde @aagrawal05
Added Automatic Moment of Inertia Calculations for Basic Shapes Pytho… (
0127bb8 
gazebosim#1424)

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@iche033 @aagrawal05
Add bullet and torsional friction DOM (gazebosim#1351)
faf94a0 
Signed-off-by: Ian Chen <[email protected]>
@iche033 @aagrawal05
Add python bindings for bullet and torsional friction (gazebosim#1427)
6d4141e 
Signed-off-by: Ian Chen <[email protected]>
@iche033 @aagrawal05
Backport voxel_resolution sdf element
8c0b857 
Signed-off-by: Ian Chen <[email protected]>
@iche033 @aagrawal05
Prepare for 12.8.0 release (gazebosim#1430) (gazebosim#1432)
1286675 
Signed-off-by: Ian Chen <[email protected]>
@iche033 @aagrawal05
Prepare for 14.3.0 release (gazebosim#1437)
43ba646 
Signed-off-by: Ian Chen <[email protected]>
scpeters and others added 4 commits August 16, 2024 11:57
@scpeters @aagrawal05
Add custom attribute to custom element in test (gazebosim#1406)
f0dfc5f 
Update test to confirm that gazebosim#54 is fixed.

Signed-off-by: Steve Peters <[email protected]>
@bperseghetti @scpeters @aagrawal05
Add Cone as a primitive parametric shape. (gazebosim#1415)
c3e5690 
* Backport:  Add cone shape to SDFormat spec (gazebosim#1418)

Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
@j-rivero @scpeters @aagrawal05
Prepare release 14.4.0 (gazebosim#1441)
2a66550 

Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
@aagrawal05
Style fixes
e8e45a8 
Signed-off-by: Aditya Agrawal <[email protected]>
@aagrawal05
Copy link
Author

Hey @azeey in attempting to resolve the DCO error through a rebase, it appears that I have accidentally re-committed all the 23 commits from my first changes before the merge to my latest changes...

I believe it may be a bit complicated for me to proceed accurately to revert this from this point. I feel it might be easier to just close PR and create a new PR on a new branch with a single commit (this also has the added benefit of being merged with the latest changes in sdf14).

With regards to the style I believe the errors should be resolved but I need to test with the linter using the workflow.

Please let me know how to proceed.

@azeey azeey removed the beta Targeting beta release of upcoming collection label Sep 26, 2024
@azeey
Copy link
Collaborator

azeey commented Dec 19, 2024

@aagrawal05 yes, I think I new PR would be cleaner. It would be ideal if you can target sdf15 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters Awaiting requested review from scpeters scpeters is a code owner

@azeey azeey Awaiting requested review from azeey azeey is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
🎵 harmonic Gazebo Harmonic
Projects
Core development
Status: In review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add schema file name accessor to each DOM type C++ class
9 participants
@aagrawal05 @azeey @scpeters @iche033 @ahcorde @j-rivero @Blast545 @shameekganguly @bperseghetti