Attach SceneNode to simulation #492

aclegg3 · 2020-02-13T16:32:50Z

Motivation and Context

This change enables an existing SceneNode to be attached to a newly instanced physics object. Also enables removal of objects from simulation while deleting either the object's full SceneNode structure, the visual component only, or nothing.

This enables rendering of embodied agents (#332) and continuous control of an Agent's state via PhysicsManager functions.

Added python bindings for this functionality.

How Has This Been Tested

C++ and python unit tests and new viewer demo option to attach/detach the agent to/from physics with 'p'. While attached to an object, 'w','a','s','d' command dynamic linear velocity of the attached object (instead of navmesh discrete control of the agent) and LEFT|RIGHT command local angular velocity (+-Y axis). Edit: viewer demo shown below removed to keep things clean, but functionality is the same.

Example - agent attachment to a can with user controlled rolling:

Example - agent attachment to a locobot with user control:

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

… a physical object's SceneNode heirarchy. Enabled simple deletion of visualNode_ when detaching SceneNode from PhysicsManager without deleting the node.

codecov · 2020-02-13T16:45:47Z

Codecov Report

Merging #492 into master will increase coverage by 0.19%.
The diff coverage is 96.36%.

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   61.04%   61.23%   +0.19%     
==========================================
  Files         164      164              
  Lines        7500     7538      +38     
  Branches       84       84              
==========================================
+ Hits         4578     4616      +38     
  Misses       2922     2922

Flag	Coverage Δ
#CPP	`56.04% <95.45%> (+0.32%)`	⬆️
#JavaScript	`10% <ø> (ø)`	⬆️
#Python	`80.78% <100%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
src/esp/sim/Simulator.h	`100% <ø> (ø)`	⬆️
src/esp/physics/RigidObject.h	`10.34% <ø> (ø)`	⬆️
src/esp/physics/PhysicsManager.h	`70% <ø> (ø)`	⬆️
tests/test_physics.py	`100% <100%> (ø)`	⬆️
src/esp/physics/bullet/BulletRigidObject.cpp	`70.2% <100%> (ø)`	⬆️
habitat_sim/simulator.py	`95.29% <100%> (ø)`	⬆️
src/esp/physics/RigidObject.cpp	`34.9% <100%> (ø)`	⬆️
src/tests/SimTest.cpp	`100% <100%> (ø)`	⬆️
src/tests/PhysicsTest.cpp	`97.63% <100%> (+0.33%)`	⬆️
src/esp/physics/bullet/BulletPhysicsManager.cpp	`58.11% <77.77%> (+0.97%)`	⬆️
... and 1 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 f261f07...8fb4280. Read the comment docs.

src/esp/physics/PhysicsManager.cpp

src/esp/physics/RigidObject.cpp

matthewjmay · 2020-02-26T08:43:22Z

src/esp/physics/RigidObject.h

+  /**
+   * @brief All Drawable components are children of this node.
+   */
+  scene::SceneNode* visualNode_ = nullptr;


any reason this is public versus having a getter (like node())?

Reasoning: Currently, PhysicsManager needs access to these pointers and they can be null, so references aren't ideal. The getters would then be for raw pointer anyway. Users should not be getting direct access to a RigidObject to manipulate these, so I left them at public.

bigbike · 2020-03-03T23:48:50Z

src/tests/PhysicsTest.cpp

@@ -408,3 +408,52 @@ TEST_F(PhysicsManagerTest, TestVelocityControl) {
    ASSERT_EQ(physicsManager_->getAngularVelocity(objectId), Magnum::Vector3{});
  }
 }
+
+TEST_F(PhysicsManagerTest, TestSceneNodeAttachment) {


From now on, suggest using Corrade::TestSuite.

Yes but definitely in a separate PR :)

The new test doesn't do anything that would strongly benefit from switching to TestSuite, so I'm fine with keeping it in gtest and rewriting with TestSuite later.

bigbike · 2020-03-03T23:49:35Z

src/tests/PhysicsTest.cpp

+
+  esp::assets::PhysicsObjectAttributes physicsObjectAttributes;
+  physicsObjectAttributes.setString("renderMeshHandle", objectFile);
+  resourceManager_.loadObject(physicsObjectAttributes, objectFile);


Underscore is not needed.

This is a call to PhysicsManagerTest::resourceManager_, so underscore is needed. Did I miss your meaning?

Oh, I see. This is the member variable of the class PhysicsManagerTest.

My bad.

src/utils/viewer/viewer.cpp

bigbike · 2020-03-04T00:07:18Z

src/esp/physics/PhysicsManager.cpp

@@ -88,21 +91,27 @@ int PhysicsManager::addObject(const int objectLibIndex,

 int PhysicsManager::addObject(const std::string& configFile,
                              DrawableGroup* drawables,
+                              scene::SceneNode* attachmentNode,


May I understand why this argument is a must have?
Can we just create a scene object within addObject(...), and thus hide this detail to the user?
I did not see any usage case in this PR that leverage this argument. The only caller (in Simulator.cpp) just passed a nullptr.

Did I miss the big picture here?

I pushed this into python and wrote a test case in test_physics.py to demonstrate the usage. This PR is really to address the visual/simulated agent problems. An Agent SceneNode can be passed to addObject to use that node instead of generating a new one.

bigbike · 2020-03-04T18:06:22Z

src/tests/PhysicsTest.cpp

+
+  esp::assets::PhysicsObjectAttributes physicsObjectAttributes;
+  physicsObjectAttributes.setString("renderMeshHandle", objectFile);
+  resourceManager_.loadObject(physicsObjectAttributes, objectFile);


Oh, I see. This is the member variable of the class PhysicsManagerTest.

My bad.

* Add SceneNode attachment option to addObject and makeRigidObject. * Added RigidObject::visualNode_ to clearly separate Drawable branch of a physical object's SceneNode hierarchy. * Enabled optional deletion of visualNode_ and object SceneNode hierarchy when removing objects from PhysicsManager. * Added python bindings and unit tests in python and C++.

* typed Habitat Core * Fix more habitat typing issues * Fix objnav typecheck * Fix incorrect config type * complete habitat lab mypy conversion * Migrate mypy config to mypy.ini * Remove unused mypy config values * Re-enable episodes setter * Finish typing habitat_baselines * Stricten type hints in habitat.core.simulator * Fix registry type add constructor to Simulator * Fix import error * Make max singleton * Remove list copy * Update mypy with global ignores * Do better list casting * Update pre-commit hooks * unify pre-commit style * Add check-yaml pre-commit hook * Bugfix and optimize ci tests * Remove one more type ignore * Update habitat_baselines/rl/ppo/ppo.py * Fix ppo imports * remove dead code * Revert "remove dead code" This reverts commit acf9fd8c3e7f7989137ad54eaacab158c23dd1b4.

aclegg3 added 7 commits February 11, 2020 13:17

Add SceneNode attachment option to addObject and makeRigidObject

877eaf5

Added RigidObject::visualNode_ to clearly separate Drawable branch of…

cb93bc6

… a physical object's SceneNode heirarchy. Enabled simple deletion of visualNode_ when detaching SceneNode from PhysicsManager without deleting the node.

Added unit tests

7726466

refined viewer demo

8a91de9

Added dynamic velocity control to embodied agent demo

9f6efd2

Merge remote-tracking branch 'origin/master' into attach-agent

2c4a870

updated docs

df5fe3f

aclegg3 requested review from bigbike, dhruvbatra and msbaines February 13, 2020 16:32

aclegg3 self-assigned this Feb 13, 2020

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 13, 2020

matthewjmay reviewed Feb 26, 2020

View reviewed changes

aclegg3 added 2 commits March 2, 2020 13:37

Merge remote-tracking branch 'origin/master' into attach-agent

9e52d7d

Addressed review comments

2d3d294

bigbike requested review from erikwijmans and mosra March 3, 2020 23:46

bigbike reviewed Mar 3, 2020

View reviewed changes

bigbike suggested changes Mar 4, 2020

View reviewed changes

aclegg3 added 2 commits March 4, 2020 08:49

revert demo changes to viewer.cpp

d6bbcdd

Push agent attachment functionality into python and test.

8fb4280

aclegg3 requested a review from bigbike March 4, 2020 17:43

bigbike approved these changes Mar 4, 2020

View reviewed changes

aclegg3 merged commit eb700cd into master Mar 4, 2020

aclegg3 deleted the attach-agent branch March 4, 2020 19:04

aclegg3 mentioned this pull request Mar 19, 2020

Render the Agent! #332

Closed

aclegg3 mentioned this pull request Apr 3, 2020

Controller for Habitat Agent facebookresearch/habitat-lab#356

Closed

dhruvbatra mentioned this pull request May 2, 2020

Interactive RigidObject python tutorial page #611

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attach SceneNode to simulation #492

Attach SceneNode to simulation #492

aclegg3 commented Feb 13, 2020 •

edited

Loading

codecov bot commented Feb 13, 2020 •

edited

Loading

matthewjmay Feb 26, 2020

aclegg3 Mar 2, 2020

bigbike Mar 3, 2020

mosra Mar 4, 2020

bigbike Mar 3, 2020

aclegg3 Mar 4, 2020

bigbike Mar 4, 2020

bigbike Mar 4, 2020

aclegg3 Mar 4, 2020

bigbike Mar 4, 2020

Attach SceneNode to simulation #492

Attach SceneNode to simulation #492

Conversation

aclegg3 commented Feb 13, 2020 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

codecov bot commented Feb 13, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aclegg3 commented Feb 13, 2020 •

edited

Loading

codecov bot commented Feb 13, 2020 •

edited

Loading