#1200: Fix issue with aggregated errors in shader display when using …

…local matrices (#1205) * Updated getDerivedValue() to return const and fixed issue with mutation in JsiImageShader node The Image Shader node was previously set up so that the local matrix was updated multiple times, aggregating errors when the transform prop was not changed. This mutation is not something we wan't so this PR changes the getDerivedValue() on props to return a readonly value - which can NOT be mutatet. Any mutations must be done by creating a copy locally and modifying it. This caused a few other props to no longer compile, and these were also updated after this change to avoid any extra mutations. I tried writing a test for this, but since it required us to control multiple render passes I was not able to get the test runner to deterministically do this correctly. Instead the Filters example has been updated with touch and state - clicking on the example should no longer cause aggregated errors in the shader. fixes #1200 * CI: Lint / Clang format * Updated after CR * Add test Co-authored-by: William Candillon <[email protected]>
Shopify · Dec 16, 2022 · 4ac3c16 · 4ac3c16
1 parent b51f0b9
commit 4ac3c16
Show file tree

Hide file tree

Showing 10 changed files with 78 additions and 32 deletions.
diff --git a/docs/static/img/shaders/image-with-transform.png b/docs/static/img/shaders/image-with-transform.png
diff --git a/example/src/Examples/Filters/Filters.tsx b/example/src/Examples/Filters/Filters.tsx
@@ -24,6 +24,7 @@ half4 main(float2 xy) {
 export const Filters = () => {
   const { width, height } = useWindowDimensions();
   const progress = useLoop({ duration: 1500 });
+  const [, setState] = React.useState(0);
 
   const uniforms = useComputedValue(
     () => ({ r: mix(progress.current, 1, 100) }),
@@ -35,14 +36,16 @@ export const Filters = () => {
     return null;
   }
   return (
-    <Canvas style={{ width, height }}>
+    <Canvas style={{ width, height }} onTouch={() => setState((i) => i + 1)}>
       <Fill>
         <Shader source={source} uniforms={uniforms}>
           <ImageShader
             image={image}
             fit="cover"
             x={0}
             y={0}
+            tx="repeat"
+            ty="repeat"
             width={width}
             height={height}
           />

diff --git a/package/cpp/rnskia/dom/base/DerivedNodeProp.h b/package/cpp/rnskia/dom/base/DerivedNodeProp.h
@@ -116,7 +116,7 @@ template <typename T> class DerivedProp : public BaseDerivedProp {
   /**
   Returns the derived value
    */
-  std::shared_ptr<T> getDerivedValue() { return _derivedValue; }
+  std::shared_ptr<const T> getDerivedValue() { return _derivedValue; }
 
   /**
    Returns true if is optional and one of the child props has a value, or all
@@ -128,7 +128,7 @@ template <typename T> class DerivedProp : public BaseDerivedProp {
   /**
    Set derived value from sub classes
    */
-  void setDerivedValue(std::shared_ptr<T> value) {
+  void setDerivedValue(std::shared_ptr<const T> value) {
     setIsChanged(_derivedValue != value);
     _derivedValue = value;
   }
@@ -138,11 +138,11 @@ template <typename T> class DerivedProp : public BaseDerivedProp {
    */
   void setDerivedValue(const T &&value) {
     setIsChanged(true);
-    _derivedValue = std::make_shared<T>(std::move(value));
+    _derivedValue = std::make_shared<const T>(std::move(value));
   }
 
 private:
-  std::shared_ptr<T> _derivedValue;
+  std::shared_ptr<const T> _derivedValue;
 };
 
 /**

diff --git a/package/cpp/rnskia/dom/nodes/JsiPathNode.h b/package/cpp/rnskia/dom/nodes/JsiPathNode.h
@@ -59,20 +59,22 @@ class JsiPathNode : public JsiDomDrawingNode,
                 ", end: " + std::to_string(_endProp->value().getAsNumber()));
           }
           filteredPath.swap(filteredPath);
-          _path = std::make_shared<SkPath>(filteredPath);
+          _path = std::make_shared<const SkPath>(filteredPath);
         } else if (hasStartOffset || hasEndOffset) {
           throw std::runtime_error(
               "Failed trimming path with parameters start: " +
               std::to_string(_startProp->value().getAsNumber()) +
               ", end: " + std::to_string(_endProp->value().getAsNumber()));
         } else {
-          _path = std::make_shared<SkPath>(filteredPath);
+          _path = std::make_shared<const SkPath>(filteredPath);
         }
 
         // Set fill style
         if (_fillTypeProp->isSet()) {
           auto fillType = _fillTypeProp->value().getAsString();
-          _path->setFillType(getFillTypeFromStringValue(fillType));
+          auto p = std::make_shared<SkPath>(*_path.get());
+          p->setFillType(getFillTypeFromStringValue(fillType));
+          _path = std::const_pointer_cast<const SkPath>(p);
         }
 
         // do we have a special paint here?
@@ -105,9 +107,14 @@ class JsiPathNode : public JsiDomDrawingNode,
             precision = opts.getValue(PropNamePrecision).getAsNumber();
           }
 
-          if (!strokePaint.getFillPath(*_path.get(), _path.get(), nullptr,
+          // _path is const so we can't mutate it directly, let's replace the
+          // path like this:
+          auto p = std::make_shared<SkPath>(*_path.get());
+          if (!strokePaint.getFillPath(*_path.get(), p.get(), nullptr,
                                        precision)) {
             _path = nullptr;
+          } else {
+            _path = std::const_pointer_cast<const SkPath>(p);
           }
         }
 
@@ -157,7 +164,7 @@ class JsiPathNode : public JsiDomDrawingNode,
   NodeProp *_fillTypeProp;
   NodeProp *_strokeOptsProp;
 
-  std::shared_ptr<SkPath> _path;
+  std::shared_ptr<const SkPath> _path;
 };
 
 class StrokeOptsProps : public BaseDerivedProp {

diff --git a/package/cpp/rnskia/dom/nodes/JsiShaderNodes.h b/package/cpp/rnskia/dom/nodes/JsiShaderNodes.h
@@ -121,8 +121,16 @@ class JsiImageShaderNode : public JsiBaseShaderNode,
       if (rect != nullptr && lm != nullptr) {
         auto rc = _imageProps->getDerivedValue();
         auto m3 = _imageProps->rect2rect(rc->src, rc->dst);
-        lm->preTranslate(m3.x(), m3.y());
-        lm->preScale(m3.width(), m3.height());
+        if (_transformProp->isChanged()) {
+          // To modify the matrix we need to copy it since we're not allowed to
+          // modify values contained in properties - this would have caused the
+          // matrix to be translated and scaled more and more for each render
+          // even thought the matrix prop did not change.
+          _matrix.reset();
+          _matrix.preConcat(*lm);
+          _matrix.preTranslate(m3.x(), m3.y());
+          _matrix.preScale(m3.width(), m3.height());
+        }
       }
 
       setShader(
@@ -133,7 +141,7 @@ class JsiImageShaderNode : public JsiBaseShaderNode,
                                     _filterModeProp->value().getAsString()),
                                 getMipmapModeFromString(
                                     _mipmapModeProp->value().getAsString())),
-              lm));
+              &_matrix));
     }
   }
 
@@ -185,6 +193,8 @@ class JsiImageShaderNode : public JsiBaseShaderNode,
                              "\" is not a valid Mipmap Mode.");
   }
 
+  SkMatrix _matrix;
+
   TileModeProp *_txProp;
   TileModeProp *_tyProp;
   NodeProp *_filterModeProp;
@@ -328,12 +338,12 @@ class JsiBaseGradientNode : public JsiBaseShaderNode {
     }
   }
 
-  SkColor *_colors;
-  SkTileMode _mode;
+  const SkColor *_colors;
   double _flags;
-  SkScalar *_positions;
-  SkMatrix *_matrix;
   int _colorCount;
+  SkTileMode _mode;
+  const SkScalar *_positions;
+  const SkMatrix *_matrix;
 
 private:
   TransformsProps *_transformsProps;

diff --git a/package/cpp/rnskia/dom/props/ClipProp.h b/package/cpp/rnskia/dom/props/ClipProp.h
@@ -45,18 +45,18 @@ class ClipProp : public BaseDerivedProp {
     return _pathProp->isSet() || _rectProp->isSet() || _rrectProp->isSet();
   }
 
-  SkPath *getPath() { return _path.get(); }
-  SkRect *getRect() { return _rect.get(); }
-  SkRRect *getRRect() { return _rrect.get(); }
+  const SkPath *getPath() { return _path.get(); }
+  const SkRect *getRect() { return _rect.get(); }
+  const SkRRect *getRRect() { return _rrect.get(); }
 
 private:
   PathProp *_pathProp;
   RectProp *_rectProp;
   RRectProp *_rrectProp;
 
-  std::shared_ptr<SkPath> _path;
-  std::shared_ptr<SkRect> _rect;
-  std::shared_ptr<SkRRect> _rrect;
+  std::shared_ptr<const SkPath> _path;
+  std::shared_ptr<const SkRect> _rect;
+  std::shared_ptr<const SkRRect> _rrect;
 };
 
 } // namespace RNSkia
diff --git a/package/cpp/rnskia/dom/props/ImageProps.h b/package/cpp/rnskia/dom/props/ImageProps.h
@@ -74,7 +74,9 @@ class ImageProps : public DerivedProp<FitRects> {
   }
 
   sk_sp<SkImage> getImage() { return _imageProp->getDerivedValue(); }
-  std::shared_ptr<SkRect> getRect() { return _rectProp->getDerivedValue(); }
+  std::shared_ptr<const SkRect> getRect() {
+    return _rectProp->getDerivedValue();
+  }
 
   SkRect rect2rect(SkRect src, SkRect dst) {
     auto scaleX = dst.width() / src.width();

diff --git a/package/cpp/rnskia/dom/props/VerticesProps.h b/package/cpp/rnskia/dom/props/VerticesProps.h
@@ -40,9 +40,9 @@ class VerticesProps : public DerivedSkProp<SkVertices> {
   bool hasColors() { return _colorsProp->isSet(); }
 
   void updateDerivedValue() override {
-    SkVertices::VertexMode *vertextMode =
+    const SkVertices::VertexMode *vertextMode =
         _vertexModeProp->getDerivedValue().get();
-    std::vector<SkColor> *colors = _colorsProp->getDerivedValue().get();
+    const std::vector<SkColor> *colors = _colorsProp->getDerivedValue().get();
     auto vertices = _verticesProp->getDerivedValue();
     auto textures = _texturesProp->getDerivedValue();
     auto indices = _indicesProp->getDerivedValue();

diff --git a/package/src/dom/nodes/paint/Shaders.ts b/package/src/dom/nodes/paint/Shaders.ts
@@ -67,20 +67,20 @@ export class ImageShaderNode extends ShaderDeclaration<ImageShaderProps> {
   materialize() {
     const { fit, image, tx, ty, fm, mm, ...imageShaderProps } = this.props;
     const rct = getRect(this.Skia, imageShaderProps);
+    const m3 = this.Skia.Matrix();
     if (rct) {
       const rects = fitRects(
         fit,
         { x: 0, y: 0, width: image.width(), height: image.height() },
         rct
       );
-      const m3 = rect2rect(rects.src, rects.dst);
-      imageShaderProps.transform = [
-        ...(imageShaderProps.transform ?? []),
-        ...m3,
-      ];
+      const [x, y, sx, sy] = rect2rect(rects.src, rects.dst);
+      m3.translate(x.translateX, y.translateY);
+      m3.scale(sx.scaleX, sy.scaleY);
     }
     const lm = this.Skia.Matrix();
     processTransformProps(lm, imageShaderProps);
+    lm.concat(m3);
     return image.makeShaderOptions(
       TileMode[enumKey(tx)],
       TileMode[enumKey(ty)],

diff --git a/package/src/renderer/__tests__/e2e/Shader.spec.tsx b/package/src/renderer/__tests__/e2e/Shader.spec.tsx
@@ -9,7 +9,8 @@ import {
   Shader,
   ColorShader,
 } from "../../components";
-import { importSkia, surface } from "../setup";
+import { images, importSkia, surface } from "../setup";
+import { ImageShader } from "../../components/image/ImageShader";
 
 const bilinearInterpolation = `
 uniform vec4 position;
@@ -177,4 +178,27 @@ half4 main(float2 p) {
     );
     checkImage(img, docPath("shaders/color.png"));
   });
+
+  it("should display an image and respect the transform", async () => {
+    const { oslo } = images;
+    const { width, height } = surface;
+    const { vec } = importSkia();
+    const img = await surface.draw(
+      <Fill>
+        <ImageShader
+          image={oslo}
+          x={0}
+          y={0}
+          width={width}
+          height={height}
+          fit="cover"
+          transform={[{ scale: 2 }]}
+          origin={vec(width / 2, height / 2)}
+        />
+      </Fill>
+    );
+    checkImage(img, docPath("shaders/image-with-transform.png"), {
+      overwrite: true,
+    });
+  });
 });