[culling] Compute absolute AABB (NOT the existing local AABB) for each mesh component of the static scene (ptex, mp3d ONLY) #400

bigbike · 2019-12-21T01:15:37Z

Motivation and Context

Computes an AABB in world space for each mesh component of the static scene;
Applies to ptex mesh and MP3D mesh;
Will not compute absolute AABB for dynamic object, or instance scene;

Each mesh component corresponds to a static drawable. Static drawables have fixed absolute transformations during the runtime. Repeat computation is not necessary;
absolute AABB will be used in the frustum culling process;
more details: [DO NOT accept][Experiment only] Some quick, hacky tests on frustum culling #380

Existing issue: #390

How Has This Been Tested

local

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.

-) compute the absTransform; -) staticDrawables_; TODO: absPositions

abs aabb for MP3D TODO: abs aabb for ptex

abs aabb ptex

bigbike · 2019-12-21T01:23:32Z

Code is ready to be reviewed.

Thanks.

bigbike · 2019-12-21T01:51:09Z

@msb:
I already added: // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
But clang-tidy still complains the potential memory leaks.

erikwijmans

It seems that this PR introduces a second way to compute bounding boxes for a mesh as there is already the functionality to compute bounding boxes for physics related things in the code base. @bigbike @aclegg3 can you guys work together to figure out a way to unify this?

erikwijmans · 2019-12-21T20:45:13Z

src/esp/assets/ResourceManager.h

+  //
+  // -) ptex mesh:
+  // meshID is the index of the submesh corresponding to the drawable;
+  std::vector<std::pair<std::reference_wrapper<esp::gfx::Drawable>, uint32_t>>


This vector is concerning. Are we sure that we won't try to access the drawable after it gets deleted?

This is a valid concern. However, it is unlikely to happen.
This variable so far is used as a "temporary" variable that is to collect drawables, and meshIDs at the loading stage. After that, it will not be used any more.

It is needed in multiple functions, so that I put it here as a member variable to avoid passing it as a function parameter.

I think solution is easy:
What I can do to address your concern is to clear this vector after its job is completed in loading stage.

Is the drawable being used in anyway besides getting access to the node? If so, why not store a reference to the scene node? The scene node and drawable should have very similar lifetimes so it should be equally safe.

Yes, good suggestion. In terms of functionality, both methods are the same. But I read my code, and what you suggested can potentially lead to simpler code. So why not? Let me try to change it.

src/esp/assets/ResourceManager.cpp

bigbike · 2019-12-21T21:10:54Z

It seems that this PR introduces a second way to compute bounding boxes for a mesh as there is already the functionality to compute bounding boxes for physics related things in the code base. @bigbike @aclegg3 can you guys work together to figure out a way to unify this?

These 2 AABBs are different.
One is in local model frame, and the other is in world coordinate frame.

src/esp/assets/ResourceManager.cpp

bigbike · 2019-12-21T21:08:38Z

src/esp/assets/ResourceManager.h

+  //
+  // -) ptex mesh:
+  // meshID is the index of the submesh corresponding to the drawable;
+  std::vector<std::pair<std::reference_wrapper<esp::gfx::Drawable>, uint32_t>>


This is a valid concern. However, it is unlikely to happen.
This variable so far is used as a "temporary" variable that is to collect drawables, and meshIDs at the loading stage. After that, it will not be used any more.

It is needed in multiple functions, so that I put it here as a member variable to avoid passing it as a function parameter.

I think solution is easy:
What I can do to address your concern is to clear this vector after its job is completed in loading stage.

bigbike · 2019-12-21T21:22:41Z

These 2 AABBs are different.
One is in local model frame, and the other is in world coordinate frame.

Note:
If you apply absolute transformation to the local AABB, it would become an OBB in world space. Then if you create a "new AABB" of this OBB, this "new AABB" is different from the AABB that is computed here. The AABB computed here usually is much tighter.

bigbike · 2019-12-22T00:28:33Z

The correctness of the computed absolute AABBs was tested in the branch called "culling":
https://github.com/facebookresearch/habitat-sim/compare/culling

When frustum culling is enabled, it uses the absolute AABBs computed in this PR.
The rendering is visually correct with/without frustum culling enabled. This is tested and confirmed with one MP3D model, and one PTex model respectively.
It improves the FPS and can be identified when loading the PTex model. (Cannot tell the FPS increase when loading MP3D model due to the v-sync lock on mac.)

See the attached:
NO Culling:

With Culling:

erikwijmans · 2019-12-22T05:32:41Z

These 2 AABBs are different.
One is in local model frame, and the other is in world coordinate frame.

Note:
If you apply absolute transformation to the local AABB, it would become an OBB in world space. Then if you create a "new AABB" of this OBB, this "new AABB" is different from the AABB that is computed here. The AABB computed here usually is much tighter.

Agreed, we'd need to compute an OBB in the first place. Then that can be either converted to an AABB or transformed and converted to an AABB.

src/esp/assets/ResourceManager.cpp

bigbike · 2019-12-23T18:54:44Z

src/esp/assets/ResourceManager.h

+  //
+  // -) ptex mesh:
+  // meshID is the index of the submesh corresponding to the drawable;
+  std::vector<std::pair<std::reference_wrapper<esp::gfx::Drawable>, uint32_t>>


Yes, good suggestion. In terms of functionality, both methods are the same. But I read my code, and what you suggested can potentially lead to simpler code. So why not? Let me try to change it.

…neNode, meshID>

bigbike · 2020-01-03T23:38:47Z

This PR is ready to be reviewed.
@mosra: I appreciate if you can take a look as well. Thanks.

erikwijmans · 2020-01-04T00:21:58Z

src/esp/assets/ResourceManager.cpp

-                                  drawables};
+
+        // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
+        gfx::PTexMeshDrawable* d = new gfx::PTexMeshDrawable{


I believe this can just be new gfx::PTexMeshDrawable{node, *ptexShader, *pTexMeshData, jSubmesh, drawables}; again.

Yes, right. Good catch. It was needed before as I stored drawables in previous commits. (Now I store the scene node).

erikwijmans · 2020-01-04T00:22:13Z

src/esp/assets/ResourceManager.cpp

+
+    // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
+    gfx::PrimitiveIDDrawable* d =
+        new gfx::PrimitiveIDDrawable{node, *shader, mesh, group};


This change can be undone.

erikwijmans · 2020-01-04T00:23:17Z

src/esp/assets/ResourceManager.cpp

-                                          objectId, color);
+
+    // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
+    gfx::GenericDrawable* d = new gfx::GenericDrawable{


Yes, right.

aclegg3

Looks good, minor comments. I understand the desire for precise bounding boxes for the single static scene given worst case approximation.

Would be curious to know the difference between the approximate and precise bounding boxes computed over the Gibson or Replica datasets.

src/esp/assets/BaseMesh.h

aclegg3 · 2020-01-04T02:29:55Z

src/esp/assets/ResourceManager.cpp

@@ -65,6 +69,13 @@ namespace assets {
 bool ResourceManager::loadScene(const AssetInfo& info,
                                scene::SceneNode* parent, /* = nullptr */
                                DrawableGroup* drawables /* = nullptr */) {
+  // we only compute absolute AABB for evey mesh component when loading ptex


typo: every

aclegg3 · 2020-01-04T02:38:24Z

src/esp/assets/ResourceManager.h

+   */
+  void computeGeneralMeshAbsoluteAABBs();
+
+  /*


aclegg3 · 2020-01-04T02:38:59Z

src/esp/assets/ResourceManager.h

+  // this helper vector contains information of the drawables on which we will
+  // compute the absolute AABB
+  //
+  // pair: <Drawable's scene node, meshID>
+  //
+  // -) non-ptex mesh:
+  // meshID is the global index into meshes_.
+  // meshes_[meshID] is the BaseMesh corresponding to the drawable;
+  //
+  // -) ptex mesh:
+  // meshID is the index of the submesh corresponding to the drawable;
+  std::vector<
+      std::pair<std::reference_wrapper<esp::scene::SceneNode>, uint32_t>>
+      staticDrawableInfo_;
+  bool computeAbsoluteAABBs_ = false;


Reformat the comments for docs? 😃

bigbike · 2020-01-04T03:29:49Z

@aclegg3 :

Would be curious to know the difference between the approximate and precise bounding boxes computed over the Gibson or Replica datasets.

Good question. To answer it, I need to do experiment to collect data and compare the culling rate.

To compute the absolute AABB instead of approximation, better accuracy is indeed a purpose. However, there is another even more important reason -- save computation.

Because, culling test can only be done when frustum and the bounding box are in the same space (e.g., world, camera, or model space). And to make the test efficient, the bounding box must be a AABB.

If we use the existing "local" AABB in the scene node (developed by you), we will have to transform the frustum by a constant transformation matrix from clip space to model space (because the bounding box is a AABB only in the model space). That means we need to compute the "mvp" for every object in every frame. But with the absolute AABB, we only need to compute "vp" once for all objects in every frame.

msbaines

Please add a test.

msbaines · 2020-01-08T22:20:21Z

src/esp/assets/ResourceManager.cpp

 }

 void ResourceManager::createDrawable(
    const ShaderType shaderType,
    Magnum::GL::Mesh& mesh,
    scene::SceneNode& node,
+    Corrade::Containers::Optional<uint32_t> meshID, /* = NullOpt */


We seem to be over-loading createDrawable with a lot o responsiblities. Not sure if the code might be simpler if it the new functionality were added to addMeshToDrawables instead.

msbaines · 2020-01-08T22:20:28Z

src/esp/assets/ResourceManager.h

+   * -) ptex mesh:
+   * meshID is the index of the submesh corresponding to the drawable;
+   */
+  std::vector<


Maybe use a proper struct instead of a pair since this is used in such a large scope.

It also seems like this should really be a local variable instead of a member for this.

src/esp/scene/SceneNode.h

bigbike · 2020-01-09T00:01:42Z

Synced up with @msbaines offline. His comments on data and code structures, tests makes a lot of sense. I really appreciated it. Will change accordingly.

bigbike · 2020-01-11T02:57:05Z

Ready for another round review while I am writing the unit tests.

codecov-io · 2020-01-11T03:14:22Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ae15cf3). Click here to learn what that means.
The diff coverage is 90.32%.

@@            Coverage Diff            @@
##             master     #400   +/-   ##
=========================================
  Coverage          ?   57.46%           
=========================================
  Files             ?      160           
  Lines             ?     6983           
  Branches          ?       84           
=========================================
  Hits              ?     4013           
  Misses            ?     2970           
  Partials          ?        0

Flag	Coverage Δ
#CPP	`52.16% <88.88%> (?)`
#JavaScript	`10% <ø> (?)`
#Python	`78.97% <100%> (?)`

Impacted Files	Coverage Δ
src/esp/sensor/Sensor.h	`81.81% <ø> (ø)`
habitat_sim/__init__.py	`100% <ø> (ø)`
src/esp/sim/Simulator.h	`100% <ø> (ø)`
src/tests/PhysicsTest.cpp	`100% <ø> (ø)`
src/esp/assets/ResourceManager.h	`100% <ø> (ø)`
src/esp/sensor/PinholeCamera.h	`100% <ø> (ø)`
src/tests/SimTest.cpp	`100% <ø> (ø)`
src/esp/scene/test/GibsonSceneTest.cpp	`60.6% <ø> (ø)`
habitat_sim/sim.py	`100% <100%> (ø)`
habitat_sim/gfx.py	`100% <100%> (ø)`
... and 2 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 ae15cf3...95031e7. Read the comment docs.

erikwijmans

One suggestion.

erikwijmans · 2020-01-16T16:09:07Z

src/esp/assets/ResourceManager.cpp

@@ -649,6 +687,113 @@ Magnum::Range3D ResourceManager::computeMeshBB(BaseMesh* meshDataGL) {
  return Magnum::Range3D{
      Magnum::Math::minmax<Magnum::Vector3>(meshData.positions)};
 }
+void ResourceManager::computePTexMeshAbsoluteAABBs(BaseMesh& baseMesh) {


IMO, it makes sense to #ifdef the function definition itself instead of the implementation. If we aren't building with ptex, then we can assume this method should/will never be called, so there shouldn't be any code that requires this blank implementation right?

Makes sense to me. Will do.

Still #ifdef'ing the implementation and not the whole function.

I actually cannot add #ifdef to the entire function definition since it would cause the CI test failure. When compiling js, it complained undefined symbols.

If you look at the other functions, e.g., loadPTexMeshData, we did the same thing, namely, adding the conditionals on the body, not the entire definition. So the function is just empty.

I guess I will just leave it like this.

@msbaines @erikwijmans

I think you also need to #ifdef it in the header file.

Make life a little bit simpler

bigbike · 2020-01-23T23:55:33Z

Dear reviewers,

Just added the test as requested.
Sincerely appreciate if you can take another look.
Thank you.

src/esp/assets/ResourceManager.cpp

msbaines · 2020-01-24T00:25:16Z

src/esp/assets/ResourceManager.cpp

@@ -649,6 +687,113 @@ Magnum::Range3D ResourceManager::computeMeshBB(BaseMesh* meshDataGL) {
  return Magnum::Range3D{
      Magnum::Math::minmax<Magnum::Vector3>(meshData.positions)};
 }
+void ResourceManager::computePTexMeshAbsoluteAABBs(BaseMesh& baseMesh) {


Still #ifdef'ing the implementation and not the whole function.

mosra

Just for the record, I don't have any comments/requests regarding this code, everything seems in order. 👍 for using mesh tools and batch functions to calculate transformed vertex AABBs.

docs: remove :ref: as a default role.

…h mesh component of the static scene (ptex, mp3d ONLY) (facebookresearch#400) * working in progress * Done: -) compute the absTransform; -) staticDrawables_; TODO: absPositions * DONE: abs aabb for MP3D TODO: abs aabb for ptex * DONE: abs aabb ptex * fix typo, modify comments * minor * minor * try to address Erik's concern * change the pair in staticDrwableInfo_ from <drawable, meshID> to <sceneNode, meshID> * minor * minor * minor * minor * address CI errors * partially address Mandeep's concern. * minor * using reference directly instead of reference wrapper. Make life a little bit simpler * adding tests (in progress) * correct the path * minor * added tests * address reviewer's concern. * minor * address reviewer's concern

bigbike added 6 commits December 18, 2019 21:35

working in progress

5338542

Done:

a3af93a

-) compute the absTransform; -) staticDrawables_; TODO: absPositions

DONE:

3a81178

abs aabb for MP3D TODO: abs aabb for ptex

DONE:

6d200d6

abs aabb ptex

fix typo, modify comments

88626f4

minor

e6ed87f

bigbike requested review from mosra, aclegg3, erikwijmans and msbaines December 21, 2019 01:15

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 21, 2019

Merge branch 'master' into node_aabb

62d448e

erikwijmans suggested changes Dec 21, 2019

View reviewed changes

bigbike commented Dec 21, 2019

View reviewed changes

bigbike changed the title ~~[culling] Compute absolute AABB for each mesh component of the static scene (ptex, mp3d ONLY)~~ [culling] Compute absolute AABB (NOT the existing local AABB) for each mesh component of the static scene (ptex, mp3d ONLY) Dec 21, 2019

bigbike added 2 commits December 21, 2019 13:25

minor

61f6fe3

try to address Erik's concern

83e257b

bigbike mentioned this pull request Dec 21, 2019

[DO NOT accept][Experiment only] Some quick, hacky tests on frustum culling #380

Closed

11 tasks

bigbike commented Dec 23, 2019

View reviewed changes

change the pair in staticDrwableInfo_ from <drawable, meshID> to <sce…

8a8d709

…neNode, meshID>

erikwijmans reviewed Jan 4, 2020

View reviewed changes

aclegg3 reviewed Jan 4, 2020

View reviewed changes

bigbike added 4 commits January 6, 2020 18:27

minor

58d0ad3

minor

e9e13ec

Merge branch 'master' into node_aabb

96f5282

address CI errors

ce3f394

msbaines reviewed Jan 8, 2020

View reviewed changes

bigbike added 2 commits January 10, 2020 18:45

partially address Mandeep's concern.

dcd4a58

minor

a73eac6

erikwijmans approved these changes Jan 16, 2020

View reviewed changes

bigbike added 7 commits January 22, 2020 15:01

using reference directly instead of reference wrapper.

57f44a2

Make life a little bit simpler

adding tests (in progress)

e4c8975

correct the path

7b8fcf0

Merge branch 'issue442' into node_aabb

239da9f

minor

ae15cf3

Merge branch 'master' into node_aabb

c05d831

added tests

5f671f8

msbaines reviewed Jan 24, 2020

View reviewed changes

address reviewer's concern.

7211413

msbaines approved these changes Jan 24, 2020

View reviewed changes

bigbike added 2 commits January 23, 2020 19:07

minor

5d1cacd

address reviewer's concern

95031e7

bigbike merged commit 83a7ef2 into master Jan 25, 2020

bigbike deleted the node_aabb branch January 25, 2020 00:23

bigbike mentioned this pull request Jan 31, 2020

Linear Frustum Culling #449

Merged

11 tasks

mosra reviewed Jan 31, 2020

View reviewed changes

eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020

Merge pull request facebookresearch#400 from facebookresearch/docs4

1b0af67

docs: remove :ref: as a default role.

[culling] Compute absolute AABB (NOT the existing local AABB) for each mesh component of the static scene (ptex, mp3d ONLY) #400

[culling] Compute absolute AABB (NOT the existing local AABB) for each mesh component of the static scene (ptex, mp3d ONLY) #400

Conversation

bigbike commented Dec 21, 2019

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

bigbike commented Dec 21, 2019

bigbike commented Dec 21, 2019

erikwijmans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbike Dec 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbike commented Dec 21, 2019

bigbike Dec 21, 2019 • edited Loading

Choose a reason for hiding this comment

bigbike commented Dec 21, 2019

bigbike commented Dec 22, 2019

erikwijmans commented Dec 22, 2019

Choose a reason for hiding this comment

bigbike commented Jan 3, 2020

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 left a 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

bigbike commented Jan 4, 2020 • edited Loading

msbaines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbike commented Jan 9, 2020

bigbike commented Jan 11, 2020

codecov-io commented Jan 11, 2020 • edited Loading

Codecov Report

erikwijmans left a 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

bigbike commented Jan 23, 2020

Choose a reason for hiding this comment

mosra left a comment

Choose a reason for hiding this comment

bigbike Dec 21, 2019 •

edited

Loading

bigbike Dec 21, 2019 •

edited

Loading

bigbike commented Jan 4, 2020 •

edited

Loading

codecov-io commented Jan 11, 2020 •

edited

Loading