fix a couple of subtle bugs related to custom commands

- we must sort commmands *after* we have added all commands! - custom commands could change the UBO/Sampler bindings so we need to make sure to invalidate them after executing the command.
google · May 24, 2023 · 2806221 · 2806221
1 parent a976337
commit 2806221
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 36 deletions.
diff --git a/filament/src/RenderPass.cpp b/filament/src/RenderPass.cpp
@@ -764,6 +764,7 @@ void RenderPass::Executor::execute(backend::DriverApi& driver,
              */
 
             if (UTILS_UNLIKELY((first->key & CUSTOM_MASK) != uint64_t(CustomCommand::PASS))) {
+                mi = nullptr; // custom command could change the currently bound MaterialInstance
                 uint32_t const index = (first->key & CUSTOM_INDEX_MASK) >> CUSTOM_INDEX_SHIFT;
                 assert_invariant(index < mCustomCommands.size());
                 pCustomCommands[index]();

diff --git a/filament/src/details/Renderer.cpp b/filament/src/details/Renderer.cpp
@@ -863,8 +863,40 @@ void FRenderer::renderJob(ArenaScope& arena, FView& view) {
     // (i.e. it won't be culled, unless everything is culled), so no need to complexify things.
     pass.setVariant(variant);
     pass.appendCommands(engine, RenderPass::COLOR);
+
+    // color-grading as subpass is done either by the color pass or the TAA pass if any
+    auto colorGradingConfigForColor = colorGradingConfig;
+    colorGradingConfigForColor.asSubpass = colorGradingConfigForColor.asSubpass && !taaOptions.enabled;
+
+    if (colorGradingConfigForColor.asSubpass) {
+        // append color grading subpass after all other passes
+        pass.appendCustomCommand(3,
+                RenderPass::Pass::BLENDED,
+                RenderPass::CustomCommand::EPILOG,
+                0, [&ppm, &driver, colorGradingConfigForColor]() {
+                    ppm.colorGradingSubpass(driver, colorGradingConfigForColor);
+                });
+    } else if (colorGradingConfig.customResolve) {
+        // append custom resolve subpass after all other passes
+        pass.appendCustomCommand(3,
+                RenderPass::Pass::BLENDED,
+                RenderPass::CustomCommand::EPILOG,
+                0, [&ppm, &driver]() {
+                    ppm.customResolveSubpass(driver);
+                });
+    }
+
+    // sort commands once we're done adding commands
     pass.sortCommands(engine);
 
+
+    // this makes the viewport relative to xvp
+    // FIXME: we should use 'vp' when rendering directly into the swapchain, but that's hard to
+    //        know at this point. This will usually be the case when post-process is disabled.
+    // FIXME: we probably should take the dynamic scaling into account too
+    pass.setScissorViewport(hasPostProcess ? xvp : vp);
+
+
     FrameGraphTexture::Descriptor const desc = {
             .width = config.physicalViewport.width,
             .height = config.physicalViewport.height,
@@ -894,41 +926,6 @@ void FRenderer::renderJob(ArenaScope& arena, FView& view) {
             }
     );
 
-    // color-grading as subpass is done either by the color pass or the TAA pass if any
-    auto colorGradingConfigForColor = colorGradingConfig;
-    colorGradingConfigForColor.asSubpass = colorGradingConfigForColor.asSubpass && !taaOptions.enabled;
-
-    if (colorGradingConfigForColor.asSubpass) {
-        // append color grading subpass after all other passes
-        pass.appendCustomCommand(3,
-                RenderPass::Pass::BLENDED,
-                RenderPass::CustomCommand::EPILOG,
-                0, [&ppm, &driver, colorGradingConfigForColor]() {
-                    ppm.colorGradingSubpass(driver, colorGradingConfigForColor);
-                });
-    }
-
-    if (colorGradingConfig.customResolve) {
-        // append custom resolve subpass after all other passes
-        pass.appendCustomCommand(3,
-                RenderPass::Pass::BLENDED,
-                RenderPass::CustomCommand::EPILOG,
-                0, [&ppm, &driver]() {
-                    ppm.customResolveSubpass(driver);
-                });
-    }
-
-    // FIXME: sorting should happen AFTER the appendCustomCommands
-    // FIXME: concern that Material::prepare() is not called
-    // FIXME: concern that calling mi->use() will mess the renering loop
-
-
-    // this makes the viewport relative to xvp
-    // FIXME: we should use 'vp' when rendering directly into the swapchain, but that's hard to
-    //        know at this point. This will usually be the case when post-process is disabled.
-    // FIXME: we probably should take the dynamic scaling into account too
-    pass.setScissorViewport(hasPostProcess ? xvp : vp);
-
     // the color pass itself + color-grading as subpass if needed
     auto colorPassOutput = RendererUtils::colorPass(fg, "Color Pass", mEngine, view,
             desc, config, colorGradingConfigForColor, pass.getExecutor());
@@ -943,7 +940,7 @@ void FRenderer::renderJob(ArenaScope& arena, FView& view) {
     }
 
     if (colorGradingConfig.customResolve) {
-        // TODO: we have to "uncompress" (i.e. detonemap) the color buffer here because it's  used
+        // TODO: we have to "uncompress" (i.e. detonemap) the color buffer here because it's used
         //       by many other passes (Bloom, TAA, DoF, etc...). We could make this more
         //       efficient by using ARM_shader_framebuffer_fetch. We use a load/store (i.e.
         //       subpass) here because it's more convenient.