fix: 基础 shape destroy 逻辑 #343

huaxiabuluo · 2025-08-19T03:52:01Z

当 Canvas 的实例调用 render 方法完成首次渲染后，再次执行 update 方法更新画布，如果使用了 line | circle | rect | ... 等基础图形标签，会导致上一屏的基础图形不会被正确擦除。

对于下面测试用例：

const context = createContext();
const { props } = (
  <Canvas context={context}>
    <rect
      style={{
        width: 36,
        height: 36,
        fill: 'red',
      }}
    />
  </Canvas>
);

const canvas = new Canvas(props);
await canvas.render();

const { props: nextProps } = (
  <Canvas context={context}>
    <circle
      style={{
        cx: 80,
        cy: 30,
        r: 20,
        lineWidth: 2,
        stroke: '#e45649',
        fill: 'blue',
      }}
    />
  </Canvas>
);
await delay(500);
await canvas.update(nextProps);

await delay(200);
expect(context).toMatchImageSnapshot();

Before (update 渲染异常, rect 没有销毁擦除)

vnode-update-test-tsx-vnode-更新-基础图形更新-1-snap

After (update 渲染正常, rect 被销毁擦除)

tangying1027 · 2025-08-20T07:44:52Z

/gemini review

gemini-code-assist

Code Review

这个 PR 修复了在更新画布时基础图形没有被正确销毁的问题，并为此添加了详尽的测试用例。代码的改动方向是正确的，但 destroyElement 函数的实现方式引入了新的问题，可能导致在某些场景下（如纯删除或组件卸载）图形无法被销毁。我提出了一些修改建议，以使销毁逻辑更加健壮和通用，从而解决这些潜在的 bug。

gemini-code-assist · 2025-08-20T07:45:59Z

packages/f-engine/src/canvas/render/index.ts

+function destroyElement(vNode: VNode | VNode[] | null, nextVNode?: VNode) {
  Children.map(vNode, (node: VNode | null) => {
    if (!node) return;
-    const { component, children } = node;
+    const { component, children, shape, tag } = node;
    if (component) {
      component.willUnmount();
      destroyElement(children);
      component.didUnmount();
      component.destroy();
-    } else {
+    } else if (children) {
      destroyElement(children);
+    } else if (tag === Shape && shape)  {
+      // 更新模式时销毁上个节点
+      nextVNode && shape.destroy();
    }
  });
 }


当前的 destroyElement 实现虽然解决了更新时基础图形的销毁问题（当提供了 nextVNode 时），但未能正确处理其他销毁场景，这可能会导致 bug 和内存泄漏。

具体来说：

纯删除场景：当一个元素被直接移除（而非替换）时，destroyElement 的调用不会传入 nextVNode。在这种情况下，nextVNode && shape.destroy() 的判断会为 false，导致图形不会被销毁。

递归销毁场景：在销毁一个组件时，会递归调用 destroyElement(children)。这个调用同样没有传递 nextVNode，因此该组件下的所有基础图形子节点都不会被销毁。

nextVNode 这个参数及其相关检查似乎是不必要的，并且是导致这些问题的根源。无论是在更新还是删除场景下，只要 destroyElement 被调用，其对应的 shape 都应该被销毁。

建议简化该函数，移除对 nextVNode 的依赖，使销毁逻辑在所有情况下都健壮和正确。

这个修改也需要更新在 updateElement 中对 destroyElement 的调用（在第 220 行）。

function destroyElement(vNode: VNode | VNode[] | null) { Children.map(vNode, (node: VNode | null) => { if (!node) return; const { component, children, shape, tag } = node; if (component) { component.willUnmount(); destroyElement(children); component.didUnmount(); component.destroy(); } else if (children) { destroyElement(children); } else if (tag === Shape && shape) { shape.destroy(); } }); }

我也感觉改这里怪怪的，ai写的挺有道理的

直接 destroy 会导致 A --> null 这种情况离场动画失效，A --> null 的情况 vnode 会直接变为 null，在 vnode tree 中会被去掉并销毁，目前版本的代码逻辑运行没问题

gemini-code-assist · 2025-08-20T07:46:00Z

packages/f-engine/src/canvas/render/index.ts


  const nextVNode = createVNode(parent, nextElement as VNode);
-  destroyElement(lastElement);
+  destroyElement(lastElement, nextVNode);


根据对 destroyElement 函数的修改建议，此处的调用也需要更新，不再传递 nextVNode 参数，以保持一致性并确保销毁逻辑的正确性。

destroyElement(lastElement);

tangying1027 · 2025-08-21T02:36:08Z

packages/f-engine/test/canvas/render/vnodeUpdate.test.tsx

+    );
+
+    // 无离场动画效果
+    await canvas.update(nextProps);


这个应该要有离场动画才对。

这个应该要有离场动画才对。

离场动画在销毁的时候才生效，从 A --> B 这种被当成了更新，会执行 B 的入场动画效果，和现有逻辑一致

A->B 因为是完全不同的元素，所以走的不是更新吧，我感觉应该是销毁（离场）-> 入场。
要不看看其他动画库怎么定义的 react motion库
现有逻辑就是你这个pr要改的，不能参考。

而且没有A的离场但会执行B的入场？不匹配吧

现在的逻辑是执行 B 的入场动画，从 A 到 B 的一个渐变过渡效果，如果等 A 的离场执行结束，再执行 B 的入场，过渡效果就没有了，和现在已有的整体效果完全不一样了

现在的逻辑不就是这份pr要改的bug吗。。。
A到B定义了离场和入场动画，但只执行入场动画不对吧

你放的gif没看懂，是想说是哪个单测的效果呢

你放的gif没看懂，是想说是哪个单测的效果呢

「而且没有A的离场但会执行B的入场？不匹配吧」现在所有的逻辑都是以 B 入场为准，从 A 到 B 会有个过渡效果，这个 pr 只是修复 bug ，不是增加 feature 啊，不变更已有逻辑

你要不本地跑下测试用例或者手动调整代码试下？

群里直接沟通吧

tangying1027 · 2025-08-21T02:36:50Z

packages/f-engine/test/canvas/render/vnodeUpdate.test.tsx

+              property: ['width', 'height'],
+              start: {
+                width: 0,
+                height: 0,


这个单测和你加的逻辑没关系

tangying1027 · 2025-08-21T02:37:26Z

packages/f-engine/test/canvas/render/vnodeUpdate.test.tsx

+
+    await delay(200);
+    const { props: nextProps } = (
+      <Canvas context={context}>


增加一个无离场动画的销毁

tangying1027 · 2025-08-21T02:37:43Z

packages/f-engine/src/canvas/render/index.ts

+function destroyElement(vNode: VNode | VNode[] | null, nextVNode?: VNode) {
  Children.map(vNode, (node: VNode | null) => {
    if (!node) return;
-    const { component, children } = node;
+    const { component, children, shape, tag } = node;
    if (component) {
      component.willUnmount();
      destroyElement(children);
      component.didUnmount();
      component.destroy();
-    } else {
+    } else if (children) {
      destroyElement(children);
+    } else if (tag === Shape && shape)  {
+      // 更新模式时销毁上个节点
+      nextVNode && shape.destroy();
    }
  });
 }


我也感觉改这里怪怪的，ai写的挺有道理的

edokeh · 2025-09-01T13:22:00Z

关于 F2 的动画设计，达成几项一致：

leave/appear 配置，不管是单 shape 还是多 shape，都应该被执行
除非多 shape 的后者（A->B 的 B），指定了 update，则跳过 leave/appear
多 shape 过渡，A leave -> B appear，决定采用串行，比较符合上层业务的预期直觉

tangying1027 · 2025-09-02T02:16:44Z

关于 F2 的动画设计，达成几项一致：

leave/appear 配置，不管是单 shape 还是多 shape，都应该被执行

除非多 shape 的后者（A->B 的 B），指定了 update，则跳过 leave/appear

多 shape 过渡，A leave -> B appear，决定采用串行，比较符合上层业务的预期直觉

补充：
可以渐进式修改，先修改无动画情况，把带动画的单测全部去掉，以免造成混乱

fix: basic shapes update

4a5d262

gemini-code-assist bot reviewed Aug 20, 2025

View reviewed changes

tangying1027 reviewed Aug 21, 2025

View reviewed changes

fix: 基础 shape destroy 逻辑 #343

Are you sure you want to change the base?

fix: 基础 shape destroy 逻辑 #343

Uh oh!

Conversation

huaxiabuluo commented Aug 19, 2025

Uh oh!

tangying1027 commented Aug 20, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edokeh commented Sep 1, 2025

Uh oh!

tangying1027 commented Sep 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants