Skip to content

Add PAGX node channel reflection API, matrix animation, and in-place notifyChange refresh.#3505

Open
Hparty wants to merge 39 commits into
mainfrom
feature/partyhuang_pagx_notifyChange
Open

Add PAGX node channel reflection API, matrix animation, and in-place notifyChange refresh.#3505
Hparty wants to merge 39 commits into
mainfrom
feature/partyhuang_pagx_notifyChange

Conversation

@Hparty

@Hparty Hparty commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

新增按名读写 PAGX 节点通道的反射 API、矩阵关键帧动画,以及增量式的 PAGXDocument::notifyChange——把构建后的编辑反映到所有存活的 PAGScene 上,涵盖属性编辑、结构增删、动画(timeline)编辑和跨文档(外部 composition)编辑。同时在 PAGLayer 上暴露源节点 id。

反射 API (include/pagx/PAGXNodeChannel.h)

  • GetNodeChannel / SetNodeChannel —— 按通道名读写节点的标量属性(通道名与 PAGX XML 一致,如 alphaposition.xblendMode)。
  • IsAnimatableChannel / RequiresLayout —— 两个正交查询。一个通道可同时满足两者:形状的 size.width 在播放时可原地动画,但在文档上编辑它需要重新布局。内部由 ChannelFlags 位标志支撑(src/pagx/PAGXChannelTable.h)。

notifyChange —— 增量刷新

PAGXDocument::notifyChange(dirtyNodes, layoutChanged) 把编辑反映到每个存活 scene,并尽量保留 tgfx layer 句柄:

  • 属性编辑(alpha/颜色/几何)—— 原地刷新;layoutChanged 为 true 时对影响布局的字段重新布局。
  • 结构变更 —— 传入容器节点以协调其子 layer 列表;空 layer 新增内容时会被提升为 VectorLayer
  • 时间轴 —— 编辑时间轴节点(Animation/AnimationObject/Channel)会重建所有 timeline,使动画的增删改生效;非时间轴编辑不打断正在进行的播放。
  • 引用编辑 —— 当编辑改变了某个 @id 引用(如 AnimationObject.targetFill.color),调用方需把受影响引用链上的所有节点都标记为脏。
  • 跨文档 —— 嵌入外部 composition 文档的 scene 会反向注册到该文档;编辑子文档并调用其自身的 notifyChange 会重建所有嵌入它的 scene 的运行时树。节点只能通过其所属文档来通知(父文档不能通知子文档的节点)。

动画

  • 矩阵关键帧动画,使用共享的 TRS 分解(MatrixDecompose.h);为每个可动画通道注册运行时 writer,并由一个注册表一致性测试守护。
  • 已记录的限制:矩阵插值无法还原旋转圈数 —— 需要精确多圈旋转时请使用标量 Group::rotation 通道。

PAGLayer

  • PAGLayer::id() 返回源节点的 id,调用方可据此识别其对应的文档节点。

加固(来自两轮多 agent review)

  • 按真实渲染行为修正通道标志(Path position → 仅布局;Text x/y、形状 position → 可动画且影响布局)。
  • 原地刷新会重置被移除的 mask;并对 VectorLayer 强转加类型守卫,避免类型不匹配。
  • composition 子节点移除时分离的是 slot(不再遗留孤立容器);runtimeLayer 重同步只作用于普通子 layer,使 composition 子节点保留其子树根。
  • unbindSubtree 会回收内容元素 / 颜色源 / ColorStop / ImagePattern-Image 的绑定,并通过存活引用扫描尊重共享(@id)资源。
  • 修复一处 use-after-free:缓存的顶层 PAGTimeline 现在通过所属 scene 懒解析其 binding,从而在运行时树重建后依然有效。

兼容性

没有任何 PAGX 节点新增可序列化的属性字段。唯一的数据模型变更是 Channel 的值类型新增了 Matrix 备选项(向后兼容)。旧的 PAGX 文件解析行为不变。

测试

PAGFullTest 编译干净;全部 231 个 PAGXTest/PAGXRuntimeTest 用例通过,新增覆盖包括时间轴重建、跨文档与嵌套(A→B→C)同步、重建后显示选项保留,以及缓存 timeline 跨重建的路径。

已知限制 / 后续

  • Layer 标量变换通道(rotation/scale)有意未加入;精确旋转动画应驱动标量 Group::rotation 通道。
  • 跨文档编辑会重建嵌入 scene 的运行时树,因此编辑前获取的句柄需重新获取,且嵌入子树中正在进行的播放会重新开始。

@codecov-commenter

codecov-commenter commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.45377% with 343 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.92%. Comparing base (1b8e368) to head (d2c1484).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/renderer/LayerBuilder.cpp 68.08% 91 Missing and 51 partials ⚠️
src/pagx/PAGXNodeChannel.cpp 72.23% 71 Missing and 32 partials ⚠️
src/pagx/runtime/PAGComposition.cpp 71.69% 5 Missing and 25 partials ⚠️
src/renderer/LayerBuilder.h 73.25% 9 Missing and 14 partials ⚠️
test/src/PAGXTest.cpp 99.06% 4 Missing and 7 partials ⚠️
src/pagx/runtime/MatrixDecompose.h 79.06% 1 Missing and 8 partials ⚠️
src/pagx/PAGScene.cpp 83.78% 1 Missing and 5 partials ⚠️
src/pagx/PAGXImporter.cpp 0.00% 6 Missing ⚠️
src/pagx/PAGXDocument.cpp 87.50% 1 Missing and 4 partials ⚠️
src/pagx/PAGXExporter.cpp 0.00% 4 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3505      +/-   ##
==========================================
+ Coverage   80.78%   80.92%   +0.13%     
==========================================
  Files         622      625       +3     
  Lines       68220    70517    +2297     
  Branches    20234    20747     +513     
==========================================
+ Hits        55114    57064    +1950     
- Misses       9152     9335     +183     
- Partials     3954     4118     +164     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Hparty Hparty changed the title Add PAGX node channel reflection, matrix animation, and in-place notifyChange refresh. 添加 PAGX 节点通道反射 API、矩阵动画及 notifyChange 就地刷新。 Jun 12, 2026
@Hparty Hparty changed the title 添加 PAGX 节点通道反射 API、矩阵动画及 notifyChange 就地刷新。 Add PAGX node channel reflection API, matrix animation, and in-place notifyChange refresh. Jun 12, 2026
Comment thread src/pagx/PAGTimeline.cpp
return;
}
if (!resolved) {
resolveTargets();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Major] Timeline cache 失效完全依赖调用方手动标 timeline 节点,且 PAGTimeline.h 注释陈旧

这里 if (!resolved) { resolveTargets(); }resolved 一旦置 true 后再不会被重置(PAGTimeline.cpp:67)。PAGTimeline.h:142-143 的注释承诺 notifyChange → onNodesChangedresets resolved to false on affected timelines,但实际实现是:当 dirty 节点是 Animation/AnimationObject/Channel 之一时,PAGScene::onNodesChanged(PAGScene.cpp:264-281)整体清空 timelinesByAnimation 并调用 resetTimelines() 重建 PAGTimeline 实例,跟 resolved 标志无关。

更深层的真实风险:用户编辑 AnimationObject.target 字符串改成新 id 后,必须主动把 AnimationObject 节点放进 dirtyNodes 才会触发 timeline 重建。若调用方只把『新指向的目标节点』标 dirty 而忘标 AnimationObject,下一次 apply() 仍走旧的 resolvedTargets 缓存——而 PR 文档要求调用方『mark every node on the affected reference chain dirty』,但 API 没有任何防错机制,是 silent staleness。

建议:① 同步更新 PAGTimeline.h:142-143 注释为实际机制;② 在 notifyChange 内部对每个 dirty 节点反向查找它是否被某 AnimationObject 引用为 target,自动失效相关 timeline cache。

@Hparty Hparty Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

感谢 review。这条想分两段回应:注释先修了;自动反向扫描这次不做。

注释 drift 已修

PAGTimeline.h:142-143 的承诺确实和实际机制不一致,已经更新成对齐当前实现的描述:cache 不会被原地 reset,timeline 节点变更走的是"由 PAGScene::onNodesChanged 重建整个 PAGTimeline 实例"的路径。resolvedTargets 字段上方的注释也同步说明了 caller 需把 timeline-affecting 节点(包括 AnimationObject)标 dirty 才会触发 retargeting。

自动反向扫描这次不做

讨论后认为现在这个行为是合理的,不在本次 PR 处理:

  1. 不会 crash 或内存问题。runtime tree 持有的是 (Node*, vector<Channel*>) 缓存,target 节点指针仍指向 doc 里活着的节点;用户改 AnimationObject.target 但没标 dirty 时,timeline 仍驱动旧 target,新 target 不动——只是看不到效果,没有 silent corruption。

  2. 行为与其他 PAG 运行时对象一致。runtime 对象(PAGComposition / PAGLayer / RuntimeBinding 等)都是 build 时基于当时数据构建一次的快照,调用方不主动通知就看不到改动。timeline 在这条规则上没有特殊地位。

  3. API 契约已经写明白PAGXDocument::notifyChange 的注释里已有 reference chain 规则——改 @id 引用的字段必须把引用链上所有节点都标 dirty。违反契约的代价就是"没看到效果",是文档化的行为。

Comment thread src/renderer/LayerBuilder.cpp Outdated
if (color == nullptr || !_result.binding.contains(color)) {
return;
}
for (const auto* node : _result.binding.boundNodes()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Major] unbindColorSourceIfUnreferenced / unbindImageIfUnreferenced 在大文档下退化为 O(N×M)

boundNodes()(LayerBuilder.h:148)每次调用都从 unordered_map 拷贝整张表到 vector(O(N) 拷贝 + 一次堆分配)。unbindContentElements 对每个 Fill/Stroke 都触发一次 unbindColorSourceIfUnreferenced,每个 ImagePattern 还会嵌套触发 unbindImageIfUnreferenced(line 446 同样模式)。

移除一个含 K 个 painter 的子树时是 K 次 O(N) 全表扫描 + K 次 vector 分配。设计稿规模上来后(N、K 都到千级),单次结构编辑就可能产生百万级的 node 比较,体感卡顿。

建议:维护反向索引 unordered_map<const ColorSource*, std::vector<const Element*>>(painter→users),或在 RuntimeBinding 上提供 forEachOfType(NodeType, callback) 直接遍历内部 map 而不分配 vector。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同,已改。

boundNodes() 删掉,换成 forEachBoundNode<Fn>(fn) 模板:直接遍历内部 targets map,回调返回 false 终止迭代。两个调用点(unbindColorSourceIfUnreferenced / unbindImageIfUnreferenced)都改成 lambda 形态,用 stillReferenced 标志位代替原先的早 return

效果:

  • 单次 unbind 不再分配 vector,全表扫描时间复杂度不变但常数更小
  • 反向索引的方案也评估过,但维护成本(每次 set/remove 都要双向同步)和当前规模不匹配,先用零分配迭代覆盖到 K-千级规模,等真出现明显瓶颈再上反向索引

Comment thread include/pagx/PAGXDocument.h Outdated
* render-only refresh. Defaults to true. Callers that mutate via SetNodeChannel can derive this
* from RequiresLayout; structural add/remove must pass true.
*/
void notifyChange(const std::vector<Node*>& dirtyNodes, bool layoutChanged = true);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Major] layoutChanged = true 默认值隐藏了完整布局开销

默认值 true 意味着任何忽略此参数的调用都会触发 applyLayout() 全文档布局(PAGXDocument.cpp:338-340)。注释说 Callers that mutate via SetNodeChannel can derive this from RequiresLayout,但这要求调用方主动调 RequiresLayout() 取得正确值再传入;若调用方误以为『默认就是对的』就白白付出布局开销。

失败安全原则:昂贵操作不应是默认值。建议两选一:
① 移除默认值,强制调用方显式选择;
② 改成 enum class LayoutMode { Skip, Run }(或类似)让意图显式不可省略。

@Hparty Hparty Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同,已移除默认值。

具体改动:

  • notifyChange 第二参数从 bool layoutChanged = true 改成 bool layoutChanged移除默认值,强制调用方做出显式选择。
  • 注释里同步说明结构性变更(add/remove)必须传 true,render-only 编辑(如 alpha)才能传 false;并指引 SetNodeChannel 调用方用 RequiresLayout(NodeType, channel) 自动派生。

最初考虑过改成 enum class LayoutMode { RunLayout, SkipLayout },但仓库里已经有一个 pagx::LayoutMode(用于 Layer 容器排列方向 Vertical/Horizontal)会命名冲突。在新的"layout/render"语义没有更精准命名前,先保留 bool 但移除默认值即可——拿掉默认值之后调用方必然在 Set/Run 之间显式选择,加 enum 的可读性边际收益就不大了。

测试代码 24 个调用点全部按编辑语义补上注释化的实参(/*layoutChanged=*/truefalse)。

Comment thread src/pagx/PAGXDocument.cpp Outdated
// A node owned by an external (child) document must be notified through that document, not a
// parent. Reject foreign nodes: they are simply not in this document's node list.
for (auto* node : dirtyNodes) {
if (node != nullptr && !ownsNode(node)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Major] notifyChange 整批拒绝 + ownsNode 不对外暴露,调用方既无预检又无反馈

两个相关问题:

  1. 整批拒绝:发现一个外部 composition 节点立刻 return,整个 notifyChange 中止,所有合法的脏节点也不会被刷新。设想用户从 UI 多选了一批节点批量 notify,里面恰好包含一个外部节点 → 静默失败,体验断崖。

  2. 不可预检ownsNode 是 private(PAGXDocument.h:219),调用方无法在调用前预检节点归属;错误只能通过 LOGE 事后发现,没有返回值/异常告知调用方。

建议:① 改为过滤而非拒绝(跳过外部节点继续处理本文档节点);② 把 ownsNode 提为 public,或者让 notifyChange 返回 bool / 拒绝节点数让调用方知情。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同两条建议,已改。

过滤而非整批拒绝

notifyChange 实现改为:

  • 遍历 dirtyNodes 一次,把属于本文档的 push 进 ownedDirty,外部节点累加 foreignCount
  • 整批结束后聚合一条 LOGE 报告 foreign 节点数(不每个都 LOGE,避免日志噪声)
  • 只用 ownedDirty 走后续 applyLayout() + onNodesChanged() 路径

效果:editor 多选场景(dirty 列表混合本地节点和误拖入的外部节点)现在合法节点照样刷新,外部节点跳过且日志可见。原测试 ExternalPAGXParentCannotNotifyChildNode(仅传外部节点的 batch)行为保留——ownedDirty.empty() 时直接 return。

ownsNode 提为 public

加上完整 doc 注释,明确:

  • 一个节点只属于一个 document
  • 调用方静态不知道节点归属时,用 ownsNode 预检后再 dispatch 到正确的 owning document
  • 也可以拿来给 batch 做 filter
  • 注释里点出 notifyChange 内部已经会过滤,所以这个查询是 routing 工具,不是 correctness 必需

* @param type the node type.
* @param channel the channel name.
*/
bool RequiresLayout(NodeType type, const std::string& channel);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Minor] 反射 API 缺少『列出/查询合法 channel』的 introspection 接口

IsAnimatableChannel / RequiresLayout 对未知 channel 一律返回 false(注释明确说 for unknown channels),调用方无法区分『通道存在但不可动画化』vs『通道根本不存在(typo)』。GetNodeChannel 失败也是返回 false 没有日志(仅 SetNodeChannel 在 .cpp:912 输出 LOGE)。

编辑器场景下 channel 名 typo(如 Alpha vs alpha)是常见错误,目前完全静默;同时调用方除了翻 PAGX schema 文档,没有运行时手段枚举一个 NodeType 的所有合法 channel。

建议新增至少一个 introspection API:

  • std::vector<std::string> ListChannels(NodeType type);
  • bool ChannelExists(NodeType type, const std::string& channel);

或把 IsAnimatableChannel 返回 enum(Unknown / Animatable / NotAnimatable)以区分三态。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同,已加 introspection 接口 + 补 LOGE。

新增的 public API

bool ChannelExists(NodeType type, const std::string& channel);
std::vector<std::string> ListChannels(NodeType type);

ChannelExists 是单点判断,让调用方区分"通道不存在(typo)" vs "通道存在但不可动画化/不需布局"——前面 IsAnimatableChannel / RequiresLayout 把这两种情况都收敛成 false,确实信息丢了。

ListChannels 是 editor 属性面板的批量场景,返回当前节点类型的所有通道名(顺序是表里声明序,不保证跨 release 稳定,注释里写了)。

没改 IsAnimatableChannel 的返回类型成三态枚举,避免破坏 ABI;编辑器要"三态"就 ChannelExists + IsAnimatableChannel 组合一下足够。

GetNodeChannel 失败补 LOGE

SetNodeChannel / ResetNodeChannel 对齐:未知通道 / 读失败都打日志,错误信息带 channel 名和 nodeType,便于编辑器排查。

Comment thread src/pagx/runtime/MatrixDecompose.h Outdated
DecomposedMatrix out = {};
out.translateX = a.translateX + (b.translateX - a.translateX) * t;
out.translateY = a.translateY + (b.translateY - a.translateY) * t;
out.rotation = a.rotation + (b.rotation - a.rotation) * t;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Minor] 矩阵旋转跨 ±π 边界时取了长路径

out.rotation = a.rotation + (b.rotation - a.rotation) * t; 对跨 ±π 边界的旋转走错路径。例:两关键帧分别为 170° 与 190°(实际同方向连续 20° 旋转),atan2 把它们恢复为 +2.967 / -2.967 rad,直接相减得到 -5.934 rad,插值反向旋转 340°。

顶部注释的『多圈不可还原』限制是矩阵分解的固有限制,但短路径修复与之正交,可以独立成立——4 行代码就能消除大多数实际感知到的『反向旋转』bug:

float rotDiff = b.rotation - a.rotation;
if (rotDiff > M_PI) rotDiff -= 2 * M_PI;
else if (rotDiff < -M_PI) rotDiff += 2 * M_PI;
out.rotation = a.rotation + rotDiff * t;

保留头文件中『需要精确多圈旋转请用 Group::rotation 标量通道』的指导即可。

@Hparty Hparty Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同,已修。

MixDecomposed 改成把 rotation 差值 wrap 到 [-pi, pi]

float rotDiff = b.rotation - a.rotation;
if (rotDiff > Pi) rotDiff -= TwoPi;
else if (rotDiff < -Pi) rotDiff += TwoPi;
out.rotation = a.rotation + rotDiff * t;

跟你贴的逻辑一致,常量提到顶部 Pi / TwoPi 避免重复。注释里说明这是短路径修复,与"多圈不可还原"的限制正交,需要精确多圈时还是走 scalar Group::rotation 通道。

新增测试 PAGXTest.ChannelLayerMatrixRotationShortestArc:170deg → 190deg 的矩阵 keyframe,采样 25%/50%/75%,断言 sin 单调跨过零(m25.b > 0m75.b < 0),并在 50% 时正好是 180deg(cos = -1sin = 0)。修复前会绕 340deg 反向,sin 会在中段先反向再回来,断言挂;修复后直接通过。


static const ChannelDef* FindChannel(NodeType type, const std::string& channel) {
const auto& table = ChannelsFor(type);
for (const auto& field : table) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nit] FindChannel 每次按字符串线性扫描整个 channel 表

ChannelsFor(type) 返回 std::vector<ChannelDef>FindChannel 每次都按字符串遍历比较。单个表 5–30 项,单次开销可忽略,但反射 API 的所有入口(GetNodeChannel / SetNodeChannel / ResetNodeChannel / IsAnimatableChannel / RequiresLayout)都过这条路径,编辑器场景下累积开销可观。

建议把 ChannelsFor 内部改为返回静态局部 unordered_map<std::string_view, const ChannelDef*>(首次构造时从 vector 填充),保持 vector 数据驱动配置不变,仅查找路径升级到 O(1)。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同,已改。

保留 ChannelsFor(type) 仍然返回 vector<ChannelDef>(数据驱动配置不变、ListChannels 也能保证声明序),新增一个 ChannelIndexFor(type) 辅助函数:

  • 内部维护 unordered_map<&table, unique_ptr<ChannelIndex>> 进程级缓存,key 是该 type 的 vector 地址(稳定),value 是对应的 unordered_map<string_view, const ChannelDef*>
  • 第一次调用某 type 时按需构造该 type 的 lookup map;之后命中既存条目
  • mutex 保护首次插入,稳态读路径只走 find
  • string_view 直接引用 ChannelDef::channel 的静态字符串字面量,生命周期与进程同寿

FindChannel 改成查这个 map:单次查询从 O(N) 字符串比对降到 O(1) 哈希。所有反射 API 入口(Get / Set / Reset / IsAnimatable / RequiresLayout / ChannelExists)共享这条 fast path。

Comment thread src/pagx/runtime/PAGComposition.cpp Outdated
}
// Newly added layer: build its tgfx subtree into this binding and wrap it in a runtime node.
if (layer->composition != nullptr) {
std::unordered_set<const Composition*> visited = {};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nit] syncChildren 用空 visited 调用 MakeChild,丢失 ancestor cycle 检测的第一层

这里 std::unordered_set<const Composition*> visited = {}; 是空的本地集合。原始 buildChildren 路径上 visited 是从根传下来的、覆盖完整 ancestor 链;但通过 syncChildren 新增的 layer 失去了这层保护。

后果:若新增层引用的 composition 已经存在于当前 PAGComposition 的祖先链上(如 Cself → C1 → 新增层指向 Cself),第一层 MakeChild 检测不到(visited 为空),但进入 MakeChild 后 visited.insert(Cself) 再递归会捕获——所以不会无限递归或崩溃,代价是多分配一个 PAGComposition 对象。仍是真实的语义降级。

建议:让 syncChildren 接收/构造完整的 ancestor 路径(从 refreshNodes 调用栈传入),覆盖完整祖先链。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同,已修。

PAGComposition::refreshNodessyncChildren 都加了 unordered_set<const Composition*>& visited 参数,承担与 buildChildren 相同的 ancestor cycle 检测语义:

  • PAGScene::onNodesChanged 入口构造空 visited(root composition 无源 Composition)后传入
  • refreshNodes 递归进 child composition 前把 child->node->composition 插入 visited,返回时只 erase 本次插入的(insert(...).second 标志位),保护 sibling 共享的下游 composition 不被误丢
  • syncChildren 直接把 visited 转发给 MakeChild

这样新增的 layer 如果引用回当前 composition 或任一祖先,在 MakeChild 入口就被 visited 检测到,不再多分配一次 PAGComposition 才在下一层捕获。


// Resets the timelines of this composition and all descendant compositions. Called when an edit
// touches a timeline node, rebuilding the whole timeline tree rather than patching it in place.
void resetTimelines();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nit] refreshNodes / syncChildren / spawnTimelines / resetTimelines 应改为 private

这 4 个新增方法当前是 protected,但仓库中 pagx::PAGComposition 没有任何子类(同名 pag::PAGComposition 是不同命名空间的旧 SDK 类型,不相关)。注释也明确写了 Called by PAGScene::onNodesChanged 等——它们是 friend 内部协作的实现细节,不是给子类继承的扩展点。

protected 暴露了不必要的扩展面,提高了未来误用风险。建议改为 private,已有 friend class PAGScene 足以访问。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同,已改 private。

refreshNodes / syncChildren / spawnTimelines / resetTimelines 这 4 个都是 PAGScene/PAGComposition 内部协作的实现细节,没有"给子类继承的扩展点"语义;改成 private 后由现有的 friend class PAGScene 提供必要访问,扩展面收紧。

MakeChild / buildChildren / findChildForLayer 仍保留在 protected:它们是构造或 hit-test 路径上 PAGComposition 自身递归的方法,未来若有子类做 hit-test 增强还可能依赖;这次没动它们,避免动到不在 review 关注范围内的访问性。

* re-measured on the next layout pass. Authored inputs (width/height, constraints, percent sizes)
* are left unchanged. Used when re-running layout on an already-laid-out document after edits.
*/
void resetLayout();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nit] resetLayout() 注释缺调用时机说明

注释说『Used when re-running layout on an already-laid-out document after edits』,但没说明:

  • PAGXDocument::applyLayout 在 reset 分支自动调用,外部用户不需直接调用
  • 还是用户必须在某些场景下主动调用?忘记调用的后果是什么?

PAGXDocument.h:147-149 已经描述了 applyLayout 的 reset 行为,但 resetLayout() 作为 public 方法的文档应该自洽。建议补充一行说明调用方与时机,例如:

Called automatically by PAGXDocument::applyLayout when re-running layout on an already laid-out document; external callers normally don't invoke this directly.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

认同,已补。

resetLayout() 注释加了一段说明:

  • PAGXDocument::applyLayout 自动调用(notifyChangelayoutChanged=true 时会触发)
  • 外部用户通常不需要直接调用
  • 如果有代码路径绕开 applyLayout 自己跑布局,调用 resetLayout 后必须紧跟一次布局,否则节点会处在半初始化的瞬态

Comment thread src/renderer/LayerBuilder.cpp Outdated
return;
}
bool stillReferenced = false;
_result.binding.forEachBoundNode([&](const Node* node) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Major - Follow-up of #2] 修复方式违反项目编码规范:禁用 lambda

感谢消除 boundNodes() 的 vector 拷贝/分配。但新引入的 forEachBoundNode([&](const Node* node) { ... }) 直接使用了 lambda 表达式,违反了项目规范 .codebuddy/rules/Code.md 第 17 行:

避免 lambda 表达式,改用显式方法或函数

本文件 line 415 和 line 453 两处都是同样的违规。

建议两选一:

  1. RuntimeBinding 暴露迭代器:增加 begin()/end() 转发到内部 targets map,调用方用普通 for 循环,比 lambda 更直观、零开销且符合规范。
  2. 下推判定逻辑:把 unbindColorSourceIfUnreferenced 里需要的判定做成 RuntimeBinding 的成员函数,例如:
    bool isColorSourceReferencedExceptBy(const ColorSource* color, const Element* exclude) const;
    bool isImageReferencedExceptBy(const Image* image, const ImagePattern* exclude) const;
    它们直接遍历内部 map,调用方写法变成 if (binding.isColorSourceReferencedExceptBy(color, excludedOwner)) return;,代码反而更清晰。

另外提醒:本评论只针对 lambda 违规。Comment #2 中提到的 O(N×M) 核心算法问题仍然存在——每个被移除的 painter 都要扫一次完整的 binding map。彻底解决方案是维护反向索引 unordered_map<const ColorSource*, std::vector<const Element*>>(painter→users),结构编辑时增量维护,移除时直接 O(1) 查询有无残留引用,无需任何遍历。是否在本 PR 内做这个改动可以由你权衡。

Hparty added 13 commits June 16, 2026 16:03
…nels by name.

(cherry picked from commit 67ab9e4d89bf7daf3ea061c9c21368bb9d5ab688)
…e channels via a subclassed runtime target.

(cherry picked from commit b2f0fe1f4b1565bc818548fa6184fcc2f3de7d81)
… registry consistency test.

(cherry picked from commit c453d7ee78f6278ef060322ec97eccf73eaa154c)
Hparty added 24 commits June 16, 2026 16:03
…se ownsNode add ChannelExists ListChannels and fix matrix rotation shortest arc.
@Hparty Hparty force-pushed the feature/partyhuang_pagx_notifyChange branch from b408daa to d8705dd Compare June 16, 2026 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants