Skip to content

PAGLayer children/advance/apply, notifyChange resolution, and performance improvements.#3512

Merged
shlzxjp merged 23 commits into
mainfrom
feature/partyhuang_paglayer_children
Jun 18, 2026
Merged

PAGLayer children/advance/apply, notifyChange resolution, and performance improvements.#3512
shlzxjp merged 23 commits into
mainfrom
feature/partyhuang_paglayer_children

Conversation

@Hparty

@Hparty Hparty commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

将 children、advance()、apply() 从 PAGComposition 上移到 PAGLayer 基类,消除 plain layer 的 promotion 升格逻辑。构建 notifyChange 的内容节点解析管线(findLayerForContentNode、contentReferencesNode),覆盖全部节点类型。ownsNode 改为 O(1),plain 容器改用增量同步,引入 forEachComposition 虚遍历。

PAGLayer children

  • children、advance()、apply() 从 PAGComposition 上移到 PAGLayer 基类
  • plain layer 始终使用 VectorLayer,消除 runtimeLayer 的 promotion 切换
  • forEachComposition 虚遍历替代各类 InDescendants 特化遍历

notifyChange 内容解析

  • findLayerForContentNode 返回所有引用节点的 Layer(支持共享节点)
  • contentReferencesNode 覆盖 Fill、Stroke、ImagePattern、Gradient、Group、TextBox、Path::data、TextPath::path
  • ownsNode 通过 nodeSet 索引升级为 O(1)
  • filter/style 按顶层指针匹配(color 字段为值类型,无需递归)
  • Document 节点检测放在 foreign 节点检测之后,避免外部文档被误判
  • 无法解析的 content node 通过 LOGE 输出而非静默丢弃

性能

  • refreshPlainContainerChildren 改为增量同步(existing map + kept set),不再全量重建
  • refreshNodes 的 dirtySet 在入口构建一次作为参数传递,避免递归重建
  • ownsNode O(N) → O(1),通过 unordered_set 索引
  • notifyChange 注释补充性能提示

修复

  • refreshPlainContainerChildren 清空 children 时正确清理 tgfx tree
  • resetTimelines 穿透 PAGLayer 容器递归
  • BuildChildren 完全委托给 BuildChildLayer,消除三处重复逻辑
  • 移除死代码,回退无效的重构尝试
  • 恢复被删除的算法注释(cycle guard、reorder、RefreshLayerInPlace)
  • 测试辅助函数移除 lambda(VerifyLayerConsistency 静态函数)

测试

  • plain 容器 children 增删清空及一致性校验
  • 嵌套 composition 重置回归测试
  • 全量 SVG 往返验证(AssertSceneConsistent)

@codecov-commenter

codecov-commenter commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.66942% with 75 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.04%. Comparing base (6e7b4b6) to head (8ac0456).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/pagx/PAGXDocument.cpp 60.00% 19 Missing and 11 partials ⚠️
src/pagx/runtime/PAGComposition.cpp 74.75% 8 Missing and 18 partials ⚠️
test/src/PAGXTest.cpp 97.83% 6 Missing and 5 partials ⚠️
src/pagx/PAGScene.cpp 73.33% 1 Missing and 3 partials ⚠️
src/pagx/PAGLayer.cpp 83.33% 0 Missing and 3 partials ⚠️
include/pagx/PAGXDocument.h 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3512      +/-   ##
==========================================
+ Coverage   80.93%   81.04%   +0.11%     
==========================================
  Files         625      626       +1     
  Lines       70517    71155     +638     
  Branches    20747    20814      +67     
==========================================
+ Hits        57074    57669     +595     
- Misses       9331     9354      +23     
- Partials     4112     4132      +20     

☔ 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.

static_cast<PAGComposition*>(child.get())->refreshNodes(dirtyNodes, visited);
} else if (!child->children.empty()) {
refreshPlainContainerChildren(child.get(), dirtyNodes, 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.

[P0 高危] 嵌套 plain 容器中孙级 plain 层的 tgfx 实例切换未被处理。

上方 refreshNodes 第 224-231 行的 runtimeLayer 同步循环只遍历 this->children(直接子节点),refreshPlainContainerChildren 里也未对 plain 后代做相同的"binding 取最新 → 替换 runtimeLayer"操作。

复现路径:嵌套 Container -> ChildContainer -> LeafnotifyChange({Leaf, layoutChanged}),Leaf 第一次获得 contents 触发 promote → 孙级 PAGLayer::runtimeLayer 永久指向陈旧实例,hit-test/getBounds/getGlobalMatrix 全部失效。

建议在递归处理 plain 容器时同步刷新所有 plain 后代的 runtimeLayer,并联动更新 layerRegistry(参见 P0-1 评论)。

Comment thread include/pagx/PAGLayer.h Outdated

const Layer* node = nullptr;
std::shared_ptr<tgfx::Layer> runtimeLayer = nullptr;
std::vector<std::shared_ptr<PAGLayer>> children = {};

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.

[P1 封装性] node/runtimeLayer/children 改为 public 破坏不变量。

这三个字段与 binding、tgfx tree、layerRegistry 三处状态强耦合,外部直接 push_back/clear/swap 会立即破坏不变量(tgfx 树不同步、binding 残留、registry 陈旧)。

按项目规范 Test.md,测试代码已可通过编译参数访问 private 成员,没必要为测试把字段公开。

建议改回 protected/private;如确需外部只读,提供 const accessor:

const std::vector<std::shared_ptr<PAGLayer>>& getChildren() const { return children; }

Comment thread src/pagx/runtime/PAGComposition.cpp Outdated
// would overwrite runtimeLayer with the slot and break hit-testing, bounds and nested re-attach.
for (auto& child : children) {
if (child != nullptr && child->node != nullptr && child->layerType() == LayerType::Layer) {
auto refreshed = binding->get<tgfx::Layer>(child->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.

[P0 高危] runtimeLayer 替换后 layerRegistry 未同步,会导致 hit-test 命中失败。

本循环(226-231 行)只更新了 child->runtimeLayer = refreshed;,但 PAGScene::layerRegistry 中:

  1. 旧 tgfx 指针仍指向该 PAGLayer(陈旧映射,新 tgfx 同地址复用时会返回错误的 PAGLayer)
  2. 新 tgfx 指针未注册

RefreshLayerInPlace 确实会替换 plain 层的 tgfx 实例(plain 层获得 contents 时会被升格为 VectorLayer,被删的旧注释也曾说明这一点)。后续 PAGScene::getLayersUnderPoint 拿到的 hit tgfx 层是新实例,registry 找不到,该层无法被命中。

建议替换时同步更新 registry:

if (refreshed != nullptr && refreshed != child->runtimeLayer) {
  auto scene = rootScene.lock();
  if (scene != nullptr) {
    scene->layerRegistry.erase(child->runtimeLayer.get());
    scene->layerRegistry[refreshed.get()] = child.get();
  }
  child->runtimeLayer = refreshed;
}

Comment thread src/pagx/PAGLayer.cpp

void PAGLayer::advance(int64_t deltaMicroseconds) {
for (auto& child : children) {
child->advance(deltaMicroseconds);

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.

[P2·防御性] advance/apply 默认实现未做 nullptr 校验。

这里直接 child->advance(...),而下方的 forEachComposition 显式做了 if (child != nullptr)。两者不一致。

虽然当前所有插入路径(BuildChildren/syncChildren/refreshPlainContainerChildren)都过滤了 nullptr,且 children 字段已被改为 public(参见 PAGLayer.h 评论),外部直接 push_back(nullptr) 时会立即崩溃。建议统一加上空指针校验:

for (auto& child : children) {
  if (child != nullptr) {
    child->advance(deltaMicroseconds);
  }
}

Comment thread src/pagx/runtime/PAGComposition.cpp Outdated
if (child == nullptr) {
continue;
}
if (!layer->children.empty()) {

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.

[P2 重复] "plain 层带 children 子树 attach"逻辑在三处复制粘贴。

完全相同的"递归 BuildChildren + 把 nestedChild->runtimeLayer addChild 进 child->runtimeLayer"模式出现在:

  • 此处 BuildChildren 第 184-191 行
  • syncChildren 第 299-306 行
  • refreshPlainContainerChildren 第 353-358 行

建议提取为辅助函数(如 AttachNestedChildrenToContainer(child, scene, visited)),避免后续修复其中一处而漏改其他两处。

Comment thread src/pagx/runtime/PAGComposition.cpp Outdated
}

void PAGComposition::resetTimelines() {
spawnTimelinesFromScene();

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.

[P2] resetTimelines 首行调用冗余。

下一行 forEachComposition(ResetCompositionTimelines) 在 PAGComposition override 中会先访问后代再 visit this,所以 root composition 自己也会被 visitor 调用一次 spawnTimelinesFromScene。第一行的显式调用是冗余的(root composition node==nullptrspawnTimelines 会 early return,无副作用,但语义上的重复值得清理)。

建议简化为:

void PAGComposition::resetTimelines() {
  forEachComposition(ResetCompositionTimelines);
}

并把 forEachComposition 重命名为 forEachCompositionIncludingSelf 或在头注释明确 PAGComposition override 包含自身的语义。

Comment thread src/pagx/runtime/PAGComposition.cpp Outdated
continue;
}
newChildren.push_back(std::shared_ptr<PAGLayer>(new PAGLayer(layer, layerRuntime, scene)));
auto child = std::shared_ptr<PAGLayer>(new PAGLayer(layer, layerRuntime, scene));

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.

[P2] MakeChildLayer 工厂使用不一致。

本 PR 在 PAGComposition.h 新引入了静态工厂 MakeChildLayer(PAGComposition.cpp:146),目的应是统一 plain PAGLayer 的构造路径。但 syncChildren 这里仍直接 new PAGLayer(...)

建议二选一:

  • 删除 MakeChildLayer(仅是单行包装,没必要保留)
  • 或改用 MakeChildLayer 保持构造入口一致

// by sibling layers is not mistaken for a cycle. The external-file chain guard in
// PAGXDocument::LoadExternalComposition does not cover same-document @id references.
auto* sourceComposition = ownerLayer->composition;
if (visited.find(sourceComposition) != visited.end()) {

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.

[P3·可维护性] 被删除的"为什么"型算法注释建议恢复。

本 PR 一并删除了多处非显而易见的算法选择说明,按项目规范 Code.md("对非显而易见的算法选择、特殊边界处理、或绕过已知问题的 workaround 需加注释说明原因")应保留:

  1. MakeChild 内 cycle guard 的设计意图(visited 作为路径栈、为什么 sibling 复用不算 cycle、external-file chain guard 不覆盖同文档 @id 引用)
  2. refreshNodes 内 RefreshLayerInPlace 可能 swap tgfx 实例的说明(与 P0-1 直接相关,恢复后能避免下次重构再次踩坑)
  3. syncChildren 中"detach 的是 slot 而非 runtimeLayer"的原因
  4. 第 327 行附近"reorder 时为什么按 source 顺序 addChild 即可"

这些"为什么"型注释删掉后,后续维护者很容易在重构时破坏隐含约束。建议恢复关键的几条。

Comment thread src/pagx/runtime/PAGComposition.cpp Outdated
if (dirty) {
auto scene = rootScene.lock();
if (scene != nullptr) {
container->children.clear();

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.

[P1] 全量重建丢失复用机会 + binding 残留风险。

这里容器一旦 dirty 就 clear() + removeChildren() + 全量重建,与 syncChildren 通过 existing map 复用现有子节点的策略不一致:

  1. 失去复用机会,每次 dirty 都全量重建 children 子树(即使大部分 source layer 没变)
  2. 旧 PAGLayer 析构时只清 layerRegistry不会binding;后续 BuildChildrenbinding->get(layer) != nullptr 又会跳过重建,相当于旧 binding 条目被重新绑到新 PAGLayer 上——表面正常但语义混乱,旧条目被遗弃后没有显式 binding->remove,存在累积风险

建议复用 syncChildren 的 reconcile 逻辑(按 source layer 节点匹配复用与移除),或显式说明 plain 容器是有意采用"全量重建"的简化策略,并补上对消失 source 的 binding->remove

[P1·性能] dirtySet 重复构建。
上方第 338 行 std::unordered_set<const Node*> dirtySet(...) 在每次递归调用时都重新构建一次。建议把 dirtySet 提升为参数(与 visited 类似)传入。

Comment thread include/pagx/PAGLayer.h
* operations that must reach every composition in the tree, including those nested under plain
* PAGLayer containers.
*/
virtual void forEachComposition(void (*visitor)(PAGComposition*));

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.

[P3·命名] forEachComposition 在两个类中语义不同。

  • PAGLayer::forEachComposition:仅访问后代(不含 self)
  • PAGComposition::forEachComposition(override):访问后代后再 visit self

读者必须看实现才知道 PAGComposition 的 override 会包含自身。建议:

  • 命名上区分(如 forEachCompositionIncludingSelf),或
  • 在头注释里明确写出"PAGComposition override 会在访问完后代后再访问自身"

避免后续添加新 override 时误以为只是简单递归。

@Hparty Hparty changed the title Eliminate composition promotion for layers with children Support PAGLayer children in plain containers for nested layer composition. Jun 17, 2026
@Hparty Hparty changed the title Support PAGLayer children in plain containers for nested layer composition. PAGLayer supports adding child layers directly. Jun 17, 2026

@Hparty Hparty left a comment

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.

全部评论已处理,逐一回复:

P0 (孙级 plain 层 tgfx 实例切换):已修复。最终方案通过始终使用 VectorLayer 消除了 promotion,runtimeLayer 不再变化,不需要 sync。

P1 (字段封装):node/runtimeLayer/children 已移至 protected,添加了 getNode()/getChildren() const accessor。

P0 (layerRegistry 未同步):已修复。syncChildren 和 refreshPlainContainerChildren 都会同步更新 layerRegistry。

P2 (advance/apply nullptr):已改为 DEBUG_ASSERT。项目规范要求此处使用断言而非运行时检查。

P2 (三处重复):BuildChildren 已完全委托给 BuildChildLayer,消除所有重复。

P2 (resetTimelines 冗余):已删除冗余的 spawnTimelinesFromScene() 调用。

P2 (MakeChildLayer 不一致):MakeChildLayer 已删除,统一使用 BuildChildLayer 工厂。

P3 (被删除的注释):cycle guard、reorder、RefreshLayerInPlace 的算法注释均已恢复。

P1 (全量重建):refreshPlainContainerChildren 已改为增量同步(existing map + kept set)。

P3 (forEachComposition 语义):PAGLayer.h 注释已详细说明两个类的语义差异。

Comment thread src/pagx/PAGXDocument.cpp
return layer;
}
}
for (auto* filter : layer->filters) {

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.

[NEW-P0-1 高危] findLayerForContentNode 只返回首个匹配 Layer,共享节点漏更新。

这里 return layer 在找到第一个引用 target 的 Layer 时就返回。但 PAGX 明确支持共享节点:

  • LayerBuilder.cpp:1042colorSourceUsers 反向索引
  • 测试 GroupInnerFillRemovedUnbindsSharedColor 等都依赖共享 SolidColor / Gradient / Image

复现路径:两个 Fill 共享同一 SolidColor,mutate solid->colornotifyChange({solid})findLayerForContentNode 只返回首个匹配 Layer → 只有一个 Layer 被 refresh,其他 Layer 保留旧颜色。

该方法签名 const Layer*(单数返回值)本身就暗示了设计缺陷。

修复建议:改为返回 std::vector<const Layer*> 收集所有引用 target 的 Layer,上层 notifyChange 把它们全部加入 layerDirty

std::vector<const Layer*> findLayersForContentNode(...) {
  std::vector<const Layer*> result;
  for (auto& node : allNodes) {
    if (node->nodeType() != NodeType::Layer) continue;
    auto* layer = static_cast<const Layer*>(node.get());
    bool matched = false;
    for (auto* elem : layer->contents) {
      if (elem == target || contentReferencesNode(elem, target)) { matched = true; break; }
    }
    // ... filters/styles 同样
    if (matched) result.push_back(layer);
  }
  return result;
}

并补充共享节点的回归测试。

Comment thread src/pagx/PAGXDocument.cpp
if (filter == target) {
return layer;
}
}

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.

[NEW-P0-2 高危] Filter / Style 子引用未递归解析,子节点 mutate 后被静默丢弃。

这里 for (auto* filter : layer->filters) 只比较顶层 filter 指针,未递归进入 filter 的引用字段;下方 styles 同理。

问题

  • DropShadowFilter / DropShadowStyle / InnerShadowFilter 等都引用 ColorSource(例如 dropShadow->color = solid)。当子节点 mutate 后调 notifyChange({solid})
    1. 进入 Layer,遍历 contents → 找不到(在 filters 里)
    2. 遍历 filters → 只比顶层指针,filter == solid 不成立
    3. 返回 nullptr → 节点被静默 drop(PAGXDocument.cpp:462-465 分支)
    4. 实际效果:阴影颜色不会更新
  • PAGXDocument.h:180 注释明确承诺 "Content nodes (Image, SolidColor, Gradient, ColorStop, Fill, Stroke, Group, filters, styles, etc.)" 都接受 — 与实际行为不一致
  • 测试 NotifyChangeFilter(test/src/PAGXTest.cpp:10431)只验证了 BlurFilter 自身被传入(这种情况确实命中顶层比较),未覆盖"filter 内嵌 ColorSource"场景

修复建议:把 filter/style 也走 contentReferencesNode 风格的递归,覆盖 DropShadowFilter::colorInnerShadowFilter::colorBlendFilter::colorDropShadowStyle::colorInnerShadowStyle::color 等所有引用字段。并补充对应测试用例。

Comment thread src/pagx/PAGXDocument.cpp Outdated
if (fill->color == target) {
return true;
}
if (fill->color != nullptr && contentReferencesNode(fill->color, target)) {

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.

[NEW-P1-1] contentReferencesNode 仅覆盖 Fill / Stroke / ImagePattern / Gradient / Group,其他可作为 layer.contents 元素的节点未深度解析。

本函数和 findLayerForContentNode 一起承担"任意 content node → owning Layer"解析,但未覆盖以下节点的子引用解析:

  • TrimPathRoundCornerMergePathRepeater — 几何/修饰修改器
  • Text — 可包含 Fill/Stroke 等子结构(如 text fills)
  • TextModifierTextPathTextBoxRangeSelector — 文本动画器

这些节点本身作为 target 时可走外层 elem == target 命中(OK),但子结构 mutate 时无法解析到 owning Layer,会被静默 drop。

修复建议

  1. 确认 PAGX 实际节点结构中这些类型是否真的支持嵌套引用(如 Text 内嵌 Fill / Repeater 内嵌任意 element),如果支持则补充 switch 分支
  2. 兜底机制:解析失败时输出 LOGE 而非静默 drop,便于调用方排查

Comment thread src/pagx/PAGXDocument.cpp Outdated
type == NodeType::AnimationObject || type == NodeType::Channel) {
layerDirty.push_back(node);
} else {
auto* owningLayer = findLayerForContentNode(nodes, 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.

[NEW-P1-2 性能] 反向解析 O(N·M),大文档批量 mutate 时显著开销。

这里每个非 Layer/Timeline 类型的 dirty node 都触发一次 findLayerForContentNode,而该函数会扫描整个 nodes(document 全部节点),对每个 Layer 又遍历其 contents/filters/styles 并递归 contentReferencesNode

复杂度 ≈ O(dirty × allNodes × layerContents × refChainDepth)。

修复建议:在 PAGXDocument 上构建并维护反向索引:

std::unordered_map<const Node*, std::vector<const Layer*>> contentToLayers;
  • applyLayout() 或 layer 增删时构建/维护
  • notifyChange 直接 O(1) 查找

该问题与 NEW-P0-1(共享节点漏更新)建议合并解决:反向索引天然返回 vector<Layer*>,同时修复正确性和性能。

@@ -176,12 +176,12 @@ void PAGXView::advanceTimelines(double frameStartMs) {
if (deltaUs <= 0) {
return;
}

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.

[NEW-P1-3 工程纪律] commit message 与实际改动不符。

该改动属于 commit 0d038b830

"Fix advanceTimelines passing 0 instead of computed deltaUs to advanceAndApply."

但实际 diff 只是调换了 defaultTimeline->advanceAndApply(deltaUs)scene->advanceAndApply(deltaUs) 的调用顺序,并没有任何"传 0 vs 传 deltaUs"的改动。

按 Git.md 规范,commit message 应准确描述用户可感知的变化。建议修订为:

Drive default timeline before scene to avoid one-frame lag in nested compositions.
(具体原因应是 nested composition 内的 timeline 必须先 advance,再让 scene 把它 apply 到 tgfx 树,否则首帧表现错位)

另外,相关 commit c343307c9 的 message 只描述了 7 个修复中的 1 项(slot bypass),也应拆分或在 message 中列出所有修复点。后续提交建议遵循"一个修复一个 commit"原则。

Comment thread src/pagx/PAGScene.cpp Outdated
//
// The document node itself (NodeType::Document) triggers a full rebuild so changes to
// PAGXDocument::width/height are reflected in the root runtime layer.
for (auto* node : dirtyNodes) {

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.

[NEW-P2-1] Document 节点早返回未校验 ownership,与 foreign 检测顺序耦合。

这里把 Document 节点检测放在 foreign 检测之前。如果调用方误传了别的 document 节点(外部嵌入文档),这里会直接走 buildRuntimeTree() 而绕过 foreign 检测路径。

虽然 foreign 场景的最终处理本来就是 buildRuntimeTree()(PAGScene.cpp:271),结果碰巧一致,但语义不清晰

  • "Document 节点 → rebuild" 与 "foreign 节点 → rebuild" 是两个独立原因
  • 当前实现混合在一起,未来若 foreign 路径增加额外处理(如错误日志、scene 重注册),Document 早返回会跳过

修复建议:把 Document 早返回放到 foreign 检测之后,或在注释中明确"foreign-document node 也会命中此分支,逻辑等价":

// First handle foreign-node rebuild...
// Then handle Document-node rebuild (own doc resize):
for (auto* node : dirtyNodes) {
  if (node != nullptr && node->nodeType() == NodeType::Document) {
    buildRuntimeTree();
    return;
  }
}

Comment thread src/pagx/PAGXDocument.cpp
@@ -366,6 +490,9 @@ void PAGXDocument::notifyChange(const std::vector<Node*>& dirtyNodes, bool layou
}

bool PAGXDocument::ownsNode(const Node* node) const {

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.

[NEW-P2-2] ownsNode(this) 行为变化未在注释中说明。

本 PR 让 ownsNode 对 document 自身返回 true(line 493-495),目的是支持 notifyChange({doc}) 路径。但:

  • 原语义是"document 持有的子节点",不包括 document 本身
  • 其他调用方使用 ownsNode 做路由判断时,可能没有预期 doc 自身返回 true
  • 头文件 PAGXDocument.hownsNode 的注释没有更新这一行为变化

修复建议:在 PAGXDocument.hownsNode 注释中补充说明:

* Returns true also for the document node itself, so the document can be passed
* to notifyChange to trigger a full rebuild (e.g. on width/height changes).

避免下游调用方误解。

Comment thread src/pagx/runtime/PAGComposition.cpp Outdated
bool dirty = dirtySet.find(container->node) != dirtySet.end();
if (dirty) {
auto scene = rootScene.lock();
if (scene != nullptr && binding != nullptr) {

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.

[NEW-P2-3] binding != nullptr 校验仅在 dirty 分支内做,鲁棒性可改进。

这里把 binding != nullptr 校验放在 if (dirty) 内部。当前函数其他逻辑(line 373-382 子节点递归)虽然不直接用 binding,但未来若新增依赖 binding 的代码可能踩坑。

修复建议:把 binding != nullptr 提到函数入口的 early-return 区域,与 container == nullptr || container->node == nullptr 一起统一校验:

if (container == nullptr || container->node == nullptr || binding == nullptr) {
  return;
}

保持单一校验出口,提升后续维护安全性。

(注:当前实现在功能上是正确的,此条仅为防御性改进。)

if (layerRuntime == nullptr) {
return nullptr;
}
auto child = std::shared_ptr<PAGLayer>(new PAGLayer(layer, layerRuntime, scene));

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.

[NEW-P2-4] BuildChildLayer 入口缺 scene 校验。

本方法是 private static,但已成为类的多处共用入口(BuildChildrensyncChildrenrefreshPlainContainerChildren 都调用)。当前 scene null 校验仅在 BuildChildren 入口(line 152)做。若未来其他路径直接调用 BuildChildLayer 而绕开 BuildChildren,传入 null scene 会导致:

  • line 240 new PAGLayer(layer, layerRuntime, scene) 构造时 scene 是 weak_ptr 不会立刻报错
  • PAGLayer 构造体内 scene->layerRegistry[...] 因 scene 为 null 会跳过注册(PAGLayer.cpp:31-33 已有判空),表面"无副作用"但实际丢失了 registry 入口 → hit-test 失败

修复建议:在 BuildChildLayer 入口加防御性校验:

static std::shared_ptr<PAGLayer> BuildChildLayer(...) {
  if (layer == nullptr || binding == nullptr || scene == nullptr) {
    return nullptr;
  }
  ...
}

类似 MakeChild (line 64-66) 的风格。

* to its child list; editing an animation, animation object, or channel applies the new timeline
* data to subsequent playback.
* Reflects post-build edits to the given nodes in every scene created from this document. Layer
* handles are preserved wherever possible. Pass a container node to reflect changes to its child

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.

[NEW-P3-1 注释] notifyChange 注释承诺与实现脱节。

这里注释承诺接受 "Content nodes (Image, SolidColor, Gradient, ColorStop, Fill, Stroke, Group, filters, styles, etc.)",但实现存在两处偏差:

  1. filters / styles 子引用未递归解析(见 NEW-P0-2 评论) — 例如 notifyChange({dropShadowFilterColor}) 会被静默 drop
  2. 共享节点只更新首个引用 Layer(见 NEW-P0-1 评论) — 例如共享 SolidColor 只刷新一个 Layer
  3. "etc." 表述模糊,未明确列出全部支持类型(如 TrimPath、TextModifier、Repeater 等是否支持需要文档化)

修复建议:先修复 NEW-P0-1 / NEW-P0-2 的实际逻辑,再把注释改成精确的支持列表,避免误导调用方。建议格式:

* Supported content nodes (resolved to owning Layer):
*   - ColorSources: SolidColor, LinearGradient, RadialGradient, ...
*   - Painters/modifiers: Fill, Stroke, Group, TrimPath, ...
*   - Filters (including their nested color refs): BlurFilter, DropShadowFilter, ...
*   - Styles (including their nested color refs): DropShadowStyle, ...
* Unsupported (silently dropped, see logs):
*   - GlyphRun, Font, Glyph (layout-time data)

@Hparty Hparty left a comment

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.

已处理第二轮审查的 10 条评论,逐一回复:

NEW-P0-1(findLayerForContentNode 共享节点):已修复。改为返回所有匹配 Layer,notifyChange 中遍历所有 owning layers。

NEW-P0-2(Filter/Style 子引用):所有 filter/style 的 color 字段均为值类型(Color color = {}),不存在指向其他节点的指针引用,无需递归解析。设计上是安全的。

NEW-P1-1(未覆盖 element 类型):已添加 TextBox 支持(其 elements 字段与 Group 同理)。

NEW-P1-2(O(N·M) 性能):接受当前方案。典型文档节点数在数百级别,批量 mutate 场景极罕见。如有实际瓶颈,后续可通过正向索引(Node→Layer 映射)优化。

NEW-P1-3(commit message 不匹配):该 commit 已被 squash 淘汰,最终分支历史中不存在。

NEW-P2-1(Document 节点早返回):已修复。Document 检测移至 foreign 检测之后,外部文档不会被误判。

NEW-P2-2(ownsNode(this) 注释):已在 API 注释中补充说明。

NEW-P2-3(binding 校验位置):binding 仅在 dirty 分支内使用,当前作用域正确。若移到外部 return 会影响非 dirty 分支的递归下降路径。维持现状。

NEW-P2-4(BuildChildLayer 缺 scene 守卫):已添加 layer/binding/scene 空指针守卫。

NEW-P3-1(API 注释脱节):已修正注释,添加 TextBox 支持说明,并澄清 filter/style 的匹配方式(顶层指针匹配,非递归)。

Comment thread src/pagx/PAGXDocument.cpp Outdated
if (parent == nullptr || target == nullptr) {
return false;
}
if (parent->nodeType() == NodeType::Fill) {

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.

[R3-P1] Path::data / TextPath::path 引用的 PathData 节点 mutate 后 notifyChange 失败。

contentReferencesNode 当前覆盖 Fill / Stroke / ImagePattern / Gradient / Group / TextBox,但没有 Path / TextPath 分支

这两个节点都有 PathData* 指针引用:

  • Path::data → 引用 PathData 资源(include/pagx/nodes/Path.h
  • TextPath::path → 引用 PathData 资源(include/pagx/nodes/TextPath.h

PathData 是项目支持的共享资源节点(NodeType::PathData,注释 "A reusable path data resource"),且暴露 moveTo/lineTo/cubicTo/close 等 mutate API(用于动态形变动画或编辑器修改路径形状的场景)。

复现路径

auto pathData = doc->makeNode<PathData>("shared");
pathData->moveTo(0, 0);
pathData->lineTo(100, 100);
auto path = doc->makeNode<Path>();
path->data = pathData;
layer->contents = {path, fill};
auto scene = PAGScene::Make(doc);

// 之后 mutate 形状
pathData->reset();
pathData->moveTo(0, 0);
pathData->lineTo(50, 50);
doc->notifyChange({pathData}, /*layoutChanged=*/true);
// → findLayerForContentNode 进入 layer.contents → contentReferencesNode(path, pathData) 没有 Path 分支 → 返回 false
// → owningLayers 为空 → LOGE 输出 "content node not reachable" → 用户看到日志但渲染不更新

修复建议

if (parent->nodeType() == NodeType::Path) {
  auto* p = static_cast<const Path*>(parent);
  return p->data == target;
}
if (parent->nodeType() == NodeType::TextPath) {
  auto* p = static_cast<const TextPath*>(parent);
  return p->path == target;
}

并补充共享 PathData 的回归测试。

注:严重性比 NEW-P0-1(共享 SolidColor)略低,因为 PathData mutate 不在 SetNodeChannel 体系中,但仍是 PR 注释承诺支持的 content node 解析路径。

Comment thread src/pagx/runtime/PAGComposition.cpp Outdated

void PAGComposition::refreshNodes(const std::vector<Node*>& dirtyNodes,
std::unordered_set<const Composition*>& visited) {
std::unordered_set<const Node*> dirtySet(dirtyNodes.begin(), dirtyNodes.end());

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.

[R3-P3 性能·建议] refreshNodes 递归调用时仍重建 dirtySet。

refreshNodes 是递归方法(line 206 进入 child composition;line 381 在 refreshPlainContainerChildren 内进入 child composition),每次递归都重建一次 dirtySet。

之前的评审已经把 refreshPlainContainerChildren 的 dirtySet 提到参数(commit c343307c9),但 refreshNodes 自身递归路径漏掉了同样的优化。

实际影响有限(dirtyNodes 通常个位数、递归深度通常个位数),属优化建议。

修复建议:把 dirtySet 也提到参数,与 refreshPlainContainerChildren 签名保持一致:

void refreshNodes(const std::vector<Node*>& dirtyNodes,
                  std::unordered_set<const Composition*>& visited,
                  const std::unordered_set<const Node*>& dirtySet);

入口(PAGScene::onNodesChanged)构建一次后向下传递。

Comment thread test/src/PAGXTest.cpp Outdated
return;
}
std::function<void(const std::shared_ptr<pagx::PAGLayer>&)> verifyLayer;
verifyLayer = [&](const std::shared_ptr<pagx::PAGLayer>& layer) {

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.

[R3-P3 风格·建议] AssertSceneConsistent 内的递归 lambda 与项目规范不一致。

项目规范 Code.md 明确写:"避免 lambda 表达式,改用显式方法或函数"。

这里 verifyLayerstd::function + 递归 lambda 的组合。虽然测试代码里此前已存在一处 lambda 先例(line 9573 auto sample = [&](...)),但既然规范明确禁止,新增代码可优先采用显式静态函数:

static void VerifyLayerConsistency(const std::shared_ptr<pagx::PAGScene>& scene,
                                    pagx::RuntimeBinding* binding,
                                    const std::shared_ptr<pagx::PAGLayer>& layer) {
  if (layer == nullptr) return;
  auto* node = layer->getNode();
  if (node != nullptr) {
    auto bound = binding->get<tgfx::Layer>(node);
    EXPECT_EQ(bound.get(), layer->runtimeLayer.get())
        << "binding mismatch: promotion sync may not have run for this layer";
  }
  if (layer->runtimeLayer != nullptr) {
    auto it = scene->layerRegistry.find(layer->runtimeLayer.get());
    EXPECT_NE(it, scene->layerRegistry.end())
        << "layerRegistry missing entry: hit-test will miss this layer";
    if (it != scene->layerRegistry.end()) {
      EXPECT_EQ(it->second, layer.get())
          << "layerRegistry maps to wrong PAGLayer: hit-test returns incorrect layer";
    }
  }
  for (auto& child : layer->getChildren()) {
    VerifyLayerConsistency(scene, binding, child);
  }
}

static void AssertSceneConsistent(const std::shared_ptr<pagx::PAGScene>& scene) {
  if (scene == nullptr || scene->rootComposition() == nullptr) return;
  auto* binding = scene->mutableBinding();
  if (binding == nullptr) return;
  VerifyLayerConsistency(scene, binding, scene->rootComposition());
}

属于建议性改进,不阻塞合并。

Comment thread src/pagx/PAGXDocument.cpp Outdated
}
if (parent->nodeType() == NodeType::Fill) {
auto* fill = static_cast<const Fill*>(parent);
if (fill->color == target) {

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.

[R3-P3 风格·建议] contentReferencesNode 用 if 链 + fall-through,建议改 switch 或 else if 链。

当前每个分支都用独立 if (parent->nodeType() == ...),节点类型守卫确保了 fall-through 不会误匹配,但:

  • 各分支退出方式不一致:Fill/Stroke/Group/TextBox 是 fall-through,ImagePattern 早 return,Gradient 显式 return false
  • 易让维护者误读为"匹配 Fill 后还会继续检查 Stroke",实际不会

修复建议:用 switch 统一:

bool contentReferencesNode(const Node* parent, const Node* target) {
  if (parent == nullptr || target == nullptr) {
    return false;
  }
  switch (parent->nodeType()) {
    case NodeType::Fill: {
      auto* fill = static_cast<const Fill*>(parent);
      return fill->color == target ||
             (fill->color != nullptr && contentReferencesNode(fill->color, target));
    }
    case NodeType::Stroke: {
      auto* stroke = static_cast<const Stroke*>(parent);
      return stroke->color == target ||
             (stroke->color != nullptr && contentReferencesNode(stroke->color, target));
    }
    case NodeType::ImagePattern: {
      return static_cast<const ImagePattern*>(parent)->image == target;
    }
    case NodeType::LinearGradient:
    case NodeType::RadialGradient:
    case NodeType::ConicGradient:
    case NodeType::DiamondGradient: {
      auto* gradient = static_cast<const Gradient*>(parent);
      for (auto* stop : gradient->colorStops) {
        if (stop == target) return true;
      }
      return false;
    }
    case NodeType::Group: {
      auto* group = static_cast<const Group*>(parent);
      for (auto* elem : group->elements) {
        if (elem == target || contentReferencesNode(elem, target)) return true;
      }
      return false;
    }
    case NodeType::TextBox: {
      auto* textBox = static_cast<const TextBox*>(parent);
      for (auto* elem : textBox->elements) {
        if (elem == target || contentReferencesNode(elem, target)) return true;
      }
      return false;
    }
    default: return false;
  }
}

属于代码风格优化,不阻塞合并。

Comment thread src/pagx/PAGXDocument.cpp
@@ -366,6 +515,9 @@ void PAGXDocument::notifyChange(const std::vector<Node*>& dirtyNodes, bool layou
}

bool PAGXDocument::ownsNode(const Node* node) const {

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.

[R3-P2 性能] ownsNode O(N) 线性扫描是 notifyChange 入口的热点。

bool PAGXDocument::ownsNode(const Node* node) const {
  if (node == this) return true;
  for (auto& ownedNode : nodes) {  // O(N)
    if (ownedNode.get() == node) return true;
  }
  return false;
}

notifyChange 入口对每个 dirty node 调一次 → 整体 O(dirty × allNodes)。后续 findLayerForContentNode 又做一次 O(allNodes × layerContents) 扫描。批量编辑大文档时性能瓶颈明显。

修复建议:在 PAGXDocument 上维护反向索引:

std::unordered_set<const Node*> nodeSet = {};
  • makeNode<T>() 内 push 节点的同时插入 nodeSet
  • 节点删除路径同步移除
  • ownsNode 改为 O(1)

配合后续 findLayerForContentNode 的反向索引(如 unordered_map<const Node*, vector<const Layer*>> contentToLayers),可把 notifyChange 路径整体复杂度降到 O(dirty)。

* to its child list; editing an animation, animation object, or channel applies the new timeline
* data to subsequent playback.
* Reflects post-build edits to the given nodes in every scene created from this document. Layer
* handles are preserved wherever possible. Pass a container node to reflect changes to its child

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.

[R3-P3 文档·建议] notifyChange 注释建议补充性能提示。

当前注释已经较完整,描述了支持的 content node 类型和 filter/style 的匹配方式。但缺少性能建议:传 Layer 直接给 notifyChange 比传 content node 更高效(避免 findLayerForContentNode 的 O(N) 扫描)。

修复建议:在注释末尾加一段 performance note:

* Performance note: For best efficiency, prefer passing the owning Layer directly
* when known. Resolving a content node to its owning Layer requires scanning all
* document nodes (O(N)). For batch edits to a known Layer, pass the Layer once
* instead of every modified content child.

属文档优化,不阻塞。

…Node, dirtySet reuse, switch refactor, lambda removal, and docs update.
@Hparty Hparty changed the title PAGLayer supports adding child layers directly. PAGLayer children/advance/apply, notifyChange resolution, and performance improvements. Jun 18, 2026
@shlzxjp shlzxjp merged commit 5fbd2f1 into main Jun 18, 2026
9 checks passed
@shlzxjp shlzxjp deleted the feature/partyhuang_paglayer_children branch June 18, 2026 08:20
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