[WIP][AO migration] collision groups, Bullet debugging, contact queries, RigidObject changes by aclegg3 · Pull Request #1247 · facebookresearch/habitat-sim

aclegg3 · 2021-05-18T19:27:06Z

Motivation and Context

ArticulatedObject migration PR split from #1226.

This PR wraps all significant physics related changes which are not directly related to ArticulatedObjects:

Bullet debug queries (contact points and step summary)
custom CollisionGroup configuration
contactTest with configurable STATIC filter
refactoring object activation API
refactoring collision compound construction from mesh hierarchy

These changes are being further broken into individual PRs for review iteration/polish and this PR will be closed.

How Has This Been Tested

Should be covered by current tests. Expanded a PhysicsTest to check new contactTest functionality.

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.

jturner65 · 2021-05-18T21:49:56Z

+  template <typename Func>
+  void processActiveManifolds(btMultiBodyDynamicsWorld* bWorld, Func func);


jturner65

LGTM. Just a few docstrings missing that I found.

jturner65 · 2021-05-18T21:50:41Z

+  }
+  std::string getStepCollisionSummary() override {
+    return BulletDebugManager::get().getStepCollisionSummary(bWorld_.get());
+  }


docstrings for these

jturner65 · 2021-05-18T21:51:33Z

+  void lookUpObjectIdAndLinkId(const btCollisionObject* colObj,
+                               int* objectId,
+                               int* linkId) const;
+


doc strings

eundersander

I left a bunch of comments. Overall, I'm concerned here. I implemented CollisionGroupHelper and BulletDebugManager, and I certainly intended them as short-term hacks to unblock Andrew, not well-engineered solutions to be merged into master. Unless we are in a rush to get this merged into master, we should step back and re-think these interfaces.

Update: my biggest concern is that the CollisionGroupHelper interface will force Hab2.0 users to fork hab-sim (instead of using, say, a conda installation). This is because another user is unlikely to want/need the exact set of groups and group interactions that Andrew needed, which is currently what CollisionGroupHelper hard-codes.

eundersander · 2021-05-18T23:42:55Z

+        };
+*/
+
+enum class CollisionGroup {


This list is specific to Andrew's Fetch rearrangement project, would you agree? Do you expect that hab-sim users need to always fork hab-sim for their application?

I suggest that we add python bindings to let the application specify collision groups as well as their interactions, rather than hard-coding these.

CollisionGroupHelper is a class I created as a temporary hack. I don't think it's suited to merge to master as-is.

eundersander · 2021-05-18T23:46:43Z

+   * @brief Describe collision-filtering for all known collision objects (see
+   * also mapCollisionObjectTo).
+   *
+   * Unfortunately, this will cause a crash if you have added and then removed


This is not robust enough as-is to merge to master, as described here. I have an idea for a fix that I will post in the cpp.

eundersander · 2021-05-18T23:48:41Z

+
+std::string BulletDebugManager::getCollisionFilteringSummary(bool doVerbose) {
+  std::stringstream s2;
+  for (const auto& pair0 : collisionObjectToDebugName_) {


The declaration for the function mentions how this function is unsafe (it can crash). The issue is that Bullet collision objects can be deleted but this debug manager doesn't get notified of that.

One solution is to change this function to start by iterating through all Bullet collision objects known to Bullet (this list must exist somewhere). In this way, we naturally skip over items in collisionObjectToDebugName_ corresponding to deleted collision objects.

eundersander · 2021-05-18T23:50:06Z

+        s << "  (none)" << std::endl;
+      }
+
+      LOG(INFO) << s.str();


This logging is a horrible hack I did as a workaround at one point (specifically, I had trouble printing very long strings to stdout). This function shouldn't directly log; it should return all results in the string.

eundersander · 2021-05-18T23:51:55Z

+      return;
+    }
+    // TODO: will come with AO support
+    /* else if (existingArticulatedObjects_.count(rawObjectId)) {


What's going on here? This function looks half-finished.

Just trying to show why linkId is part of this function at all, since it is still valid for rigids. This block should be uncommented with AOs.

eundersander · 2021-05-18T23:54:17Z

+  Noncollidable = 256
+};
+
+class CollisionGroupHelper {


documentation for this class?

eundersander · 2021-05-18T23:55:06Z

+  Noncollidable = 256
+};
+
+class CollisionGroupHelper {


We should have a unit test for this class's functionality (setting collision group for objects and verifying they interact or don't correctly).

eundersander · 2021-05-18T23:58:23Z

+  };
+*/
+
+int CollisionGroupHelper::getMaskForGroup(CollisionGroup group) {


Two problems with this implementation:

It is app-specific and shouldn't be hard-coded in hab-sim (as I've already mentioned)

As far as actually specifying collision-group interaction (whether in C++ or python), a better way to do this is to explicitly enable interaction between pairs of groups. A problem with the current approach here is that you are able to express contradictory interactions: for example, in Static's case statement, Static can be set to interact with GraspedObject, while in the GraspedObject case statement, GraspedObject might be set to not interact with Static.

…ions

aclegg3 · 2021-05-26T21:22:29Z

Not much value left in this WIP PR.

Alexander William Clegg added 2 commits May 18, 2021 11:59

1st pass migrating non-AO physics and collision changes onto master

bce94c7

refactor isActive and setActive

7f27aec

aclegg3 requested review from eundersander and jturner65 May 18, 2021 19:27

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 18, 2021

jturner65 reviewed May 18, 2021

View reviewed changes

jturner65 approved these changes May 18, 2021

View reviewed changes

eundersander reviewed May 19, 2021

View reviewed changes

aclegg3 mentioned this pull request May 20, 2021

[AO migration] minor physics changes #1254

Merged

11 tasks

Merge remote-tracking branch 'origin/master' into ao-migration-collis…

de2574a

…ions

aclegg3 changed the title ~~[AO migration] collision groups, Bullet debugging, contact queries, RigidObject changes~~ [WIP][AO migration] collision groups, Bullet debugging, contact queries, RigidObject changes May 21, 2021

This was referenced May 21, 2021

[AO migration] query contact points #1257

Merged

[WIP][AO migration] collision groups and modified contactTest #1260

Closed

aclegg3 closed this May 26, 2021

aclegg3 deleted the ao-migration-collisions branch May 28, 2021 23:12

		template <typename Func>
		void processActiveManifolds(btMultiBodyDynamicsWorld* bWorld, Func func);

Conversation

aclegg3 commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jturner65 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eundersander left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aclegg3 commented May 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aclegg3 commented May 18, 2021 •

edited

Loading

eundersander left a comment •

edited

Loading