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

Bullet Physics Integration #1 #91

Merged
merged 92 commits into from Aug 16, 2019

Conversation

hzyjerry
Copy link
Contributor

@hzyjerry hzyjerry commented Jun 28, 2019

Motivation and Context

  • First PR towards physical interactions with Habitat 😊
  • Early feedbacks are severely welcomed 🎉
  • Laying the foundation for upcoming development (articulated agents & objects, robotics, etc) requires moderate refactoring of existing habitat code and data organizations.

Types of changes

  • Introduced esp/assets/PhysicsManager (similar to esp/assets/ResourceManager in style) to track & manage states of physics simulation, such as bulletWorld, FPS, timestep, objects etc
  • Introduced esp/physics/BulletObject as an extended SceneNode. BulletObject interacts with PhysicsManager to create collision mesh, set object properties, etc.
  • Supporting APIs: generic object state update, instancing
    • loadObject(xyz, rotation)
    • setObject(ID, xyz, rotation)
    • removeObject(ID)
    • Move functions from viewer.cpp to habitat core
    • Support object instancing by ID
  • Testing

Challenges 💥

  • File formats: getting GLB/obj/ply objects to all work correctly (E.g. Bullet mesh uses PHY_INTEGER, incompatible with Magnum::UnsignedInt)
  • Object: requires preprocessing before we can use them
  • Speed: physics simulation slows down significantly when there are >20 objects
  • Physical metadata: how to store physical properties (friction, restitution, mass)

Checklist

  • Magnum mesh -> Bullet mesh object Interface
    • Integrate Physics Loader based on Magnum Issue
    • Load GLB object/scene composite mesh
    • Load FRL object/scene composite mesh (bullet can handle <= 1.5M triangles)
    • Load PLY object/scene composite mesh
  • Update habitat object loader to be more physics-ready
    • Currently GLB/FRL/PLY loaders work differently. A uniform getMeshData() function can better interface with PhysicsManager to create collision mesh
  • Use smart pointers
  • Simple object Interactions
    • Apply force by object id
    • Apply impulse by object id (optional)
  • Prepare demo script
    • View.cpp with --enbable-physics option
    • View.cpp backward compatible
  • Physical Object management in PhysicsManager and ResourceManager
    • Refactoring: separate loadObject and addObject
    • Support cloning object by cloning sceneNode (decided to push to next PR)
    • Test and support instancing
  • JSON Config files
    • Object config for: friction, mass, restitution, center of mass
    • Scene config for: list of objects, scene name, scene layout
  • Prepare object meshes
    • Pre-processing: we require all objects to be convex-decomposed. Provide preprocessing script for end users.
    • Provide starter object mesh files

PR Milestones

July 2 GLB Scene
July 4 Surreal Scene
July 5 Replica Scene
Tested on Gibson & MP3D
Screen Shot 2019-08-02 at 10 39 16 AM

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jun 28, 2019
@abhiskk abhiskk requested a review from bigbike June 28, 2019 18:49
Copy link
Contributor

@erikwijmans erikwijmans left a comment

Choose a reason for hiding this comment

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

Left some high-level comments. This should be rebased on master ASAP as that will make seeing how it interfaces with the current code easier.

src/esp/assets/FRLInstanceMeshData.h Outdated Show resolved Hide resolved
src/esp/assets/FRLInstanceMeshData.h Outdated Show resolved Hide resolved
src/esp/assets/PhysicsManager.h Outdated Show resolved Hide resolved
src/esp/assets/ResourceManager.cpp Outdated Show resolved Hide resolved
src/esp/assets/ResourceManager.cpp Outdated Show resolved Hide resolved
src/esp/assets/ResourceManager.cpp Outdated Show resolved Hide resolved
@@ -227,15 +461,27 @@ bool ResourceManager::loadGeneralMeshData(const AssetInfo& info,
MeshMetaData metaData;
loadTextures(*importer, &metaData);
loadMaterials(*importer, &metaData);
loadMeshes(*importer, &metaData);
loadMeshes(*importer, &metaData, shiftOrigin);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the origin shift needed? That will probably break all of our current episode datasets. We will also have to make sure we propagate the new origin to the annotations for MP3D.

Copy link
Contributor

Choose a reason for hiding this comment

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

Default shift is false. If shifted, the transform is now stored for query. This is only expected to be used for object meshes, not static scenes.

Copy link
Collaborator

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Just quickly went through -- please have a look at the reinterpret_casts ;)

src/cmake/FindMagnumIntegration.cmake Outdated Show resolved Hide resolved
src/esp/assets/CMakeLists.txt Outdated Show resolved Hide resolved
src/esp/assets/CMakeLists.txt Outdated Show resolved Hide resolved
src/esp/assets/FRLInstanceMeshData.cpp Outdated Show resolved Hide resolved
src/esp/assets/FRLInstanceMeshData.h Outdated Show resolved Hide resolved
}

// TODO (JH) Bottom up search gives bus error because scene"s rootnode does
// not point to nullptr, but something strange
Copy link
Collaborator

Choose a reason for hiding this comment

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

... or maybe it points to Scene from Magnum's SceneGraph and so static_cast<const scene::SceneNode*> will likely not work?

src/esp/gfx/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@mathfac mathfac left a comment

Choose a reason for hiding this comment

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

As soon it's passing CI it looks good to be merged.

tinyply
PRIVATE
Assimp::Assimp
geo
io
)

set_directory_properties(PROPERTIES CORRADE_USE_PEDANTIC_FLAGS ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need this?

(*cpu_vbo_3_)[i] = cpu_vbo_[i].head<3>();
}

// create ibo converting quads to tris [0, 1, 2, 3] -> [0, 1, 2],[0,2,3]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// create ibo converting quads to tris [0, 1, 2, 3] -> [0, 1, 2],[0,2,3]
// create ibo converting quads to tris [0, 1, 2, 3] -> [0, 1, 2], [0, 2, 3]

@aclegg3 aclegg3 merged commit 9575dcd into facebookresearch:master Aug 16, 2019
@mathfac mathfac added this to 2019 Q3 in Habitat Roadmap Sep 5, 2019
@mathfac mathfac moved this from 2019 Q3 to 2019 Q4 in Habitat Roadmap Sep 5, 2019
@mathfac mathfac removed this from 2019 Q4 in Habitat Roadmap Sep 5, 2019
@mathfac mathfac deleted the bullet-integration branch September 5, 2019 22:24
This was referenced Sep 13, 2019
"FPS is below regression threshold: %0.1f < %0.1f"
% (perf["fps"], args.test_fps_regression)
)
# assert perf["fps"] > args.test_fps_regression, (
Copy link
Contributor

Choose a reason for hiding this comment

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

@aclegg3, maybe I missed the comment. Can you remind me why we commented out the speed regression check??

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be there when not running with physics. May have been commented out in the rush to verify physics pybind, sorry for that. We will need a separate threshold for the new physics benchmarks, but we can add this back when enable_physics is false.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aclegg3, no worries, just wanted to check there is no serious reason to disable it.

@aclegg3 aclegg3 mentioned this pull request Oct 16, 2019
11 tasks
eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
* Introduced esp/assets/PhysicsManager to track & manage states of kinematic and physically simulated objects in Habitat-sim. Functionality includes adding/removing objects, kinematic updates, applying forces/torques, getters/setters for physical properties.

* Introduced esp/physics/BulletObject as an extended SceneNode. BulletObject interacts with PhysicsManager to create collision mesh, set object properties, and interface with specific physics engine implementations.

* Introduced global and object specific physics config files (JSON), parsers, containers, and resource libraries in esp/assets/ResourceManager.

* Introduced dynamic simulation of rigid objects through optional build with Bullet physics engine (build argument --bullet with Bullet3 installed).

* Implemented python bindings for basic physics functionality exposed through simulator.py.

* Implemented physics functionality demos in C++ (Viewer.cpp) and python (example.py).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants