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

Merge sdf13 ➡️ main #1297

Merged
merged 72 commits into from
Jul 26, 2023
Merged

Merge sdf13 ➡️ main #1297

merged 72 commits into from
Jul 26, 2023

Conversation

quarkytale
Copy link
Contributor

@quarkytale quarkytale commented Jun 29, 2023

➡️ Forward port

Port sdf13 to main

Branch comparison: main...sdf13

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

Need help with resolving sdf/embedSdf.py, seems like the file was first added to main and backported to sdf13, but still there are some changes in functions, see

CC: @jasmeet0915

voxik and others added 30 commits January 25, 2023 20:24
`File.exists?` has been deprecated and is now removed in Ruby 3.2. 

https://github.com/ruby/ruby/blob/a528908271c678360d2d8ca232c178e7cdd340b4/NEWS.md?plain=1#L58

Signed-off-by: Vít Ondruch <vondruch@redhat.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Version v2 of the actions/checkout workflow is
deprecated, so switch to v3.

Part of gazebo-tooling/release-tools#862.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
* 12.7.0~pre1

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Update changelog

Signed-off-by: Nate Koenig <nate@openrobotics.org>

---------

Signed-off-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Nate Koenig <nate@openrobotics.org>
When parsing a URDF file, the `_source` parameter is set to "urdf file" which breaks relative path resolution downstream. This commit changes the call to `sdf::readDoc` to use `filename` as the `_source` parameter instead.

Signed-off-by: Robert Plante <RobertDPlante@gmail.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Nate Koenig <nate@openrobotics.org>
It seems quite hard to guarantee the same flow of the execution of SDF_ASSERT when using a FATAL_ERROR instead. Because of this, this PR reverts all previous SDF_ASSERT changes into FATAL_ERROR


Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Co-authored-by: Bi0T1N <Bi0T1N@users.noreply.github.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Implements merge-includes for worlds as discussed in gazebosim/sdf_tutorials#86. 

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
Version v2 of the actions/checkout workflow is deprecated, so switch to v3.

Part of gazebo-tooling/release-tools#862.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
The LICENSE file contained a copy of the stanze
used at the top of source code files, while the
actual license was in the COPYING file. So remove
the stanza and put the actual Apache 2.0 license text
in LICENSE.

Similar to gazebosim/gz-math#521.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: tejalbarnwal <tejalbarnwal@gmail.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
This is an update for the class `Element` making the calls use sdf::Errors whenever possible to avoid unwanted console outputs.

Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
* Adding tests that catch warnings when urdf have no inertia element, or if mass is too small

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

* Promote link inertia and mass related urdf2sdf sdfdbg to sdfwarn with more verbose messages

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

* Added flag for converting urdf links with small or no mass to frames

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

* Converts urdf links with small or no mass to frames, added tests

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

* Adding warning when conversion happens, added tests for small masses, being explicit about epsilon in equal

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

* Fix cpplint errors

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

* Adding integration test

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

* Fix lint

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

* Use camelCase

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

* Added URDFMinimumAllowedLinkMass

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

* Fix tests expecting warnings when converting to frames

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

* Adding inline contains and notContains to test_utils

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

* Using RedirectConsoleStream and ScopeExit

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

* Refactor to use Root::LoadSdfString

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

* Removing debug message when converted frame is from a root link

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

* Added attached_to for frames during conversion, using < instead of math::equals

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

* Update brief of URDFSetConvertLinkWithNoMassToFrame to mention case of no inertial frame

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

* Remove stale commentted code

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

* Update comment about the errors we are expecting

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

* Convert to frame by default, remove minimal mass option, refactor implementation to handle lumping, modified unit tests

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

* Only convert to frame when parent joint is fixed, attaches and transforms pose to parent link, leaves out the fixed joint

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

* Rephrased conversion error, modified unit tests

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

* prints zero mass errors as well when conversion to frame fails

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

* integration tests that mimic the unit tests

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

* Added integration test with valid and invalid use of force torque sensor where massless child links occur

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

* Fix lint

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

* Fix joint reduction logic, more specific error messages, more targeted unit tests for each case

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

* Convert joints to frames when converting links, attach them to converted links

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

* Integration tests with child fixed links as well

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

* Change sdferr to sdfwarn, no way to use ParserConfig::WarningsPolicy for now

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

* sdferr to sdfwarn for the case where conversion to frame is attempted

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

* Removing case within converting to frame, where parent joint turns into revolute joint, that is not covered

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

* Reduced scope of implementation, more specific warning messages

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

* Remove mention of parser config in warning, since that is not typically used by workflows

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

* Integration tests revisited and modified, removed printouts

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

* Test case showing ParserConfig::URDFSetPreserveFixedJoint(true) overrides gazebo tags for joint lumping, warnings with more information and placeholder url

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

* Use sdf::testing::contains instead of local contains functions in testing (#1251)

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

* Remove unused InitSDF, added TODO for warning when joints are converted/dropped

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

* Cleaned up if else cases

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

* Using << operator of Errors

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

* Replacing placeholder url with expected URL for the documentation from sdf_tutorials#88

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

* URL to tutorial as a constant, removing checking URL from test, just in case links change, we are not testing for that anyway

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

* Refactored fixed child joint logic as it is never reached

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

* Reduce number of if statements, renaming to only check if parent joint is reduced

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

---------

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@scpeters
Copy link
Member

need to update the target branch to main

@quarkytale quarkytale changed the base branch from sdf13 to main June 29, 2023 18:11
@scpeters
Copy link
Member

there are linking errors in CI

WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}/sdf"
OUTPUT_FILE "${PROJECT_BINARY_DIR}/src/EmbeddedSdf.cc"
)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

these RUBY changes should not be merged forward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried it out, please check if it makes sense

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #1297 (390834f) into main (446d691) will increase coverage by 0.39%.
The diff coverage is 94.80%.

❗ Current head 390834f differs from pull request most recent head 91b7265. Consider uploading reports for the commit 91b7265 to get more accurate results

@@            Coverage Diff             @@
##             main    #1297      +/-   ##
==========================================
+ Coverage   87.21%   87.60%   +0.39%     
==========================================
  Files         126      128       +2     
  Lines       16304    16830     +526     
==========================================
+ Hits        14219    14744     +525     
- Misses       2085     2086       +1     
Impacted Files Coverage Δ
include/sdf/Plugin.hh 93.75% <ø> (ø)
python/src/sdf/pyProjector.cc 52.72% <52.72%> (ø)
python/src/sdf/pyLink.cc 58.25% <57.14%> (-0.18%) ⬇️
src/parser.cc 86.80% <83.72%> (-0.19%) ⬇️
src/Element.cc 96.21% <89.24%> (+0.39%) ⬆️
src/Collision.cc 97.43% <92.30%> (+0.17%) ⬆️
include/sdf/Element.hh 97.67% <95.23%> (-2.33%) ⬇️
src/Material.cc 95.76% <95.34%> (+0.39%) ⬆️
src/Projector.cc 95.68% <95.68%> (ø)
src/parser_urdf.cc 88.14% <96.36%> (+0.34%) ⬆️
... and 30 more

... and 3 files with indirect coverage changes

@scpeters
Copy link
Member

scpeters commented Jul 1, 2023

do you mind squashing 6a8b3d1 and b41123b together?

@quarkytale
Copy link
Contributor Author

Squashed them to the main merge commit only

@quarkytale
Copy link
Contributor Author

DCO is failing for older commits, is it okay to set it pass manually or should I add my signature on previous commits?

@scpeters
Copy link
Member

scpeters commented Jul 5, 2023

DCO is failing for older commits, is it okay to set it pass manually or should I add my signature on previous commits?

I reviewed the DCO issues and set it to pass manually because it was an issue with email matching

@@ -20,12 +20,13 @@ endif()
# descriptions in a map of strings. The parser.cc file uses EmbeddedSdf.hh.
execute_process(
COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/sdf/embedSdf.py
--output-file "${PROJECT_BINARY_DIR}/src/EmbeddedSdf.cc"
WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}/sdf"
OUTPUT_FILE "${PROJECT_BINARY_DIR}/src/EmbeddedSdf.cc"
Copy link
Member

Choose a reason for hiding this comment

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

I think this OUTPUT_FILE line needs to be removed, since we are passing --output-file. I think this is causing the windows build failure

Copy link
Member

Choose a reason for hiding this comment

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

I think you removed the wrong line in your force-push to 5fbe349 (comparison https://github.com/gazebosim/sdformat/compare/25d793e8149cffb8d5c05b526a31fa030943e952..5fbe349793af470b359972c2d0e4e9be5435224d)

I think the --output-file "${PROJECT_BINARY_DIR}/src/EmbeddedSdf.cc" line should remain but the OUTPUT_FILE "${PROJECT_BINARY_DIR}/src/EmbeddedSdf.cc" line should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, should be correct now

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that line looks good now!

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@scpeters
Copy link
Member

There is still something wrong in the Windows build with lots of tests failing with an error message about converting between versions:

Unable to convert from SDF version 1.6 to 1.10
SDF does not have a version.

This seems related to the embed script behavior that was changed in #1240. I also see some compiler warnings from the generated EmbeddedSdf.cc file:

C:\J\workspace\sdformat-pr-win\ws\build\sdformat14\src\EmbeddedSdf.cc(287,1): warning C4129: 'c': unrecognized character escape sequence [C:\J\workspace\sdformat-pr-win\ws\build\sdformat14\src\sdformat14.vcxproj]

C:\J\workspace\sdformat-pr-win\ws\build\sdformat14\src\EmbeddedSdf.cc(523,1): warning C4129: 'c': unrecognized character escape sequence [C:\J\workspace\sdformat-pr-win\ws\build\sdformat14\src\sdformat14.vcxproj]
...

I copied this file from the Jenkins file and see the following on the mentioned lines:

"1.10\camera.sdf",

and

"1.10\capsule_shape.sdf",

This looks like a problem with the path separator. Glancing through the changes in #1240, I see that the get_map_content method used PurePosixPath to force use of / path separators, but that line is not present in the generate_map_content method. I think we didn't notice it in #1240 because that script isn't used in Windows CI on the sdf13 branch

@quarkytale
Copy link
Contributor Author

Thanks for taking a look! Should I work on adding/reverting that PR or needs to be discussed during the weekly?

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

Thanks for taking a look! Should I work on adding/reverting that PR or needs to be discussed during the weekly?

I've attempted a fix in 91b7265

@scpeters
Copy link
Member

Thanks for taking a look! Should I work on adding/reverting that PR or needs to be discussed during the weekly?

I've attempted a fix in 91b7265

seems to be working. I'll defer to @azeey or @mjcarroll for a review on 91b7265

@azeey
Copy link
Collaborator

azeey commented Jul 25, 2023

Thanks for taking a look! Should I work on adding/reverting that PR or needs to be discussed during the weekly?

I've attempted a fix in 91b7265

seems to be working. I'll defer to @azeey or @mjcarroll for a review on 91b7265

Looks good to me. @mjcarroll was there a reason for removing the PurePosixPath in #1240?

@mjcarroll
Copy link
Contributor

@mjcarroll was there a reason for removing the PurePosixPath in #1240?

Not for any reason that I recall at this point.

@azeey azeey merged commit 618286b into main Jul 26, 2023
13 checks passed
@azeey azeey deleted the quarkytale/13_to_main branch July 26, 2023 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet