Multi-mesh, multi-texture and multi-file support in the batch renderer #1874

mosra · 2022-09-28T21:28:28Z

Motivation and Context

With all GltfSceneConverter features needed by this code now finalized and upstreamed, I can finally publish this update. The glTF conversion being fully upstreamed also means that generating test data as well as writing code that processes several files into a single batch-optimized composite glTF file is possible directly using Habitat, without having to pull from some arbitrary Magnum branch.

What the batch renderer does compared to the initial prototype from #1798 from end of June:

It's now possible to load files that contain multiple meshes. The meshes can be completely arbitrary, with different vertex layout, even different primitives (such as lines!).
It's now possible to load files that contain multiple textures. Again, the textures can be each completely arbitrary, different formats, sizes and even mixing up 2D and 2D array textures.
And, as a consequence, it's now possible to load multiple files as well -- which are internally treated as if just one giant file with multiple meshes and textures was added.

This ultimately could mean that one can now feed the plain old unoptimized single-mesh glTFs and it will work, making this thing slightly closer to being a replacement for the current renderer, including support for slow and inefficient rendering if desired. Which is what I want to confirm next, by attempting to load a scene replay JSON, and so the PR is a draft. TODOs for this PR so far:

How Has This Been Tested

~~With sweat and tears.~~

The original test setup remains, it's just extended with a few variants of the input glTF file -- one that has multiple meshes instead of one, one that has multiple 2D textures instead of one 2D array, and the data being split across two files. That's where the majority of the changes comes from -- the glTF files and the code needed to generate them. The actually useful code is just about 1000 lines in Renderer.cpp, with another 1000 for documentation in the headers, and then some more tens of thousands in Magnum itself.

eundersander

I posted some questions and comments, but overall LGTM!

eundersander · 2022-09-30T12:48:16Z

src/esp/gfx_batch/Renderer.cpp

@@ -240,89 +291,131 @@ void Renderer::addFile(const Cr::Containers::StringView filename,
  //  for glb, bps and ply?)
  CORRADE_INTERNAL_ASSERT_OUTPUT(importer->openFile(filename));


Is this assert checking if the import succeeded? And catching a malformed GLTF file?

So this is a runtime check? Is a Corrade assert the right way to catch this? I'm used to thinking of asserts as checks on code correctness and which can be compiled out in release builds. See also Habitat's ESP_CHECK for reference.

Yeah, not great at all, and I have to fix it as it asserts on me quite often :D Should be something more graceful.

I'm thinking returning a bool here and then asserting / ESP_CHECK()ing when you forget to check the failure and attempt to add a non-existent name (from that failed-to-load file, for example).

Done, the function now returns a bool and all non-programmer errors (i.e., caused by corrupted files or unexpected data) are now runtime failures instead of assertions

Note that I'm not using Habitat's logging APIs in order to avoid linking to core, based on our discussion in the previous PR.

eundersander · 2022-09-30T13:01:17Z

src/esp/gfx_batch/Renderer.cpp

    for (Mn::UnsignedLong root : scene->childrenFor(-1)) {
      Cr::Containers::Array<Mn::UnsignedLong> children =
          scene->childrenFor(root);

      Cr::Containers::String name = importer->objectName(root);
      CORRADE_ASSERT(name,
                     "Renderer::addFile(): node" << root << "has no name", );
-      state_->meshViewRangeForName.insert(
-          {name, {offset, offset + Mn::UnsignedInt(children.size())}});
+      CORRADE_ASSERT_OUTPUT(


This seems like a runtime check that might fail due to bad data. Same question here about runtime check versus assert.

Hm, for this one I actually got an idea -- would it make sense that if you addFile() that contains identifiers/names that are already present from files added earlier, it replaces them? Instead of failing/asserting.

Let's say, for iterative workflows where you have some simulation running already and just need to figure the best scale/color/whatever for a newly added object. Or for lights. There it might make sense that you repeatedly addFile() and addMeshHierarchy() until you're happy. The consequence is of course that you gradually fill GPU memory with unused meshes, but might be an okay tradeoff compared to having to suffer the delay of shutting down and restarting everything. Or it could be an opt-in flag in the config, to allow this, and be strict by default.

Thoughts?

Yeah, hard to guess what users want here, but opt-in flag with strict by default sounds good.

Done as well, this is now a runtime error and the function will return false in that case.

To avoid too many unused features creeping in, I didn't add the opt-in flag to replace existing names. There's a TODO and I will implement it once an actual use case appears.

eundersander · 2022-09-30T13:23:42Z

src/esp/gfx_batch/Renderer.cpp

-                              .slice(&DrawCommand::indexOffsetInBytes));
+            scene.textureTransformationUniform);
+
+      /* Submit all draw batches */


When we talk about a "batch renderer", we generally mean batching things (meshes) into fewer draw calls. I guess I always assumed we'd do batching across scenes. E.g. all the usage of ycb_composite.gltf, namely all instances of all YCB objects, across all scenes, could be one draw call. But it looks like we don't do any batching across scenes here, correct? Why not?

Across all scenes not really due to scissoring, otherwise objects from one scene could leak to the other. (BPS did the same IIRC.)

But I have a bunch of ideas that could circumvent this limitation (custom clip planes, multi-view / multi-viewport rendering), just need to try them out ;)

mosra · 2022-10-04T19:16:06Z

Last push contains all changes that were needed to support gfx-replay from raw unoptimized glTF files (what I showed on Slack last Friday) -- in particular files that are treated as a whole (instead of as composite), texture-less materials and scene-less files (such as STLs). Next I'm going to resolve the above comments and write some initial documentation for the interface, because it now really needs documenting.

Further feedback welcome, although with the high-level docs missing the code is probably too cryptic :)

Keeping the PR as a draft for now as it contains a dirty attempt to fix CodeCov reports.

mosra · 2022-10-06T10:53:37Z

@Skylion007 I'm getting this Clang Tidy complaint:

/home/circleci/project/habitat-sim/src/esp/gfx_batch/Renderer.cpp:494:8: error: implicit conversion 
'Magnum::UnsignedInt' (aka 'unsigned int') -> bool [readability-implicit-bool-conversion,-warnings-as-errors]
  if (!importer->sceneCount()) {
      ~^
                              == 0u

Do you think it's useful? Because I don't, at all, and so I think I'd just disable this check altogether. Being forced to write == 0 doesn't improve readability in any way in my opinion.

Skylion007 · 2022-10-06T19:59:31Z

@Skylion007 I'm getting this Clang Tidy complaint:
/home/circleci/project/habitat-sim/src/esp/gfx_batch/Renderer.cpp:494:8: error: implicit conversion 
'Magnum::UnsignedInt' (aka 'unsigned int') -> bool [readability-implicit-bool-conversion,-warnings-as-errors]
  if (!importer->sceneCount()) {
      ~^
                              == 0u
Do you think it's useful? Because I don't, at all, and so I think I'd just disable this check altogether. Being forced to write == 0 doesn't improve readability in any way in my opinion.

It's useful in PyBind11 for finding C "bool"s that actually should have error handling on the CPython API. Probably not as useful in Habitat-Sim.

mosra · 2022-10-06T20:15:43Z

Last commit contains high-level docs for the batch renderer, mainly interesting for @0mdc. You can read the documentation source here, rendered output including links to relevant Magnum APIs and colored console output will appear here once merged.

Oh, and Clang-Format is making a mess from the doc blocks, line wraps done in the middle of Doxygen @ref and @section commands are very often causing it to misbehave (and the breakages are hard to detect), so I'd rather not comply with what it suggests. @Skylion007 any ideas, apart from wrapping the whole block in // clang-format off? 😅

Probably not as useful in Habitat-Sim.

Am I allowed to disable that check? :)

Skylion007 · 2022-10-06T20:16:59Z

@mosra Yeah, feel free to.

Skylion007 · 2022-10-06T20:18:51Z

@mosra You can set a comment Pragma in the clang-format setting. If it sees a comment with a special token in it, it won't format it.

mosra · 2022-10-06T20:26:53Z

Ah, nice. So would it be fine if I add @ref and @section there? That would cover most of the doc blocks, though... but maybe that's what we actually want after all.

mosra · 2022-10-06T20:47:03Z

Hm, the comment pragma fixed it for most of the cases but not all. Strange, is some line wrapping excluded from the pragmas? (The code block containing the suggested clang-format change contains @section above so it should be excluded as a whole if I understand correctly.)

~~Could I do anything else apart from using my tweeting skills and shortening the wording in the heading until it fits? :D~~ I lost patience and just // clang-format off'd the whole block. Argh.

It gets fully overwritten each frame, so there's no need to keep it around. The single-upload TODO still holds, although I'm not sure about the benefits anymore.

This significantly reduces the amount of data passed to the GPU each frame, at the cost of a more complex node transformation hierarchy processing. But that will eventually get moved to Magnum itself and heavily SIMDified.

This also prepares for additional operations that are too expensive to do after every add, like sorting by mesh ID.

Using a *.gltf file for self-documenting purposes, but bundling the KTX image inside the *.bin to reduce the amount of test files used.

Gradually regrouping / redoing the test files to avoid having too many of them.

And make them available under their filename, instead of using node names.

Currently just for vertex colors, but makes room for further expansion.

Yay, wouldn't have discovered this bug without all those regression tests.

Importing the first mesh from these and assigning a default material to it.

It's an often-used pattern, actually.

Just so we're sure they're implemented.

Off you go.

Using Magnum's error output facilities instead of Habitat's logging to avoid dependency on the core library, as discussed in the previous PR.

mosra · 2022-10-08T10:55:23Z

Assuming the CIs don't start failing again, this PR is done from my side, with pending TODOs either implemented or postponed (see the list in the PR description).

Awaiting your feedback on the high-level documentation. In case you don't have any, I propose merging this so the gfx-replay integration can be branched off these changes.

Skylion007 · 2022-10-11T19:00:43Z

Ah, nice. So would it be fine if I add @ref and @section there? That would cover most of the doc blocks, though... but maybe that's what we actually want after all.

For future reference, here is a more aggressive hack: https://stackoverflow.com/a/51913607/2444240

mosra · 2022-10-12T08:17:09Z

I ended up using the most aggressive variant in certain places, // clang-format off ;)

mosra requested review from aclegg3, eundersander and 0mdc September 28, 2022 21:28

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 28, 2022

mosra requested a review from Skylion007 September 28, 2022 21:38

Skylion007 approved these changes Sep 29, 2022

View reviewed changes

mosra mentioned this pull request Sep 29, 2022

Update Magnum submodules with support for querying MeshData size from Python #1878

Merged

mosra force-pushed the batch-renderer-multi branch from 8046b1d to 6bb02c3 Compare September 30, 2022 10:14

eundersander approved these changes Sep 30, 2022

View reviewed changes

0mdc approved these changes Oct 4, 2022

View reviewed changes

mosra force-pushed the batch-renderer-multi branch from 3279e98 to 1cdebce Compare October 4, 2022 19:19

mosra mentioned this pull request Oct 5, 2022

Update Doxygen tagfiles and Python inventories for Corrade/Magnum doc crosslinking #1889

Merged

mosra mentioned this pull request Oct 6, 2022

Better colors for "info" and "success" notes in the documentation and website #1893

Merged

mosra force-pushed the batch-renderer-multi branch 2 times, most recently from ef78fc5 to fd99dc9 Compare October 6, 2022 19:57

mosra force-pushed the batch-renderer-multi branch from fd99dc9 to 0c325ed Compare October 6, 2022 20:04

mosra force-pushed the batch-renderer-multi branch from 025b32a to ab361b9 Compare October 6, 2022 20:40

mosra added 5 commits October 7, 2022 17:07

Use a single temporary buffer calculating absolute transformations.

f6bd6e7

It gets fully overwritten each frame, so there's no need to keep it around. The single-upload TODO still holds, although I'm not sure about the benefits anymore.

Mask out transformations that aren't used for any draws.

8aa499c

This significantly reduces the amount of data passed to the GPU each frame, at the cost of a more complex node transformation hierarchy processing. But that will eventually get moved to Magnum itself and heavily SIMDified.

Upload per-draw uniforms at most once per frame, not after every add().

880a712

This also prepares for additional operations that are too expensive to do after every add, like sorting by mesh ID.

Update Magnum Find modules.

977347b

Adapt to fixes in the finalized glTF exporter, bundle images.

1f100bb

Using a *.gltf file for self-documenting purposes, but bundling the KTX image inside the *.bin to reduce the amount of test files used.

mosra added 12 commits October 7, 2022 17:07

Support texture-less materials as well.

5fb4a32

Gradually regrouping / redoing the test files to avoid having too many of them.

Ability to treat files as a whole.

7dee07c

And make them available under their filename, instead of using node names.

Show filename in asserts.

a017429

Support different shaders per batch.

0454c99

Currently just for vertex colors, but makes room for further expansion.

Don't add mesh offset to mesh ID more than once.

753bee5

Yay, wouldn't have discovered this bug without all those regression tests.

Support scene-less files as well.

06d1cdc

Importing the first mesh from these and assigning a default material to it.

Support non-indexed meshes as well.

4a1154a

Hello Clang Format, thanks for ruining the diffs.

dd544d0

High-level docs for the batch renderer.

953ca43

Clang Tidy: don't error on implicit bool conversion.

da28827

It's an often-used pattern, actually.

Exclude doc blocks with problematic Doxygen commands from autowrapping.

094f988

Update Magnum submodules with fixes for plugin warning output.

5443581

mosra force-pushed the batch-renderer-multi branch from ab361b9 to 6b76543 Compare October 7, 2022 17:56

mosra added 4 commits October 7, 2022 19:58

Test also the compressed texture code path in the batch renderer.

0810fba

Test also the flag getters.

e538fe7

Just so we're sure they're implemented.

Exclude unreachable line from code coverage.

5b2b860

Clang Format, you've been given enough chances to behave.

3a61032

Off you go.

mosra force-pushed the batch-renderer-multi branch from 6b76543 to 3a61032 Compare October 7, 2022 18:01

mosra added 2 commits October 8, 2022 12:19

Explicitly test and fix for material-less files.

5d065de

addFile() now returns a bool instead of asserting.

7593ca9

Using Magnum's error output facilities instead of Habitat's logging to avoid dependency on the core library, as discussed in the previous PR.

mosra marked this pull request as ready for review October 8, 2022 10:45

mosra added 3 commits October 8, 2022 12:50

Ability to query if a particular mesh hierarchy exists.

3e9771b

TODOs.

7445195

Clang Format, good afternoon to you as well.

cc1d29f

mosra force-pushed the batch-renderer-multi branch from 1a4a145 to cc1d29f Compare October 8, 2022 10:52

mosra merged commit 88dc7f7 into main Oct 14, 2022

mosra deleted the batch-renderer-multi branch October 14, 2022 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-mesh, multi-texture and multi-file support in the batch renderer #1874

Multi-mesh, multi-texture and multi-file support in the batch renderer #1874

mosra commented Sep 28, 2022 •

edited

Loading

eundersander left a comment

eundersander Sep 30, 2022 •

edited

Loading

mosra Sep 30, 2022

mosra Oct 8, 2022

eundersander Sep 30, 2022 •

edited

Loading

mosra Sep 30, 2022

eundersander Oct 3, 2022

mosra Oct 8, 2022

eundersander Sep 30, 2022

mosra Sep 30, 2022

mosra commented Oct 4, 2022

mosra commented Oct 6, 2022 •

edited

Loading

Skylion007 commented Oct 6, 2022

mosra commented Oct 6, 2022

Skylion007 commented Oct 6, 2022

Skylion007 commented Oct 6, 2022

mosra commented Oct 6, 2022

mosra commented Oct 6, 2022 •

edited

Loading

mosra commented Oct 8, 2022 •

edited

Loading

Skylion007 commented Oct 11, 2022

mosra commented Oct 12, 2022

		@@ -240,89 +291,131 @@ void Renderer::addFile(const Cr::Containers::StringView filename,
		// for glb, bps and ply?)
		CORRADE_INTERNAL_ASSERT_OUTPUT(importer->openFile(filename));

Multi-mesh, multi-texture and multi-file support in the batch renderer #1874

Multi-mesh, multi-texture and multi-file support in the batch renderer #1874

Conversation

mosra commented Sep 28, 2022 • edited Loading

Motivation and Context

How Has This Been Tested

eundersander left a comment

Choose a reason for hiding this comment

eundersander Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

mosra Sep 30, 2022

Choose a reason for hiding this comment

mosra Oct 8, 2022

Choose a reason for hiding this comment

eundersander Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

mosra Sep 30, 2022

Choose a reason for hiding this comment

eundersander Oct 3, 2022

Choose a reason for hiding this comment

mosra Oct 8, 2022

Choose a reason for hiding this comment

eundersander Sep 30, 2022

Choose a reason for hiding this comment

mosra Sep 30, 2022

Choose a reason for hiding this comment

mosra commented Oct 4, 2022

mosra commented Oct 6, 2022 • edited Loading

Skylion007 commented Oct 6, 2022

mosra commented Oct 6, 2022

Skylion007 commented Oct 6, 2022

Skylion007 commented Oct 6, 2022

mosra commented Oct 6, 2022

mosra commented Oct 6, 2022 • edited Loading

mosra commented Oct 8, 2022 • edited Loading

Skylion007 commented Oct 11, 2022

mosra commented Oct 12, 2022

mosra commented Sep 28, 2022 •

edited

Loading

eundersander Sep 30, 2022 •

edited

Loading

eundersander Sep 30, 2022 •

edited

Loading

mosra commented Oct 6, 2022 •

edited

Loading

mosra commented Oct 6, 2022 •

edited

Loading

mosra commented Oct 8, 2022 •

edited

Loading