Adding HaltMotion (Or new suggested name) to physics plugin #728

Lobotuerk · 2021-03-30T19:55:26Z

Signed-off-by: Tomas Lorente jtlorente@ekumenlabs.com

🎉 New feature

Summary

Adds the functionality to stop a model's joint's movement in other cases than the battery being 0. By adding a HaltMotion component to the model, its joints movement can be turned on/off. Also added some QoL to kineticEnergyMonitor plugin with profilers and some checking.

(Updates)
19acc8d ~~adds the capability to turn movement halting off if you only need the advice (and making it backward compatible).~~
4a9f551 Moved everything into GameLogicPlugin, just left here the halt component evaluation in physics and the QoL changes in kineticEnergyMonitor

Test it

TO-DO tested it with SubT worlds for now

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

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

src/systems/kinetic_energy_monitor/KineticEnergyMonitor.hh

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

codecov · 2021-03-31T21:12:30Z

Codecov Report

Merging #728 (4a9f551) into ign-gazebo4 (302f5ed) will decrease coverage by 10.20%.
The diff coverage is 68.63%.

❗ Current head 4a9f551 differs from pull request most recent head 6b19cd7. Consider uploading reports for the commit 6b19cd7 to get more accurate results

@@               Coverage Diff                @@
##           ign-gazebo4     #728       +/-   ##
================================================
- Coverage        77.37%   67.16%   -10.21%     
================================================
  Files              217      241       +24     
  Lines            12217    18893     +6676     
================================================
+ Hits              9453    12690     +3237     
- Misses            2764     6203     +3439

Impacted Files	Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh	`100.00% <ø> (ø)`
include/ignition/gazebo/rendering/SceneManager.hh	`100.00% <ø> (ø)`
src/Conversions.cc	`81.56% <ø> (-0.33%)`	⬇️
.../plugins/component_inspector/ComponentInspector.cc	`7.29% <0.00%> (-1.32%)`	⬇️
.../plugins/component_inspector/ComponentInspector.hh	`28.57% <ø> (ø)`
src/network/PeerTracker.hh	`100.00% <ø> (ø)`
src/systems/apply_joint_force/ApplyJointForce.hh	`100.00% <ø> (ø)`
src/systems/breadcrumbs/Breadcrumbs.hh	`100.00% <ø> (ø)`
src/systems/buoyancy/Buoyancy.hh	`100.00% <ø> (ø)`
src/systems/contact/Contact.hh	`100.00% <ø> (ø)`
... and 98 more

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 86fe608...6b19cd7. Read the comment docs.

azeey

IMO, the kinetic energy monitor should not be concerned with halting a vehicle. If we adhere to the separation of concerns principle, I would think another System in SubT (eg. GameLogicPlugin) would monitor the output of the kinetic energy monitor and set the state of the HaltMotion component.

I also left some comments in case we decide to proceed with keeping that logic in the kinetic energy monitor.

include/ignition/gazebo/components/HaltMotion.hh

src/systems/kinetic_energy_monitor/KineticEnergyMonitor.hh

src/systems/kinetic_energy_monitor/KineticEnergyMonitor.cc

Lobotuerk · 2021-04-08T15:38:47Z

IMO, the kinetic energy monitor should not be concerned with halting a vehicle. If we adhere to the separation of concerns principle, I would think another System in SubT (eg. GameLogicPlugin) would monitor the output of the kinetic energy monitor and set the state of the HaltMotion component.

I do agree with moving the HaltMotion logic into the GameLogicPlugin, but that would require adding A PreUpdate there. @nkoenig Do you have a strong feeling for either of them?

nkoenig · 2021-04-09T16:28:54Z

IMO, the kinetic energy monitor should not be concerned with halting a vehicle. If we adhere to the separation of concerns principle, I would think another System in SubT (eg. GameLogicPlugin) would monitor the output of the kinetic energy monitor and set the state of the HaltMotion component.

I also left some comments in case we decide to proceed with keeping that logic in the kinetic energy monitor.

Agreed. @Lobotuerk , can you remove the logic for halting motion from the Kinetic Energy Monitor and place it in the SubT Game Logic Plugin?

nkoenig · 2021-04-09T16:29:44Z

I do agree with moving the HaltMotion logic into the GameLogicPlugin, but that would require adding A PreUpdate there. @nkoenig Do you have a strong feeling for either of them?

Adding a PreUpdate to the GameLogicPlugin sounds good.

Lobotuerk · 2021-04-20T18:28:57Z

@azeey When moving the halting logic to GameLogic, should I leave the HaltMotion component creation inside KineticEnergyMonitor?

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

azeey · 2021-04-21T16:12:09Z

@azeey When moving the halting logic to GameLogic, should I leave the HaltMotion component creation inside KineticEnergyMonitor?

IMO, We don't need to modify KineticEnergyMonitor at all. All the logic can be in GameLogic and Physics. And I think the HaltMotion component creation can go into GameLogic

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

azeey

LGTM! It would be great if you can add a test showing that a vehicle stops once the HaltMotion component is set.

chapulina

I'm wondering if this functionality couldn't be achieved using components::JointVelocityReset / components::JointPositionReset?

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

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

Lobotuerk · 2021-05-18T15:45:46Z

I'm wondering if this functionality couldn't be achieved using components::JointVelocityReset / components::JointPositionReset?

It's better for a long time halt to simply stop the physics layer before applying forces, than trying to reset the joint every single timestep

tested halt motion feature

9f0c918

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

Lobotuerk requested review from azeey and chapulina as code owners March 30, 2021 19:55

github-actions bot added the 🔮 dome Ignition Dome label Mar 30, 2021

gonzodepedro reviewed Mar 31, 2021

View reviewed changes

src/systems/kinetic_energy_monitor/KineticEnergyMonitor.hh Show resolved Hide resolved

Lobotuerk mentioned this pull request Mar 31, 2021

KineticEnergy and HaltMotion added to models osrf/subt#858

Closed

added flag to turn off movement halting

19acc8d

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

azeey reviewed Apr 8, 2021

View reviewed changes

Lobotuerk added 2 commits April 21, 2021 12:39

Moved logic into gamelogic

67c262b

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

docs

f5c472e

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

Lobotuerk requested review from azeey and gonzodepedro April 21, 2021 15:46

moved everything into gamelogic

4a9f551

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

azeey approved these changes Apr 28, 2021

View reviewed changes

chapulina reviewed May 5, 2021

View reviewed changes

Lobotuerk and others added 2 commits May 14, 2021 13:06

test added

45044f7

Signed-off-by: Tomas Lorente <jtlorente@ekumenlabs.com>

Merge branch 'ign-gazebo4' into lobotuerk/haltMotion

bbfe0c0

nkoenig self-requested a review May 17, 2021 20:14

nkoenig mentioned this pull request May 17, 2021

Implements vehicle damage based on kinetic energy osrf/subt#916

Merged

Merge branch 'ign-gazebo4' into lobotuerk/haltMotion

f2ff0bc

nkoenig approved these changes May 18, 2021

View reviewed changes

Fix codecheck

6b19cd7

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

nkoenig merged commit 27ef720 into ign-gazebo4 May 18, 2021

nkoenig deleted the lobotuerk/haltMotion branch May 18, 2021 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding HaltMotion (Or new suggested name) to physics plugin #728

Adding HaltMotion (Or new suggested name) to physics plugin #728

Lobotuerk commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

azeey left a comment

Lobotuerk commented Apr 8, 2021

nkoenig commented Apr 9, 2021

nkoenig commented Apr 9, 2021

Lobotuerk commented Apr 20, 2021

azeey commented Apr 21, 2021 •

edited

Loading

azeey left a comment

chapulina left a comment

Lobotuerk commented May 18, 2021

Adding HaltMotion (Or new suggested name) to physics plugin #728

Adding HaltMotion (Or new suggested name) to physics plugin #728

Conversation

Lobotuerk commented Mar 30, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

codecov bot commented Mar 31, 2021 • edited Loading

Codecov Report

azeey left a comment

Choose a reason for hiding this comment

Lobotuerk commented Apr 8, 2021

nkoenig commented Apr 9, 2021

nkoenig commented Apr 9, 2021

Lobotuerk commented Apr 20, 2021

azeey commented Apr 21, 2021 • edited Loading

azeey left a comment

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

Lobotuerk commented May 18, 2021

Lobotuerk commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

azeey commented Apr 21, 2021 •

edited

Loading