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

Added Friction and ODE classes #955

Merged
merged 5 commits into from
Apr 14, 2022
Merged

Added Friction and ODE classes #955

merged 5 commits into from
Apr 14, 2022

Conversation

ahcorde
Copy link
Collaborator

@ahcorde ahcorde commented Apr 7, 2022

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

related with this issue https://github.com/ignitionrobotics/sdformat/issues/954

Summary

Added Friction and ODE classes

Test it

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Apr 7, 2022
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Apr 7, 2022
@chapulina chapulina added the usd label Apr 7, 2022
@scpeters
Copy link
Member

scpeters commented Apr 8, 2022

@azeey this might be the right time to try to clean up our friction parameters, per #31 and specifically this comment: #31 (comment)

@azeey
Copy link
Collaborator

azeey commented Apr 8, 2022

@azeey this might be the right time to try to clean up our friction parameters, per #31 and specifically this comment: #31 (comment)

I think @ahcorde is pressed for time and needs this for the USD-SDF work. But yeah, we might be able to make time for it in the next couple months.

@scpeters
Copy link
Member

scpeters commented Apr 8, 2022

@azeey this might be the right time to try to clean up our friction parameters, per #31 and specifically this comment: #31 (comment)

I think @ahcorde is pressed for time and needs this for the USD-SDF work. But yeah, we might be able to make time for it in the next couple months.

oh I didn't look at the labels and thought this was for mujoco. Since this PR is creating an API, I'd like to think a little extra about its design

@scpeters
Copy link
Member

scpeters commented Apr 8, 2022

oh I didn't look at the labels and thought this was for mujoco. Since this PR is creating an API, I'd like to think a little extra about its design

my main concern is that the fact that all friction parameters are in the <ode> block feels like a hack. We started trying to clean this up with the FrictionPyramid class in gazebo classic but never pushed that model structure into SDFormat.

so, do we want to encode the hacky SDFormat structure in our C++ API in libsdformat12 and then deprecate it in libsdformat13? The advantage of that would be that we could get it merged faster, and that it maintains a transparent mapping between the DOM API and the SDFormat data structure. Or do we want to try for a better API now?

@scpeters
Copy link
Member

scpeters commented Apr 9, 2022

oh I didn't look at the labels and thought this was for mujoco. Since this PR is creating an API, I'd like to think a little extra about its design

my main concern is that the fact that all friction parameters are in the <ode> block feels like a hack. We started trying to clean this up with the FrictionPyramid class in gazebo classic but never pushed that model structure into SDFormat.

so, do we want to encode the hacky SDFormat structure in our C++ API in libsdformat12 and then deprecate it in libsdformat13? The advantage of that would be that we could get it merged faster, and that it maintains a transparent mapping between the DOM API and the SDFormat data structure. Or do we want to try for a better API now?

after discussing offline, it sounds reasonable to proceed with this approach for libsdformat12 since it matches the specification and deprecate it in libsdformat13 if we change how the friction parameters are specified per #31

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2022

Codecov Report

Merging #955 (80117d1) into sdf12 (0fa9e78) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            sdf12     gazebosim/sdformat#955   +/-   ##
=======================================
  Coverage   65.38%   65.38%           
=======================================
  Files           2        2           
  Lines          26       26           
=======================================
  Hits           17       17           
  Misses          9        9           

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 0fa9e78...80117d1. Read the comment docs.

@ahcorde ahcorde mentioned this pull request Apr 13, 2022
8 tasks
include/sdf/Surface.hh Outdated Show resolved Hide resolved
include/sdf/Surface.hh Outdated Show resolved Hide resolved
include/sdf/Surface.hh Outdated Show resolved Hide resolved
include/sdf/Surface.hh Outdated Show resolved Hide resolved
include/sdf/Surface.hh Outdated Show resolved Hide resolved
include/sdf/Surface.hh Outdated Show resolved Hide resolved
src/Surface.cc Outdated Show resolved Hide resolved
src/Surface.cc Outdated
errors.insert(errors.end(), err.begin(), err.end());
}

// \todo(nkoenig) Parse the remaining collide properties.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this TODO needed? If so, can you update it?

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 update the username?

src/Surface_TEST.cc Outdated Show resolved Hide resolved
src/Surface_TEST.cc Show resolved Hide resolved
@ahcorde ahcorde requested a review from azeey April 13, 2022 17:19
src/Surface.cc Outdated
errors.insert(errors.end(), err.begin(), err.end());
}

// \todo(nkoenig) Parse the remaining collide properties.
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 update the username?

Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
@ahcorde ahcorde merged commit a759c9a into sdf12 Apr 14, 2022
@ahcorde ahcorde deleted the ahcorde/friction_ode branch April 14, 2022 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress usd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants