Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions packages/f-engine/src/canvas/render/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,17 +185,20 @@ function createElement(parent: VNode, element: JSX.Element): VNode | VNode[] | n
});
}

function destroyElement(vNode: VNode | VNode[] | null) {
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();
}
});
}
Comment on lines +188 to 204

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

具体来说:

  1. 纯删除场景:当一个元素被直接移除(而非替换)时,destroyElement 的调用不会传入 nextVNode。在这种情况下,nextVNode && shape.destroy() 的判断会为 false,导致图形不会被销毁。
  2. 递归销毁场景:在销毁一个组件时,会递归调用 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();
    }
  });
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand All @@ -214,7 +217,7 @@ function updateElement(parent: VNode, nextElement: JSX.Element, lastElement: VNo
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

  destroyElement(lastElement);

return nextVNode;
}

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
205 changes: 205 additions & 0 deletions packages/f-engine/test/canvas/render/vnodeUpdate.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,209 @@ describe('vnode 更新', () => {

expect(ref.current._vNode.props.update).toBe(true);
});

it('基础图形更新', async () => {
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();
});

it('基础图形销毁带离场动画', async () => {
const context = createContext();
const { props } = (
<Canvas context={context}>
<rect
style={{
width: 36,
height: 36,
fill: 'red',
}}
animation={{
leave: {
easing: 'ease-out',
duration: 500,
property: ['width', 'height'],
start: {
width: 36,
height: 36,
},
end: {
width: 0,
height: 0,
},
},
}}
/>
</Canvas>
);

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</Canvas>
);
await canvas.update(nextProps);
await delay(600);
expect(context).toMatchImageSnapshot();
});

it('基础图形更新带离场动画', async () => {
const context = createContext();
const { props } = (
<Canvas context={context}>
<rect
style={{
width: 36,
height: 36,
fill: 'red',
}}
animation={{
leave: {
easing: 'ease-out',
duration: 500,
property: ['width', 'height'],
start: {
width: 36,
height: 36,
},
end: {
width: 0,
height: 0,
},
},
}}
/>
</Canvas>
);

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

群里直接沟通吧

await delay(600);
expect(context).toMatchImageSnapshot();
});

it('基础图形更新带入场动画', async () => {
const context = createContext();
const { props } = (
<Canvas context={context}>
<circle
style={{
cx: 30,
cy: 30,
r: 20,
lineWidth: 2,
stroke: '#e45649',
fill: 'blue',
}}
animation={{
appear: {
easing: 'ease-in',
duration: 300,
property: ['r'],
start: {
r: 0,
},
end: {
r: 20,
},
},
}}
/>
</Canvas>
);

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

await delay(500);
const { props: nextProps } = (
<Canvas context={context}>
<rect
style={{
x: 80,
y: 30,
width: 36,
height: 36,
fill: 'red',
}}
animation={{
update: {
easing: 'ease-out',
duration: 500,
property: ['width', 'height'],
start: {
width: 0,
height: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
end: {
width: 36,
height: 36,
},
},
}}
/>
</Canvas>
);

// 执行入场动画
await canvas.update(nextProps);
await delay(600);
expect(context).toMatchImageSnapshot();
});

});
Loading