Merge branch 'main' into zm/add-warning

filament/backend/CMakeLists.txt

@@ -540,6 +540,8 @@ if (APPLE AND NOT IOS)
 
 add_executable(metal_utils_test test/MetalTest.mm)
 
+target_compile_options(metal_utils_test PRIVATE "-fobjc-arc")
+
 target_link_libraries(metal_utils_test PRIVATE
         backend
         getopt

filament/backend/include/backend/DriverEnums.h

@@ -318,8 +318,19 @@ struct Viewport {
     int32_t right() const noexcept { return left + int32_t(width); }
     //! get the top coordinate in window space of the viewport
     int32_t top() const noexcept { return bottom + int32_t(height); }
-};
 
+    friend bool operator==(Viewport const& lhs, Viewport const& rhs) noexcept {
+        // clang can do this branchless with xor/or
+        return lhs.left == rhs.left && lhs.bottom == rhs.bottom &&
+               lhs.width == rhs.width && lhs.height == rhs.height;
+    }
+
+    friend bool operator!=(Viewport const& lhs, Viewport const& rhs) noexcept {
+        // clang is being dumb and uses branches
+        return bool(((lhs.left ^ rhs.left) | (lhs.bottom ^ rhs.bottom)) |
+                    ((lhs.width ^ rhs.width) | (lhs.height ^ rhs.height)));
+    }
+};
 
 /**
  * Specifies the mapping of the near and far clipping plane to window coordinates.

filament/backend/src/metal/MetalState.h

@@ -442,7 +442,7 @@ class MetalBufferBindings {
 
 private:
     static_assert(N <= 8);
-    std::array<__unsafe_unretained id<MTLBuffer>, N> mBuffers = { nil };
+    std::array<__weak id<MTLBuffer>, N> mBuffers = { nil };
     std::array<NSUInteger, N> mOffsets = { 0 };
     utils::bitset8 mDirtyBuffers;
     utils::bitset8 mDirtyOffsets;

filament/src/RenderPass.cpp

@@ -861,12 +861,6 @@ void RenderPass::Executor::overridePolygonOffset(backend::PolygonOffset const* p
     }
 }
 
-void RenderPass::Executor::overrideScissor(backend::Viewport const* scissor) noexcept {
-    if ((mScissorOverride = (scissor != nullptr))) { // NOLINT(*-assignment-in-if-condition)
-        mScissor = *scissor;
-    }
-}
-
 void RenderPass::Executor::overrideScissor(backend::Viewport const& scissor) noexcept {
     mScissorOverride = true;
     mScissor = scissor;
@@ -883,15 +877,15 @@ backend::Viewport RenderPass::Executor::applyScissorViewport(
     // clang vectorizes this!
     constexpr int32_t maxvali = std::numeric_limits<int32_t>::max();
     // compute new left/bottom, assume no overflow
-    int32_t l = scissor.left + scissorViewport.left;
-    int32_t b = scissor.bottom + scissorViewport.bottom;
+    int32_t const l = scissor.left + scissorViewport.left;
+    int32_t const b = scissor.bottom + scissorViewport.bottom;
     // compute right/top without overflowing, scissor.width/height guaranteed
     // to convert to int32
     int32_t r = (l > maxvali - int32_t(scissor.width)) ? maxvali : l + int32_t(scissor.width);
     int32_t t = (b > maxvali - int32_t(scissor.height)) ? maxvali : b + int32_t(scissor.height);
     // clip to the viewport
-    l = std::max(l, scissorViewport.left);
-    b = std::max(b, scissorViewport.bottom);
+    assert_invariant(l == std::max(l, scissorViewport.left));
+    assert_invariant(b == std::max(b, scissorViewport.bottom));
     r = std::min(r, scissorViewport.left + int32_t(scissorViewport.width));
     t = std::min(t, scissorViewport.bottom + int32_t(scissorViewport.height));
     assert_invariant(r >= l && t >= b);
@@ -913,9 +907,14 @@ void RenderPass::Executor::execute(FEngine& engine,
     if (first != last) {
         SYSTRACE_VALUE32("commandCount", last - first);
 
-        bool const scissorOverride = mScissorOverride;
-        if (UTILS_UNLIKELY(scissorOverride)) {
-            // initialize with scissor override
+        // The scissor rectangle is associated to a render pass, so the tracking can be local.
+        backend::Viewport currentScissor{ 0, 0, INT32_MAX, INT32_MAX };
+        bool const hasScissorOverride = mScissorOverride;
+        bool const hasScissorViewport = mHasScissorViewport;
+        if (UTILS_UNLIKELY(hasScissorViewport || hasScissorOverride)) {
+            // we should never have both an override and scissor-viewport
+            assert_invariant(!hasScissorViewport || !hasScissorOverride);
+            currentScissor = mScissor;
             driver.scissor(mScissor);
         }
 
@@ -999,17 +998,24 @@ void RenderPass::Executor::execute(FEngine& engine,
 
                 if (UTILS_UNLIKELY(mi != info.mi)) {
                     // this is always taken the first time
-                    mi = info.mi;
-                    assert_invariant(mi);
+                    assert_invariant(info.mi);
 
+                    mi = info.mi;
                     ma = mi->getMaterial();
 
-                    if (UTILS_LIKELY(!scissorOverride)) {
+                    // if we have the scissor override, the material instance and scissor-viewport
+                    // are ignored (typically used for shadow maps).
+                    if (!hasScissorOverride) {
+                        // apply the MaterialInstance scissor
                         backend::Viewport scissor = mi->getScissor();
-                        if (UTILS_UNLIKELY(mi->hasScissor())) {
-                            scissor = applyScissorViewport(mScissorViewport, scissor);
+                        if (hasScissorViewport) {
+                            // apply the scissor viewport if any
+                            scissor = applyScissorViewport(mScissor, scissor);
+                        }
+                        if (scissor != currentScissor) {
+                            currentScissor = scissor;
+                            driver.scissor(scissor);
                         }
-                        driver.scissor(scissor);
                     }
 
                     if (UTILS_LIKELY(!polygonOffsetOverride)) {
@@ -1100,16 +1106,18 @@ RenderPass::Executor::Executor(RenderPass const& pass, Command const* b, Command
           mInstancedUboHandle(pass.mInstancedUboHandle),
           mInstancedDescriptorSetHandle(pass.mInstancedDescriptorSetHandle),
           mColorPassDescriptorSet(pass.mColorPassDescriptorSet),
-          mScissorViewport(pass.mScissorViewport),
+          mScissor(pass.mScissorViewport),
           mPolygonOffsetOverride(false),
           mScissorOverride(false) {
+    mHasScissorViewport = mScissor != backend::Viewport{ 0, 0, INT32_MAX, INT32_MAX };
     assert_invariant(b >= pass.begin());
     assert_invariant(e <= pass.end());
 }
 
 RenderPass::Executor::Executor() noexcept
         : mPolygonOffsetOverride(false),
-          mScissorOverride(false) {
+          mScissorOverride(false),
+          mHasScissorViewport(false) {
 }
 
 RenderPass::Executor::Executor(Executor&& rhs) noexcept = default;

filament/src/RenderPass.h

@@ -350,12 +350,17 @@ class RenderPass {
         BufferObjectSharedHandle mInstancedUboHandle;
         DescriptorSetSharedHandle mInstancedDescriptorSetHandle;
         ColorPassDescriptorSet const* mColorPassDescriptorSet = nullptr;
-        backend::Viewport mScissorViewport;
-
-        backend::Viewport mScissor{};            // value of scissor override
-        backend::PolygonOffset mPolygonOffset{}; // value of the override
-        bool mPolygonOffsetOverride : 1;         // whether to override the polygon offset setting
-        bool mScissorOverride : 1;               // whether to override the polygon offset setting
+        // this stores either the scissor-viewport or the scissor override
+        backend::Viewport mScissor{ 0, 0, INT32_MAX, INT32_MAX };
+
+        // value of the polygon offset override
+        backend::PolygonOffset mPolygonOffset{};
+        // whether to override the polygon offset from the MaterialInstance
+        bool mPolygonOffsetOverride : 1;
+        // whether to override the scissor rectangle from the MaterialInstance
+        bool mScissorOverride : 1;
+        // whether the scissor-viewport is set
+        bool mHasScissorViewport : 1;
 
         Executor(RenderPass const& pass, Command const* b, Command const* e) noexcept;
 
@@ -382,8 +387,6 @@ class RenderPass {
         // if non-null, overrides the material's polygon offset
         void overridePolygonOffset(backend::PolygonOffset const* polygonOffset) noexcept;
 
-        // if non-null, overrides the material's scissor
-        void overrideScissor(backend::Viewport const* scissor) noexcept;
         void overrideScissor(backend::Viewport const& scissor) noexcept;
 
         void execute(FEngine& engine, const char* name) const noexcept;
@@ -420,7 +423,7 @@ class RenderPass {
             uint8_t channel, Pass pass, CustomCommand custom, uint32_t order,
             Executor::CustomCommandFn command);
 
-    static Command* resize(Arena& arena, Command* const last) noexcept;
+    static Command* resize(Arena& arena, Command* last) noexcept;
 
     // sorts commands then trims sentinels
     static Command* sortCommands(
@@ -461,7 +464,7 @@ class RenderPass {
 
     FScene::RenderableSoa const& mRenderableSoa;
     ColorPassDescriptorSet const* const mColorPassDescriptorSet;
-    backend::Viewport const mScissorViewport;
+    backend::Viewport const mScissorViewport{ 0, 0, INT32_MAX, INT32_MAX };
     Command const* /* const */ mCommandBegin = nullptr;   // Pointer to the first command
     Command const* /* const */ mCommandEnd = nullptr;     // Pointer to one past the last command
     mutable BufferObjectSharedHandle mInstancedUboHandle; // ubo for instanced primitives
@@ -509,6 +512,8 @@ class RenderPassBuilder {
         return *this;
     }
 
+    // Specifies the viewport for the scissor rectangle, that is, the final scissor rect is
+    // offset by the viewport's left-top and clipped to the viewport's width/height.
     RenderPassBuilder& scissorViewport(backend::Viewport viewport) noexcept {
         mScissorViewport = viewport;
         return *this;

filament/src/details/Renderer.cpp

@@ -1108,7 +1108,9 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
     // 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
-    passBuilder.scissorViewport(hasPostProcess ? xvp : vp);
+    // if MSAA is enabled, we end-up rendering in an intermediate buffer. This is the only case where
+    // "!hasPostProcess" doesn't guarantee rendering into the swapchain.
+    passBuilder.scissorViewport(hasPostProcess || msaaOptions.enabled ? xvp : vp);
 
     // This one doesn't need to be a FrameGraph pass because it always happens by construction
     // (i.e. it won't be culled, unless everything is culled), so no need to complexify things.