[HM3D Annotation] --HM3D SSD file support; Misc SSD Fixes by jturner65 · Pull Request #1556 · facebookresearch/habitat-sim

jturner65 · 2021-11-02T21:00:35Z

Motivation and Context

This PR adds the following functionality for SSD support :

More intelligent default file name choices for stage atttributes (not only SSD but semantic mesh and navmesh as well)
Function for setting the colormap to use for the TextureVisualizerShader, so that an external map can be used (instead of being forced to use the hard-coded Magnum turbo colormap)
A colormap is synthesized when a file is loaded based on vertex colors, to be used for Semantic Scene visualizations. The Magnum Turbo colormap is still supported as a fallback option.
HM3D SSD file (house file) load and process supported based on the current annotation file format from Appen.
More consistent naming and access for SSD files from StageAttributes.
Support parsing any Mn::Trade::Meshdata as a semantic scene mesh, not just ply files.

What's left is appropriate bindings and tests.

How Has This Been Tested

Locally C++ and Python tests.

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.

mosra

Didn't really go through everything because the colormap/objectID-generating code may need some further work.

mosra · 2021-11-05T16:44:07Z

-                 float colorMapOffset,
-                 float colorMapScale) {
+                 float colorMapOffset = -1.0f,
+                 float colorMapScale = -1.0f) {


Negative offset and scale also makes sense (for example to flip the colormap), to convey the optionality and that the values are expected to either be both set or both not, I'd use Cr::Containers::Optional<std::pair<float, float>> instead.

Hm, actually, having default values in an internal (PIMPL) API doesn't sound like a good idea. This should have no default values and the default values should be handled in the public Renderer interface instead. That way you also don't need the overload without the offset/scale parameters.

mosra · 2021-11-05T16:53:54Z

      }
-      shader->setColorMapTransformation(colorMapOffset, colorMapScale);
+      if ((colorMapOffset >= 0) && (colorMapScale >= 0)) {
+        shader->setColorMapTransformation(colorMapOffset, colorMapScale);


Hmm... You're not recreating the shader here every time you render, so if you don't set these values every time, it will stay at whatever value it was set to before, instead of the 1.0f/512.0f and 1.0f/256.0f defaults you probably expected, leading to strange different behavior depending on the order in which you switch the colormap visualization.

As we discussed :) the semantic visualization and the depth visualization each use different instances of TextureVisualizerShader and different compiled frag shader programs. Because of this (that both use cases have their own ShaderProgram and compiled frag shader) they do not "cross-pollinate" their values, so it is reasonable to assume that these values will not need to be reset on subsequent draws.

mosra · 2021-11-05T17:04:39Z

+   */
+  TextureVisualizerShader& setColorMapTexture(
+      Corrade::Containers::ArrayView<const Magnum::Vector3ub> colorMap,
+      bool wrapEdges,


Boolean function parameters are an API smell in languages that don't have the concept of named parameters. I'd expose the Mn::SamplerWrapping type directly.

I'm also not sure about the offset/scale parameters. Why not just let the caller call also setColorMapTransformation()? There the parameters are documented and the responsibility/interaction is clear, now it's a bit unclear how the two functions affect each other.

I'm also not sure about the offset/scale parameters. Why not just let the caller call also setColorMapTransformation()? There the parameters are documented and the responsibility/interaction is clear, now it's a bit unclear how the two functions affect each other.

Originally the code was set up to fully instantiate the ShaderProgram through the constructor (including setting these values) instead of having a secondary call to setColorMapTransformation for these individual values. I tried to keep my refactoring to the existing code as minimal as possible, but if you think this should be changed, i'm all for it.

mosra · 2021-11-05T17:06:12Z

   * the ObjectId texture, resulting value is then used to fetch a color from a
-   * color map bound with bindColorMapTexture(). Here we use the Magnum built-in
-   * color map: Magnum::DebugTools::ColorMap::turbo();
+   * color map bound with bindColorMapTexture(). Here we use either the Magnum


This whole comment is wrong :) Here's no bindColorMapTexture(), only rebindColorMapTexture(), and the shader is also not responsible for picking the Turbo color map anymore, that's supplied via setColorMapTexture().

Skylion007

Looks good with @mosra 's changes.

mosra · 2021-11-08T17:33:52Z

-      if (objectIdToObjectData.find(objectId) == objectIdToObjectData.end()) {
+  if (splitMesh &&
+      (parseResult->objIdsFromPly || parseResult->objPartitionsFromSSD)) {
+    std::vector<uint16_t> meshPartitionIds = parseResult->objIdsFromPly


Is this doing a copy?

not supposed to 😄 that's embarassing. Holy cow that was expensive.

This is by the way why Containers::Array was made non-copyable -- to prevent accidents like this. And writing an explicit Utility::copy() is annoying enough to make you think twice if you really need to copy :)

mosra · 2021-11-08T19:11:34Z

+      // temporary map keyed by semantic ID and value being actual color for
+      // that ID. We need this since we do not know how many, if any, unique
+      // colors that do not have Semantic Mappings exist on the mesh
+      std::unordered_map<int, Mn::Vector3ub> tmpDestSSDColorMap;


Instead of this, why not resize() the colorMapToUse to id + 1 that you're about to insert, and then colorMapToUse[id] = stuff directly? You're copying this to a linear array after anyway so this won't save anything.

id is different for each vert - can get smaller, so don't want to resize colorMapToUse smaller than it was for some previous vert. Still though, I can get rid of the next step and go directly into the colormap.

Err, I meant this:

if(colorMapToUse.size() <= id) colorMapToUse.resize(id + 1); colorMapToUse[id] = stuff;

Using a hashmap for something that's basically contiguous indexing doesn't seem right to me :)

mosra · 2021-11-08T19:17:13Z

+      std::pair<Cr::Containers::Array<Mn::UnsignedInt>, std::size_t> out =
+          Mn::MeshTools::removeDuplicatesInPlace(
+              Cr::Containers::arrayCast<2, char>(
+                  stridedArrayView(colorsThatBecomeTheColorMap)));


I still have a feeling that populating a unique color array could be done directly in the above loop, unifying the handling... :)

Sure, we could only conditionally build the tmpColorMapToSSDidAndRegionIndex if the ssd mapping exists. If it is empty, it would make a map full of "non-ssd" colors.

Using this code path for this case would be slower than what we are doing now, and less elegant than the solution using the magnum tools.

Skylion007 · 2021-11-08T21:29:40Z

This looks like the Simulator is segfaulting or failing some other way when loading the WebGL build.

eundersander

I left some comments.

eundersander · 2021-11-08T19:24:04Z

+  std::vector<uint16_t> partitionIds;
 };

 Cr::Containers::Optional<InstancePlyData> parsePly(


GenericInstanceMeshData looks to be a pretty simple container for mesh data. Would it make sense to keep this class simple and move all your annotation-parsing logic to a separate class and file?

Related: it would help if this class GenericInstanceMeshData had a class-level comment. Its parent class says "Base class for storing mesh asset data including geometry and topology" and I so wouldn't expect such a class to know e.g. the details of region indices in annotation data.

We have a long-term vision to move more of our core/generic rendering code back into the Magnum repo (e.g. PbrDrawable), so I'm interested to keep our very application-specific code (like annotation-parsing) separate.

GenericInstanceMeshData looks to be a pretty simple container for mesh data. Would it make sense to keep this class simple and move all your annotation-parsing logic to a separate class and file?

This class was always used for the semantic mesh processing - the annotation code that I added just adds flexibility for different ply formats than just the hard-coded mp3d support.

For long-term handling considerations, i see this class holding only specialized annotation information (if that - i can see a future without it too) while the general mesh data would be handled by GenericMeshData, if not Mn::Trade::MeshData directly.

Related: it would help if this class GenericInstanceMeshData had a class-level comment. Its parent class says "Base class for storing mesh asset data including geometry and topology" and I so wouldn't expect such a class to know e.g. the details of region indices in annotation data.

I can certainly more clearly comment the purpose of this class. Ultimately, from an organizational standpoint, (as I said earlier) it would probably be better if this class only held semantic-specific functionality, expanding on existing whatever MeshData structure ended up being the backbone for this class.

A little bit of redesign/refactor would pay big dividends in this department, but I felt such engineering overhead was beyond the scope of this PR.

I felt such engineering overhead was beyond the scope of this PR

I will push back a little here on the idea "refactors are out of scope". You're adding nearly 1000 lines of code in this PR. I believe we should view such large changes as opportunities to refactor, rather than piling on more tech debt to an area of code that we already regard as needing refactor.

it would probably be better if this class only held semantic-specific functionality

Consider renaming the class in this case.

I will push back a little here on the idea "refactors are out of scope". You're adding nearly 1000 lines of code in this PR. I believe we should view such large changes as opportunities to refactor, rather than piling on more tech debt to an area of code that we already regard as needing refactor.

I completely share your concerns, and normally would be in total agreement regarding addressing refactors and tech-debt whenever possible.

However, the support for the Appen annotated hm3d meshes was presented to me as being urgent, and so I approached implementation as the priority. Refactors were restricted to what was necessary to support the new functionality in as generalized a way as possible while not breaking existing code (theoretically) and addressing some of the brittleness inherent in the old designs. There's very little tech debt that's been added - the new functionality is required for hm3d annotation support, so any future reworks are going to need it on some level; some (very small amount) of tech debt has been removed, and, unfortunately a lot of debt remains.

This affected how multiple classes performed their duties:

Renderer (instance shaders on construction instead of the previous method of create shader on first visualize call, expand colormap support)

GenericMeshLoading (support multiple vert color formats, support no baked-in ids, better multi-mesh culling support)

Sim/ResourceManager (create SemanticScene descriptor in resource manager to make available during Semantic Scene mesh load)

Consider renaming the class in this case

Good idea.

jturner65 · 2021-11-09T20:21:53Z

Most recent force-push changed how shaders were loading. Loading them in the Renderer constructor (as this PR previously introduced) was not compatible with the current Javascript code protocols, so this mechanism was removed and the existing on-demand instancing is being used instead.

mosra

Two minor things + the unresolved color map map issue I asked about on Slack. Other than that looks good :)

mosra · 2021-11-12T09:35:19Z

  SHIFT-RIGHT:
    Click a mesh to highlight it.
-  CTRL-RIGHT:
+  CTRL-RIGHT:m


mosra · 2021-11-12T09:39:18Z

+
+std::vector<std::unique_ptr<GenericSemanticMeshData>>
+GenericSemanticMeshData::buildSemanticMeshData(
+    const Cr::Containers::Optional<Mn::Trade::MeshData>& srcMeshData,


No Optional here please :) The code of the function assumes the optional is non-null anyway.

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

jturner65 requested a review from aclegg3 November 2, 2021 21:00

jturner65 marked this pull request as draft November 2, 2021 21:00

jturner65 force-pushed the SSD_HM3DAndImprovements branch 5 times, most recently from 6db6032 to e771709 Compare November 4, 2021 13:13

Skylion007 reviewed Nov 4, 2021

View reviewed changes

Comment thread src/esp/scene/HM3DSemanticScene.cpp Outdated

Skylion007 suggested changes Nov 4, 2021

View reviewed changes

Comment thread src/esp/scene/HM3DSemanticScene.cpp

Comment thread src/esp/scene/HM3DSemanticScene.cpp

Comment thread src/esp/scene/HM3DSemanticScene.cpp

jturner65 requested a review from mosra November 4, 2021 22:53

jturner65 marked this pull request as ready for review November 5, 2021 07:51

jturner65 changed the title ~~[WIP][HM3D Annotation] --HM3D SSD file support; Misc SSD Fixes~~ [HM3D Annotation] --HM3D SSD file support; Misc SSD Fixes Nov 5, 2021

Skylion007 reviewed Nov 5, 2021

View reviewed changes

Comment thread src/esp/assets/GenericInstanceMeshData.cpp Outdated

mosra reviewed Nov 5, 2021

View reviewed changes

Skylion007 reviewed Nov 7, 2021

View reviewed changes

Comment thread src/esp/metadata/managers/StageAttributesManager.cpp

Skylion007 reviewed Nov 7, 2021

View reviewed changes

Comment thread src/esp/metadata/managers/StageAttributesManager.cpp

Skylion007 reviewed Nov 7, 2021

View reviewed changes

Comment thread src/esp/metadata/managers/StageAttributesManager.cpp

Skylion007 reviewed Nov 7, 2021

View reviewed changes

Comment thread src/esp/metadata/managers/StageAttributesManager.cpp

Skylion007 approved these changes Nov 7, 2021

View reviewed changes

mosra reviewed Nov 8, 2021

View reviewed changes

jturner65 force-pushed the SSD_HM3DAndImprovements branch from 8310853 to 22ea32d Compare November 8, 2021 20:23

jturner65 requested a review from eundersander November 8, 2021 21:01

jturner65 force-pushed the SSD_HM3DAndImprovements branch 2 times, most recently from 9b3bc89 to 22ea32d Compare November 8, 2021 21:27

eundersander reviewed Nov 9, 2021

View reviewed changes

jturner65 force-pushed the SSD_HM3DAndImprovements branch from 22ea32d to 0fcf7c0 Compare November 9, 2021 16:00

jturner65 added 2 commits November 9, 2021 12:57

--Improve default SSD handling; Naming Consistency; Back Compat rigor

dd7862a

--load SSD file for HM3D annotations

126874e

jturner65 added 2 commits November 9, 2021 13:45

--Expand annotated ply support; ssd-based colormap; RM load ssd

d714f87

--Support external colormap in Shader

ebaf56a

jturner65 force-pushed the SSD_HM3DAndImprovements branch from 0fcf7c0 to ebaf56a Compare November 9, 2021 20:17

jturner65 added 4 commits November 9, 2021 15:36

--rename GenericInstanceMeshData to GenericSemanticMeshData;

43affdf

--Make sure color map includes all ssd colors; bug with non-ssd map

30141c3

--Add viewer capability to query and display Semantic ID via click

030bfd4

--Improve buffer read. Yay Corrade!

96dc6e1

jturner65 force-pushed the SSD_HM3DAndImprovements branch from ab9b613 to 96dc6e1 Compare November 11, 2021 15:32

jturner65 added 2 commits November 11, 2021 13:36

--more rigor for colormap construction

93f47c1

--refactor to handle any source meshdatas and remove superfluous code.

da67f3c

mosra reviewed Nov 12, 2021

View reviewed changes

jturner65 added 2 commits November 12, 2021 06:48

--improve colormap construction

71fcf8e

--reviewer suggestion

8812142

jturner65 force-pushed the SSD_HM3DAndImprovements branch from 8c4b416 to 8812142 Compare November 12, 2021 11:58

jturner65 merged commit cd270dc into facebookresearch:main Nov 12, 2021

jturner65 deleted the SSD_HM3DAndImprovements branch November 12, 2021 12:50

Conversation

jturner65 commented Nov 2, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mosra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

mosra Nov 5, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Skylion007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jturner65 Nov 8, 2021 • 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!

jturner65 Nov 8, 2021 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skylion007 commented Nov 8, 2021

Uh oh!

eundersander 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!

jturner65 Nov 9, 2021 • 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!

eundersander Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jturner65 commented Nov 2, 2021 •

edited

Loading

mosra Nov 5, 2021 •

edited

Loading

jturner65 Nov 8, 2021 •

edited

Loading

jturner65 Nov 8, 2021 •

edited

Loading

jturner65 Nov 9, 2021 •

edited

Loading

eundersander Nov 9, 2021 •

edited

Loading

jturner65 commented Nov 9, 2021 •

edited

Loading