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

[TPE] Update link pose #179

Merged
merged 17 commits into from
Apr 5, 2021
Merged

[TPE] Update link pose #179

merged 17 commits into from
Apr 5, 2021

Conversation

claireyywang
Copy link
Contributor

Support updating individual link pose, related to gazebosim/gz-sim#427

Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang claireyywang added the 🏰 citadel Ignition Citadel label Dec 17, 2020
@claireyywang claireyywang self-assigned this Dec 17, 2020
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang claireyywang marked this pull request as ready for review December 18, 2020 02:04
@claireyywang claireyywang requested a review from mxgrey as a code owner December 18, 2020 02:04
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good. Can you add some unit tests for the new functions in Link?

@@ -42,3 +42,44 @@ Entity &Link::AddCollision()
this->ChildrenChanged();
return *it->second.get();
}

//////////////////////////////////////////////////
void Link::SetLinearVelocity(const math::Vector3d _velocity)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's pass by reference. I noticed that it's the same in Model.hh but having const & is more efficient.

Same for SetAngularVelocity

Copy link
Contributor

Choose a reason for hiding this comment

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

Weirdly, I've read that it isn't always the case that a const reference is more efficient for small POD statically sized data structures because of cache behaviors. Copying the three double values in the Vector3d into the stack frame may have less overhead than repeatedly dereferencing the const Vector3d& which is allocated elsewhere, perhaps in the heap.

That's not say we shouldn't use const-reference here. In the absence of profiling, it's fair to assume that const T& will be more efficient than const T for non-primitive types. I just thought it's interesting that the common wisdom doesn't always hold.

Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang claireyywang requested a review from iche033 December 22, 2020 07:07
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
Copy link
Contributor Author

@iche033 I think this one is ready for another look as well :D

@luca-della-vedova
Copy link
Member

luca-della-vedova commented Dec 24, 2020

We tried the PR with TPE and thought I'd check with you how you think we should model transformation when acting on links and what frame or reference commands should use.
We tried a model with two links and noticed that when we apply a link velocity command the behavior changes depending on the link it is applied upon. Specifically in the example below we are applying a constant velocity and when clicking the button we apply an angular velocity (to simulate opening the door).

link_vel_cmds_2.mp4

When applying the angular velocity on one of the links the model coordinates are also rotated, hence the two links start moving in the opposite direction (since they both rotate by 90 degrees in opposite directions).
I am not sure what behavior would make more sense for link level commands, if the current one is OK (and we should add a "dummy" link that represents a static dummy base) or if we should change it so that any Link level command has no effect on the model coordinates itself.
What do you think?

@chapulina chapulina assigned iche033 and unassigned claireyywang Feb 19, 2021
@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #179 (2026038) into ign-physics2 (6d40353) will decrease coverage by 0.19%.
The diff coverage is 75.00%.

❗ Current head 2026038 differs from pull request most recent head 1ee1e67. Consider uploading reports for the commit 1ee1e67 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           ign-physics2     #179      +/-   ##
================================================
- Coverage         83.14%   82.94%   -0.20%     
================================================
  Files               106      106              
  Lines              3998     4034      +36     
================================================
+ Hits               3324     3346      +22     
- Misses              674      688      +14     
Impacted Files Coverage Δ
tpe/lib/src/Entity.hh 0.00% <ø> (ø)
tpe/lib/src/Link.hh 100.00% <ø> (ø)
tpe/plugin/src/FreeGroupFeatures.cc 67.10% <0.00%> (-12.59%) ⬇️
tpe/plugin/src/SDFFeatures.cc 83.33% <83.33%> (-1.13%) ⬇️
tpe/lib/src/Link.cc 100.00% <100.00%> (ø)
tpe/lib/src/Model.cc 98.38% <100.00%> (+0.02%) ⬆️
tpe/lib/src/World.cc 100.00% <100.00%> (ø)

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 6d40353...1ee1e67. Read the comment docs.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Copy link
Contributor

iche033 commented Feb 19, 2021

fixed velocity cmd frame of reference in ba45184

@iche033
Copy link
Contributor

iche033 commented Feb 19, 2021

We tried a model with two links and noticed that when we apply a link velocity command the behavior changes depending on the link it is applied upon.

Gazebo has a concept of a canonical link which is always fixed to the model. It's usually the first link in the model if not specified through SDF. So I think what's happening is that in your example, one of the links is the canonical link and setting vel of that link may have caused the pose changes to affect the whole model. I think the workaround is to create a base link that is always fixed (e.g. door frame?), and use the velocity control system on two other door links.

As for the reference frame, I've made some fixes in ba45184, the velocity should now be in link frame (it was in world frame before).

@luca-della-vedova Can you give gazebosim/gz-sim#427 and this PR a try and see if it'll produce the correct behavior now?

@iche033 iche033 requested a review from chapulina February 19, 2021 23:22
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
tpe/lib/src/Link.hh Outdated Show resolved Hide resolved
tpe/lib/src/Link_TEST.cc Show resolved Hide resolved
tpe/lib/src/Link.cc Outdated Show resolved Hide resolved
tpe/lib/src/Link.hh Show resolved Hide resolved
iche033 added 4 commits March 16, 2021 18:27
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@chapulina chapulina merged commit 8173772 into ign-physics2 Apr 5, 2021
@chapulina chapulina deleted the claire/link-velocity branch April 5, 2021 22:07
Lobotuerk pushed a commit that referenced this pull request May 19, 2021
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants