Which way is up?

Clean up and reorganize the code dealing with different conventions for Y direction. Diffs= d8d42c0f13 Which way is up? (#8911) Co-authored-by: Chris Dalton <[email protected]>
rive-app · Jan 24, 2025 · 7e50399 · 7e50399
1 parent 2653e77
commit 7e50399
Show file tree

Hide file tree

Showing 18 changed files with 155 additions and 128 deletions.
diff --git a/.rive_head b/.rive_head
@@ -1 +1 @@
-1b51fe394e81445b8e6c990780ed104ee582fde4
+d8d42c0f130ec1f7d70a9e4ca3cffb1f6e3b9860
diff --git a/renderer/include/rive/renderer/gpu.hpp b/renderer/include/rive/renderer/gpu.hpp
@@ -109,15 +109,58 @@ struct PlatformFeatures
     // Required for @ENABLE_CLIP_RECT in msaa mode.
     bool supportsClipPlanes = false;
     bool avoidFlatVaryings = false;
-    // Invert Y when drawing to offscreen render targets? (Gradient and
-    // tessellation textures.)
-    bool invertOffscreenY = false;
-    // Specifies whether the graphics layer appends a negation of Y to on-screen
-    // vertex shaders that needs to be undone.
-    bool uninvertOnScreenY = false;
-    // Does the built-in pixel coordinate in the fragment shader go bottom-up or
-    // top-down?
-    bool fragCoordBottomUp = false;
+    // clipSpaceBottomUp specifies whether the top of the viewport, in clip
+    // coordinates, is at Y=+1 (OpenGL, Metal, D3D, WebGPU) or Y=-1 (Vulkan).
+    //
+    // framebufferBottomUp specifies whether "row 0" of the framebuffer is the
+    // bottom of the image (OpenGL) or the top (Metal, D3D, WebGPU, Vulkan).
+    //
+    //
+    //                                OpenGL
+    //           (clipSpaceBottomUp=true, framebufferBottomUp=true)
+    //
+    //  Rive Pixel Space             Clip Space              Framebuffer
+    //
+    //  0 ----------->                   ^ +1                ^ height
+    //  |          width                 |                   |
+    //  |                         -1     |     +1            |
+    //  |                 ===>    <------|------>    ===>    |
+    //  |                                |                   |
+    //  |                                |                   |          width
+    //  v height                         v -1                0 ----------->
+    //
+    //
+    //
+    //                            Metal/D3D/WebGPU
+    //           (clipSpaceBottomUp=true, framebufferBottomUp=false)
+    //
+    //  Rive Pixel Space             Clip Space              Framebuffer
+    //
+    //  0 ----------->                   ^ +1                0 ----------->
+    //  |          width                 |                   |          width
+    //  |                         -1     |     +1            |
+    //  |                 ===>    <------|------>    ===>    |
+    //  |                                |                   |
+    //  |                                |                   |
+    //  v height                         v -1                v height
+    //
+    //
+    //
+    //                                Vulkan
+    //          (clipSpaceBottomUp=false, framebufferBottomUp=false)
+    //
+    //  Rive Pixel Space             Clip Space              Framebuffer
+    //
+    //  0 ----------->                   ^ -1                0 ----------->
+    //  |          width                 |                   |          width
+    //  |                         -1     |     +1            |
+    //  |                 ===>    <------|------>    ===>    |
+    //  |                                |                   |
+    //  |                                |                   |
+    //  v height                         v +1                v height
+    //
+    bool clipSpaceBottomUp = false;
+    bool framebufferBottomUp = false;
     // Backend cannot initialize PLS with typical clear/load APIs in atomic
     // mode. Issue a "DrawType::atomicInitialize" draw instead.
     bool atomicPLSMustBeInitializedAsDraw = false;

diff --git a/renderer/include/rive/renderer/webgpu/render_context_webgpu_impl.hpp b/renderer/include/rive/renderer/webgpu/render_context_webgpu_impl.hpp
@@ -69,13 +69,17 @@ class RenderContextWebGPUImpl : public RenderContextHelperImpl
     {
         PixelLocalStorageType plsType = PixelLocalStorageType::none;
         bool disableStorageBuffers = false;
+        // Invert Y when drawing to client-provided RenderTargets.
+        // TODO: We may need to eventually make this configurable
+        // per-RenderTarget.
+        bool invertRenderTargetY = false;
+        // Invert the front face when drawing to client-provied RenderTargets.
+        bool invertRenderTargetFrontFace = false;
     };
 
-    static std::unique_ptr<RenderContext> MakeContext(
-        wgpu::Device,
-        wgpu::Queue,
-        const ContextOptions&,
-        const gpu::PlatformFeatures& baselinePlatformFeatures = {});
+    static std::unique_ptr<RenderContext> MakeContext(wgpu::Device,
+                                                      wgpu::Queue,
+                                                      const ContextOptions&);
 
     virtual ~RenderContextWebGPUImpl();
 
@@ -93,11 +97,9 @@ class RenderContextWebGPUImpl : public RenderContextHelperImpl
                                   const uint8_t imageDataRGBA[]) override;
 
 protected:
-    RenderContextWebGPUImpl(
-        wgpu::Device device,
-        wgpu::Queue queue,
-        const ContextOptions&,
-        const gpu::PlatformFeatures& baselinePlatformFeatures);
+    RenderContextWebGPUImpl(wgpu::Device device,
+                            wgpu::Queue queue,
+                            const ContextOptions&);
 
     // Create the BindGroupLayout that binds the PLS attachments as textures.
     // This is not necessary on all implementations.
@@ -124,9 +126,11 @@ class RenderContextWebGPUImpl : public RenderContextHelperImpl
         EmJsHandle* renderPassJSHandleIfNeeded);
 
     wgpu::Device device() const { return m_device; }
-    wgpu::FrontFace frontFaceForOnScreenDraws() const
+    wgpu::FrontFace frontFaceForRenderTargetDraws() const
     {
-        return m_frontFaceForOnScreenDraws;
+        return m_contextOptions.invertRenderTargetFrontFace
+                   ? wgpu::FrontFace::CCW
+                   : wgpu::FrontFace::CW;
     }
     wgpu::PipelineLayout drawPipelineLayout() const
     {
@@ -160,15 +164,6 @@ class RenderContextWebGPUImpl : public RenderContextHelperImpl
     const wgpu::Queue m_queue;
     const ContextOptions m_contextOptions;
 
-    // PLS always expects CW, but when using "glsl-raw" shaders, we may need to
-    // use CCW. This is because the WebGPU layer might actually flip our
-    // frontFace if it anticipates negating our Y coordinate in the vertex
-    // shader. But when we use raw-glsl shaders, the WebGPU layer doesn't
-    // actually get a chance to negate Y like it thinks it will. Therefore, we
-    // emit the wrong frontFace, in anticipation of it getting flipped into the
-    // correct frontFace on its way to the driver.
-    wgpu::FrontFace m_frontFaceForOnScreenDraws;
-
     // Draws emulated render-pass load/store actions for
     // EXT_shader_pixel_local_storage.
     class LoadStoreEXTPipeline;

diff --git a/renderer/src/d3d/render_context_d3d_impl.cpp b/renderer/src/d3d/render_context_d3d_impl.cpp
@@ -163,7 +163,8 @@ RenderContextD3DImpl::RenderContextD3DImpl(
     m_gpu(std::move(gpu)),
     m_gpuContext(std::move(gpuContext))
 {
-    m_platformFeatures.invertOffscreenY = true;
+    m_platformFeatures.clipSpaceBottomUp = true;
+    m_platformFeatures.framebufferBottomUp = false;
     m_platformFeatures.supportsRasterOrdering =
         d3dCapabilities.supportsRasterizerOrderedViews;
     m_platformFeatures.supportsFragmentShaderAtomics = true;

diff --git a/renderer/src/gl/render_context_gl_impl.cpp b/renderer/src/gl/render_context_gl_impl.cpp
@@ -73,7 +73,8 @@ RenderContextGLImpl::RenderContextGLImpl(
         // (5-10%) improvement from not using flat varyings.
         m_platformFeatures.avoidFlatVaryings = true;
     }
-    m_platformFeatures.fragCoordBottomUp = true;
+    m_platformFeatures.clipSpaceBottomUp = true;
+    m_platformFeatures.framebufferBottomUp = true;
 
     std::vector<const char*> generalDefines;
     if (!m_capabilities.ARB_shader_storage_buffer_object)
@@ -685,6 +686,8 @@ RenderContextGLImpl::DrawShader::DrawShader(
     {
         defines.push_back(GLSL_RENDER_MODE_MSAA);
     }
+    assert(renderContextImpl->platformFeatures().framebufferBottomUp);
+    defines.push_back(GLSL_FRAMEBUFFER_BOTTOM_UP);
 
     std::vector<const char*> sources;
     sources.push_back(glsl::constants);

diff --git a/renderer/src/gpu.cpp b/renderer/src/gpu.cpp
@@ -430,11 +430,20 @@ FlushUniforms::InverseViewports::InverseViewports(
     const PlatformFeatures& platformFeatures)
 {
     float4 numerators = 2;
-    if (platformFeatures.invertOffscreenY)
+    // When rendering to the gradient and tessellation textures, ensure that
+    // row 0 in input coordinates gets written to row 0 in texture memory.
+    // This requires a Y inversion if clip space and framebuffer space have
+    // opposing senses of which way is up.
+    if (platformFeatures.clipSpaceBottomUp !=
+        platformFeatures.framebufferBottomUp)
     {
         numerators.xy = -numerators.xy;
     }
-    if (platformFeatures.uninvertOnScreenY)
+    // When drawing to a render target, ensure that Y=0 (in Rive pixel space)
+    // gets drawn to the top of thew viewport.
+    // This requires a Y inversion if Rive pixel space and clip space have
+    // opposing senses of which way is up.
+    if (platformFeatures.clipSpaceBottomUp)
     {
         numerators.w = -numerators.w;
     }
@@ -570,7 +579,7 @@ void PaintAuxData::set(const Mat2D& viewMatrix,
         {
             Mat2D paintMatrix;
             viewMatrix.invert(&paintMatrix);
-            if (platformFeatures.fragCoordBottomUp)
+            if (platformFeatures.framebufferBottomUp)
             {
                 // Flip _fragCoord.y.
                 paintMatrix =
@@ -646,7 +655,7 @@ void PaintAuxData::set(const Mat2D& viewMatrix,
     if (clipRectInverseMatrix != nullptr)
     {
         Mat2D m = clipRectInverseMatrix->inverseMatrix();
-        if (platformFeatures.fragCoordBottomUp)
+        if (platformFeatures.framebufferBottomUp)
         {
             // Flip _fragCoord.y.
             m = m * Mat2D(1, 0, 0, -1, 0, renderTarget->height());

diff --git a/renderer/src/metal/render_context_metal_impl.mm b/renderer/src/metal/render_context_metal_impl.mm
@@ -343,7 +343,8 @@ void onUnmapAndSubmitBuffer(int bufferIdx, size_t mapSizeInBytes) override
     // It appears, so far, that we don't need to use flat interpolation for path
     // IDs on any Apple device, and it's faster not to.
     m_platformFeatures.avoidFlatVaryings = true;
-    m_platformFeatures.invertOffscreenY = true;
+    m_platformFeatures.clipSpaceBottomUp = true;
+    m_platformFeatures.framebufferBottomUp = false;
 #if defined(RIVE_IOS) || defined(RIVE_XROS) || defined(RIVE_APPLETVOS)
     m_platformFeatures.supportsRasterOrdering = true;
     m_platformFeatures.supportsFragmentShaderAtomics = false;

diff --git a/renderer/src/shaders/color_ramp.glsl b/renderer/src/shaders/color_ramp.glsl
@@ -83,11 +83,9 @@ VERTEX_MAIN(@colorRampVertexMain, Attrs, attrs, _vertexID, _instanceID)
     }
     v_rampColor = unpackColorInt(columnWithinSpan <= 1 ? @a_span.z : @a_span.w);
 
-    float4 pos;
-    pos.x = x * 2. - 1.;
-    pos.y =
-        y * uniforms.gradInverseViewportY - sign(uniforms.gradInverseViewportY);
-    pos.zw = float2(0, 1);
+    float4 pos = pixel_coord_to_clip_coord(float2(x, y),
+                                           2.,
+                                           uniforms.gradInverseViewportY);
 
     VARYING_PACK(v_rampColor);
     EMIT_VERTEX(pos);

diff --git a/renderer/src/shaders/common.glsl b/renderer/src/shaders/common.glsl
@@ -185,12 +185,21 @@ UNIFORM_BLOCK_END(uniforms)
 
 #ifdef @VERTEX
 
+INLINE float4 pixel_coord_to_clip_coord(float2 pixelCoord,
+                                        float inverseViewportX,
+                                        float inverseViewportY)
+{
+    return float4(pixelCoord.x * inverseViewportX - 1.,
+                  pixelCoord.y * inverseViewportY - sign(inverseViewportY),
+                  0.,
+                  1.);
+}
+
+// Defined as a macro because 'uniforms' isn't always available at global scope.
 #define RENDER_TARGET_COORD_TO_CLIP_COORD(COORD)                               \
-    float4((COORD).x* uniforms.renderTargetInverseViewportX - 1.,              \
-           (COORD).y * -uniforms.renderTargetInverseViewportY +                \
-               sign(uniforms.renderTargetInverseViewportY),                    \
-           .0,                                                                 \
-           1.)
+    pixel_coord_to_clip_coord(COORD,                                           \
+                              uniforms.renderTargetInverseViewportX,           \
+                              uniforms.renderTargetInverseViewportY)
 
 #ifndef @RENDER_MODE_MSAA
 // Calculates the Manhattan distance in pixels from the given pixelPosition, to

diff --git a/renderer/src/shaders/draw_path.glsl b/renderer/src/shaders/draw_path.glsl
@@ -130,7 +130,7 @@ VERTEX_MAIN(@drawVertexMain, Attrs, attrs, _vertexID, _instanceID)
     // Paint matrices operate on the fragment shader's "_fragCoord", which is
     // bottom-up in GL.
     float2 fragCoord = vertexPosition;
-#ifdef FRAG_COORD_BOTTOM_UP
+#ifdef @FRAMEBUFFER_BOTTOM_UP
     fragCoord.y = float(uniforms.renderTargetHeight) - fragCoord.y;
 #endif
 
@@ -234,6 +234,9 @@ VERTEX_MAIN(@drawVertexMain, Attrs, attrs, _vertexID, _instanceID)
     if (!shouldDiscardVertex)
     {
         pos = RENDER_TARGET_COORD_TO_CLIP_COORD(vertexPosition);
+#ifdef @POST_INVERT_Y
+        pos.y = -pos.y;
+#endif
 #ifdef @RENDER_MODE_MSAA
         pos.z = normalize_z_index(pathZIndex);
 #endif

diff --git a/renderer/src/shaders/glsl.glsl b/renderer/src/shaders/glsl.glsl
@@ -533,10 +533,6 @@
 
 #define MUL(A, B) ((A) * (B))
 
-#ifndef @TARGET_VULKAN
-#define FRAG_COORD_BOTTOM_UP
-#endif
-
 precision highp float;
 precision highp int;
 

diff --git a/renderer/src/shaders/tessellate.glsl b/renderer/src/shaders/tessellate.glsl
@@ -217,11 +217,9 @@ VERTEX_MAIN(@tessellateVertexMain, Attrs, attrs, _vertexID, _instanceID)
     }
     v_contourIDWithFlags = contourIDWithFlags;
 
-    float4 pos;
-    pos.x = coord.x * (2. / TESS_TEXTURE_WIDTH) - 1.;
-    pos.y = coord.y * uniforms.tessInverseViewportY -
-            sign(uniforms.tessInverseViewportY);
-    pos.zw = float2(0, 1);
+    float4 pos = pixel_coord_to_clip_coord(coord,
+                                           2. / TESS_TEXTURE_WIDTH,
+                                           uniforms.tessInverseViewportY);
 
     VARYING_PACK(v_p0p1);
     VARYING_PACK(v_p2p3);

diff --git a/renderer/src/vulkan/render_context_vulkan_impl.cpp b/renderer/src/vulkan/render_context_vulkan_impl.cpp
@@ -2104,8 +2104,8 @@ RenderContextVulkanImpl::RenderContextVulkanImpl(
         features.fragmentStoresAndAtomics;
     m_platformFeatures.supportsClockwiseAtomicRendering =
         features.fragmentStoresAndAtomics;
-    m_platformFeatures.invertOffscreenY = false;
-    m_platformFeatures.uninvertOnScreenY = true;
+    m_platformFeatures.clipSpaceBottomUp = false;
+    m_platformFeatures.framebufferBottomUp = false;
     m_platformFeatures.maxCoverageBufferLength =
         std::min(features.maxStorageBufferRange, 1u << 28) / sizeof(uint32_t);
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		1b51fe394e81445b8e6c990780ed104ee582fde4
		d8d42c0f130ec1f7d70a9e4ca3cffb1f6e3b9860