refactor ResourceManager render assets #935

eundersander · 2020-11-13T19:52:07Z

Motivation and Context

This PR is (mostly) a no-op refactor of ResourceManager (no behavior change). The motivation is to unblock the upcoming render replay feature.

I introduce the terms "render asset" and "render asset instance". A render asset is basically a mesh. A render asset can be of type InstanceMesh ("IMesh"), PTex, Primitive, or General (see isRenderAssetGeneral).

A somewhat unrelated change is mixed in here: I'm converting some usage of ResourceKey (a hash) to std::string so that I can better serialize the key.

This is (mostly) a no-op refactor that conceptually replaces loadPTexMeshData / loadInstanceMeshData / loadGeneralMeshData with loadRenderAssetPTex,createRenderAssetInstancePTex / loadRenderAssetIMesh,createRenderAssetInstanceIMesh / loadRenderAssetGeneral,createRenderAssetInstanceGeneralPrimitive.

Some things you can do with render assets and instances:

Load a render asset (without adding anything to the scene graph; see loadRenderAsset).
Create instances of a render asset (add instances to the scene graph so they show up in observations; see createRenderAssetInstance).
Move a render asset instance in the scene (and coming soon, you can hide or unhide it).
Delete a render asset instance (remove the instance from the scene graph).
Unload a render asset (only after all its instances have been deleted; at present, I think the only way to unload is to reconfigure/reset the simulator).

More details about render assets:

Currently, a render asset instance isn't a formal class/object in our code. It's a subtree of scene nodes and drawables. It is a formal object in the serialization code I'm writing for render replay.
A PhysicsObject's attributes include its render asset. A PhysicsObject has an associated render asset instance at runtime.
A stage has an associated RGBD/"render" render asset instance and optional "semantic" render asset instance.
An articulated object will have an associated render asset instance per link at runtime (articulated objects aren't merged to master yet).

Some goals/benefits of this refactor:

Cleaner separation between render and simulation code: a replay renderer will be able to load and display stage visuals without calling loadStage, processing stageAttributes, or loading stage collision geometry.
unified loading of render assets: loadRenderAsset
unified creation of instances: createRenderAssetInstance
Users of render assets are (mostly) agnostic to the underlying type: stage, objects, and articulated object links have associated render assets, but they (mostly) don't care what type.
Any render asset type can be used for RGBD, Semantic, or both.
Where render asset types differ, we're explicit. E.g. explicit asserts that convey that PTex doesn't support scale or lighting.

Some possible future use cases this unlocks (these are side benefits; the main motivation for this PR is unblocking render replay):

preload render assets (load prior to use)
unload and reload render assets on demand
add separate semantic render assets to objects or links
use PTex or InstanceMesh render assets with objects and links

How Has This Been Tested

Loading stages with render assets of type PTex, InstanceMesh, and General (GLB), in the viewer
Adding physics objects in the viewer
Testing semantic rendering using a modified semantic_id_tutorial.py
I ran SimTest, which tests adding a primitive

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.

eundersander · 2020-11-13T19:57:27Z

src/esp/assets/ResourceManager.cpp

@@ -180,31 +188,85 @@ bool ResourceManager::loadStage(
      ((_physicsManager != nullptr) &&
       (_physicsManager->getInitializationAttributes()->getSimulator().compare(
            "none") != 0));
-  const Magnum::ResourceKey& renderLightSetup(stageAttributes->getLightSetup());
+  const std::string renderLightSetupKey(stageAttributes->getLightSetup());


See https://cvmlp.slack.com/archives/G0131KVLBLL/p1604955346004100

eundersander · 2020-11-13T19:59:26Z

src/esp/assets/ResourceManager.cpp

  std::map<std::string, AssetInfo> assetInfoMap =
      createStageAssetInfosFromAttributes(stageAttributes, buildCollisionMesh,
                                          loadSemanticMesh);

+  // set equal to current Simulator::activeSemanticSceneID_ value


This is not new code. I've simply moved this semantic loading code to happen earlier in this function. I did this because I need to know if a render asset instance is going to be used for RGBD/Semantic/both at the time of creation. I want to know as early as possible whether we're going to be in "single scene graph" vs "dual scene graph" mode.

eundersander · 2020-11-13T20:43:54Z

src/esp/assets/ResourceManager.cpp

+      RenderAssetInstanceCreation creation{
+          .filepath = semanticStageFilename,
+          .scale = Cr::Containers::NullOpt,
+          .isStatic =


isStatic roughly corresponds to the previous logic for computeSemanticAABBs.

Skylion007 · 2020-11-13T20:50:26Z

Broke the JS bindings...

eundersander · 2020-11-13T20:52:58Z

src/esp/assets/ResourceManager.cpp

-      } else if (info.type == AssetType::FRL_PTEX_MESH) {
-        meshSuccess = loadPTexMeshData(info, parent, drawables);
-      } else if (info.type == AssetType::SUNCG_SCENE) {
+      if (info.type == AssetType::SUNCG_SCENE) {


SunCG scenes are not render assets.

eundersander · 2020-11-13T20:55:38Z

src/esp/assets/ResourceManager.cpp

+            // Right now, we only allow for an asset to be loaded with one
+            // configuration, since generated mesh data may be invalid for a new
+            // configuration
+            LOG(ERROR)


This logic was moved from loadGeneralMeshData to here.

eundersander · 2020-11-13T21:13:56Z

src/esp/assets/ResourceManager.cpp

+    pTexMeshData->uploadBuffersToGPU(false);
+
+    for (int jSubmesh = 0; jSubmesh < pTexMeshData->getSize(); ++jSubmesh) {
+      scene::SceneNode& node = instanceRoot->createChild();


This is one of the few behavior changes in the PR. For the instance mesh, instead of adding all these mesh child sceneNodes directly to the parent sceneNode, I first create an instanceRoot sceneNode. I want every render asset instance to have a single root sceneNode. This is needed for render replay.

I acknowledge there's probably a slight perf cost. Long-term, our new Vulkan batched renderer probably won't use a SceneNode hierarchy, so this shouldn't matter.

eundersander · 2020-11-13T21:14:55Z

src/esp/assets/ResourceManager.cpp

+  scene::SceneNode* instanceRoot = &parent->createChild();
+
+  for (uint32_t iMesh = start; iMesh <= end; ++iMesh) {
+    scene::SceneNode& node = instanceRoot->createChild();


See https://github.com/facebookresearch/habitat-sim/pull/935/files?diff=split&w=1#r523235399 .

eundersander · 2020-11-13T21:15:50Z

src/esp/assets/ResourceManager.cpp

-    return false;
+    DrawableGroup* drawables) {
+  ASSERT(!creation.scale);  // PTex doesn't support scale
+  ASSERT(creation.lightSetupKey ==


Explicit asserts to show what a render asset type supports.

eundersander · 2020-11-13T21:26:52Z

src/esp/assets/ResourceManager.cpp

-  }  // forceReload
-     // TODO: cache visual nodes added by this process
-  std::vector<scene::SceneNode*> visNodeCache;
+  if (creation.scale) {


Currently, instances for objects use scale and instances for stages do not.

eundersander · 2020-11-13T21:29:10Z

src/esp/assets/ResourceManager.cpp

@@ -1833,7 +1899,15 @@ bool ResourceManager::loadSUNCGHouseFile(const AssetInfo& houseInfo,
        nodeIds.push_back(id);
        objectNode.setId(nodeIndex);
        if (info.type == AssetType::SUNCG_OBJECT) {
-          loadGeneralMeshData(info, &objectNode, drawables);
+          CHECK(loadRenderAsset(info));


This codepath is untested so who knows if it actually works. https://cvmlp.slack.com/archives/G0131KVLBLL/p1604940887000900

eundersander · 2020-11-13T22:49:41Z

src/esp/assets/ResourceManager.h

   * @param parent The @ref scene::SceneNode to which the mesh will be added
-   * as a child.
+   * as a child. See also creation->isRGBD and creation->isSemantic. nullptr if


There's an important subtlety here: currently, the way the user chooses RGBD vs Semantic is by passing the appropriate parent and drawables (from either the RGBD or Semantic scene graph). But I'm also requiring the user to specify isRGBD and isSemantic in the creation struct (so that I can record the choice in the replay data).

Eventually, we could refactor this interface so that the user just specifies the creation struct, and then we choose the correct parent and drawables on the backend.

aclegg3

Overall looks fine to me. Minor comment that we are using the renderAsset loading pipeline for importing collision proxy assets also. Technically these are render-ready and could be instanced, so I don't think the re-naming is a problem. Just wanted to point this out.

aclegg3 · 2020-11-16T17:54:58Z

src/esp/assets/RenderAssetInstanceCreation.h

+namespace assets {
+
+// parameters to control how a render asset instance is created
+struct RenderAssetInstanceCreation {


Maybe clarify that this is an info struct?

Suggested change

struct RenderAssetInstanceCreation {

struct RenderAssetInstanceCreationInfo {

Do we want to rename the file to match the struct?

bigbike

@mosra : Can you take another round of review? Thanks!

bigbike · 2020-11-14T00:21:29Z

src/esp/assets/ResourceManager.cpp

+namespace {
+bool isRenderAssetGeneral(AssetType type) {
+  return type == AssetType::MP3D_MESH || type == AssetType::UNKNOWN ||
+         type == AssetType::SUNCG_OBJECT;


Shall we remove the last one? We should not touch it, or have any "new" code that relatives to it.

I chatted with Kathryn Duvall from FB legal and she says this will be fine. What I'm trying to do here is not break the SUNCG loading codepath, with the big caveat that we can't test this loading path on SUNCG data or otherwise use SUNCG data. We should consider support for SUNCG data to be deprecated. I will add a runtime warning about this codepath being deprecated.

Awesome! Thanks for following up on this!

bigbike · 2020-11-16T23:26:47Z

src/esp/assets/RenderAssetInstanceCreation.h

+struct RenderAssetInstanceCreation {
+  std::string filepath;  // see also AssetInfo::filepath
+  Corrade::Containers::Optional<Magnum::Vector3> scale;
+  bool isStatic =


can we use the Flags instead of a couple of booleans? It is more efficient.
Here is a good example:

habitat-sim/src/esp/gfx/RenderCamera.h

Line 22 in fe9f78b

enum class Flag : unsigned int {

bigbike · 2020-11-16T23:36:54Z

src/esp/assets/ResourceManager.cpp

+    meshSuccess = loadRenderAssetIMesh(info);
+  } else if (isRenderAssetGeneral(info.type)) {
+    meshSuccess = loadRenderAssetGeneral(info);
+  } else {


CORRADE_INTERNAL_ASSERT_UNREACHABLE() ??
https://doc.magnum.graphics/corrade/Assert_8h.html

Yes, compared to what's there now, CORRADE_INTERNAL_ASSERT_UNREACHABLE() will give the compiler a correct hint, allowing for better optimizations even in release builds.

bigbike · 2020-11-16T23:41:50Z

src/esp/assets/ResourceManager.cpp

+    DrawableGroup* drawables,
+    std::vector<scene::SceneNode*>* visNodeCache) {
+  // assert that asset is already loaded
+  CHECK(resourceDict_.count(creation.filepath));


Do we have anything equivalent to the CHECK or more powerful in Corrade? @mosra

How is a CHECK different from an ASSERT? I'd just use CORRADE_ASSERT here.

CHECK simply outputs Warning while ASSERT interrupts system running, I believe.

Oh. But the comment says assert and I feel like it should be an assert :)

I believe CHECK will also abort if the condition is not met. Similar to CORRADE_ASSERT.

OK. I checked the google docs. Yes, you guys are right. I was wrong.
// CHECK dies with a fatal error if condition is not true.

bigbike · 2020-11-16T23:43:20Z

src/esp/assets/ResourceManager.cpp

+  const auto& info = loadedAssetData.assetInfo;
+  scene::SceneNode* newNode = nullptr;
+  if (info.type == AssetType::FRL_PTEX_MESH) {
+    ASSERT(


Suggest using CORRADE_ASSERT which is much more powerful and informative.
https://doc.magnum.graphics/corrade/Assert_8h.html

Good example is here:
https://github.com/facebookresearch/habitat-sim/blob/master/src/esp/assets/PTexMeshData.cpp

Search: CORRADE_ASSERT.

bigbike · 2020-11-16T23:50:48Z

src/esp/assets/ResourceManager.cpp

+            return false;
+          }
+        } else {
+          if (resourceDict_[filename].assetInfo != info) {


For cases that just output warning, but the system will not be interrupted, do you have similar stuffs in Magnum? @mosra

Mn::Warning{} << bla :) Behaviorally same as Error{} but can be redirected / muted independently of it.

bigbike · 2020-11-16T23:51:14Z

src/esp/assets/ResourceManager.cpp

+        }
+        // create render asset instance if requested
+        if (parent) {
+          ASSERT(creation);


CORRADE_ASSERT(...)

mosra

I feel a bit like being thrown in a cold water here -- I'm not fully aware of the progress / design / ideas around replay, so I can't really do a thorough review without first accompanying myself with everything related to this work (which I'm not sure I have capacity for, right now).

So all I can offer is random hints and suggestions re Magnum usage :) Hope that's fine.

Oh, and in addition to the comments below basically everything @bigbike says.

mosra · 2020-11-17T18:30:10Z

src/esp/assets/ResourceManager.cpp

+      .isStatic = true,
+      .isRGBD = true,
+      .isSemantic = !isSeparateSemanticScene,
+      .lightSetupKey = renderLightSetupKey};


Apologies if I'm not up-to-date on C++ standard requirements in Habitat, but designated initializers are standard only since C++20 or require a particular GNU extension, which may hurt portability (especially if we'd want to reconsider Windows support again). I'd personally stay with standard C++ with no compiler-specific extensions.

Same in other places (I spotted one or two more occurence at least).

mosra · 2020-11-17T18:32:04Z

src/esp/assets/ResourceManager.cpp

+    meshSuccess = loadRenderAssetIMesh(info);
+  } else if (isRenderAssetGeneral(info.type)) {
+    meshSuccess = loadRenderAssetGeneral(info);
+  } else {


Yes, compared to what's there now, CORRADE_INTERNAL_ASSERT_UNREACHABLE() will give the compiler a correct hint, allowing for better optimizations even in release builds.

mosra · 2020-11-17T18:33:02Z

src/esp/assets/ResourceManager.cpp

+    ASSERT(false);  // createRenderAssetInstance doesn't yet support the
+                    // requested asset type
+    newNode = nullptr;


CORRADE_INTERNAL_ASSERT_UNREACHABLE() here again, and because it marks the code as unreachable, the newNode = nullptr isn't needed to suppress "potentially uninitialized variable" warnings.

mosra · 2020-11-17T18:33:52Z

src/esp/assets/ResourceManager.cpp

+            return false;
+          }
+        } else {
+          if (resourceDict_[filename].assetInfo != info) {


Mn::Warning{} << bla :) Behaviorally same as Error{} but can be redirected / muted independently of it.

jturner65

One minor question (feel free to ignore) otherwise LGTM.

jturner65 · 2020-11-17T19:25:11Z

src/esp/assets/RenderAssetInstanceCreation.h

+namespace assets {
+
+// parameters to control how a render asset instance is created
+struct RenderAssetInstanceCreation {


Do we want to rename the file to match the struct?

Skylion007 · 2020-11-23T18:38:18Z

Pull from master to fix pre-commit

bigbike · 2020-11-30T19:46:48Z

src/esp/assets/RenderAssetInstanceCreationInfo.h

+  RenderAssetInstanceCreationInfo(
+      const std::string& _filepath,
+      const Corrade::Containers::Optional<Magnum::Vector3>& _scale,
+      bool isStatic,


Because you have defined Flags, these 3 booleans can be replaced by a single argument Flags flags, aren't they? And it will be less error-prone.
That is the purpose to have the Flags introduced.

aclegg3

LGTM

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

eundersander commented Nov 13, 2020

View reviewed changes

bigbike requested review from mosra, jturner65, bigbike and aclegg3 November 14, 2020 00:23

aclegg3 reviewed Nov 16, 2020

View reviewed changes

bigbike reviewed Nov 17, 2020

View reviewed changes

mosra reviewed Nov 17, 2020

View reviewed changes

jturner65 approved these changes Nov 17, 2020

View reviewed changes

eundersander force-pushed the eundersander/render_asset_instance branch from 0041bbf to 69eebef Compare November 22, 2020 23:45

eundersander added 5 commits November 24, 2020 19:48

refactor ResourceManager render assets

859428f

compile fix for non-PTex build

c70913b

rename to RenderAssetInstanceCreationInfo; indentation fixes

e3f024a

switch to EnumSet<Flag> in RenderAssetInstanceCreationInfo

06a90e5

change some asserts to Corrade

efcc55c

eundersander force-pushed the eundersander/render_asset_instance branch from c059881 to efcc55c Compare November 25, 2020 03:57

bigbike reviewed Nov 30, 2020

View reviewed changes

aclegg3 approved these changes Nov 30, 2020

View reviewed changes

eundersander added 2 commits November 30, 2020 12:57

refactor for RenderAssetInstanceCreationInfo flags

c7a0cbc

warning about deprecated SUNCG support

638dd13

bigbike approved these changes Nov 30, 2020

View reviewed changes

eundersander merged commit 2aac611 into facebookresearch:master Dec 1, 2020

eundersander deleted the eundersander/render_asset_instance branch December 1, 2020 03:50

Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020

refactor ResourceManager render assets (facebookresearch#935)

ab1078a

	struct RenderAssetInstanceCreation {
	struct RenderAssetInstanceCreationInfo {

refactor ResourceManager render assets #935

refactor ResourceManager render assets #935

Conversation

eundersander commented Nov 13, 2020 • edited

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

Choose a reason for hiding this comment

eundersander Nov 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skylion007 commented Nov 13, 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

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

bigbike 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

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

Choose a reason for hiding this comment

mosra 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

jturner65 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skylion007 commented Nov 23, 2020

bigbike Nov 30, 2020 • edited

Choose a reason for hiding this comment

aclegg3 left a comment

Choose a reason for hiding this comment

eundersander commented Nov 13, 2020 •

edited

eundersander Nov 13, 2020 •

edited

bigbike Nov 30, 2020 •

edited