Add checkerboard grid background support for pagx-viewer#3520
Add checkerboard grid background support for pagx-viewer#3520jinwuwu001 wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3520 +/- ##
=======================================
Coverage 81.06% 81.06%
=======================================
Files 626 626
Lines 71226 71310 +84
Branches 20826 20837 +11
=======================================
+ Hits 57737 57811 +74
- Misses 9352 9353 +1
- Partials 4137 4146 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| canvas->clear(); | ||
| } | ||
| _backgroundLayer->draw(canvas); | ||
| displayList->render(tgfxSurface.get(), false); |
There was a problem hiding this comment.
autoClear=false 与 backgroundLayer 的语义冲突。
PAGScene::draw() 的契约（PAGScene.h:99-106）规定：autoClear=false 时"overlay the scene content on top of the existing surface content"，即在已有内容之上叠加。
但本分支即使 autoClear=false 也会执行 _backgroundLayer->draw(canvas)，背景层会盖在 surface 已有内容之上，再绘制 scene 内容。这与"叠加 scene 内容"的语义冲突——backgroundLayer 反倒会把用户已有的 surface 内容遮挡。
建议在 backgroundLayer 路径下，当 autoClear=false 时跳过背景层绘制；或在 setBackgroundLayer 的 API 注释里明确说明该 API 与 autoClear=false 的交互行为。
| scene->getDisplayOptions()->setBackgroundColor({0, 0, 0, 0}); | ||
| auto density = currentCanvasWidth > 0 ? static_cast<float>(lastSurfaceWidth) / | ||
| static_cast<float>(currentCanvasWidth) | ||
| : 1.0f; |
There was a problem hiding this comment.
density 计算实际恒为 1.0，HiDPI 适配未生效。
第 327 行 syncSurfaceSize(currentCanvasWidth, currentCanvasHeight) 执行后，syncSurfaceSize 内部会执行 lastSurfaceWidth = canvasWidth（即把 currentCanvasWidth 赋给 lastSurfaceWidth）。因此这里 lastSurfaceWidth / currentCanvasWidth 永远是 1.0。
从命名推测原意应是 devicePixelRatio，但当前文件没有任何地方读取 DPR，emscripten_get_canvas_element_size 返回的本来就是 canvas 的像素分辨率而非 CSS 像素，分子分母同源。
结果是 GridBackgroundLayer 里 tileSize = 32 * density 始终等于 32 个物理像素，HiDPI 屏（DPR=2）下棋盘格视觉只有 16 CSS 像素，与"在不同屏幕看起来一致"的设计意图相悖。
建议从 JS 端通过 window.devicePixelRatio 把真实 DPR 传入，或在 GridBackgroundLayer 内部读取 DPR。
| int bgHeight = lastSurfaceHeight; | ||
| if (!backgroundLayer || bgWidth != lastBackgroundWidth || bgHeight != lastBackgroundHeight || | ||
| std::abs(density - lastBackgroundDensity) > 0.001f) { | ||
| backgroundLayer = GridBackgroundLayer::Make(bgWidth, bgHeight, density); |
There was a problem hiding this comment.
GridBackgroundLayer 的 density 参数设计意图与现状不一致。
GridBackground.cpp 里 tileSize = 32 * density 的注释明确说"Use fixed logical size (32px) so the grid looks the same on all screens"，意图是按 DPR 缩放。但结合上面对 density 计算的评论（恒为 1.0），这个设计意图实际上从未生效。
建议和 density 计算的修复（见上面的评论）一起改：要么传入真实 DPR，要么干脆去掉 density 参数并由 Layer 内部读取 DPR。
|
|
||
| void PAGScene::setBackgroundLayer(std::shared_ptr<tgfx::Layer> layer) { | ||
| _backgroundLayer = std::move(layer); | ||
| } |
There was a problem hiding this comment.
新增公开 API 缺少单元测试覆盖。
Codecov 报告本 PR 行覆盖率仅 10%，test/src/ 下也没有 setBackgroundLayer 的任何用例。按项目 Test 规范，新增公开 API 应有 GTest 覆盖。
建议至少补充：
- 不设置 backgroundLayer 时
draw()行为正常； - 设置背景层后渲染正确（可用截图基准对比）；
- 传 nullptr 能恢复无背景层状态；
- 与
DisplayOptions::setBackgroundColor的交互行为（与上面 请问能否在android或者Ios上面，导出成视频格式。 #1 的评论相关）。
| * in surface coordinates (not affected by zoomScale or contentOffset) after the surface is | ||
| * cleared and before the display list is rendered. Pass nullptr to remove a previously set | ||
| * background layer. | ||
| * @param layer the layer to draw as background, or nullptr to remove. |
There was a problem hiding this comment.
API 注释建议补充 tgfx::Layer 的使用约束。
按 tgfx 文档（tgfx/layers/Layer.h 中 draw() 注释）：
"The layer is drawn in its local space without applying its own matrix, alpha, blend mode, visible, scrollRect, or mask."
这意味着用户如果对传入的 background layer 设置了 alpha / 变换 / mask 等属性，这些属性都不会生效。这是个调用者很容易踩的坑（例如想给背景层加 alpha 让其半透明）。
建议在此注释里明确补充："The layer is drawn directly in surface local space; its own matrix, alpha, blendMode, visible, scrollRect and mask properties have no effect — wrap them inside the layers content if needed."
| * background layer. | ||
| * @param layer the layer to draw as background, or nullptr to remove. | ||
| */ | ||
| void setBackgroundLayer(std::shared_ptr<tgfx::Layer> layer); |
There was a problem hiding this comment.
setBackgroundLayer 与 DisplayOptions::setBackgroundColor 的语义存在重叠且交互未定义，是一个潜在陷阱：
按 tgfx 文档（tgfx/layers/DisplayList.h 中 backgroundColor() 注释）：DisplayList 的 background color 在 render() 路径下会用 SrcOver 绘制。当前 PAGScene::draw() 在背景层路径下仍调用 displayList->render(surface, false)，所以如果用户同时调用了 setBackgroundLayer(myLayer) 和 getDisplayOptions()->setBackgroundColor(opaqueColor)，渲染顺序会变成：clear → myLayer → opaque color SrcOver（盖掉 myLayer） → 内容。
PAGXView 自己绕开了这个问题（背景层路径下显式设 {0,0,0,0}），但作为对外 API 这是隐式约束。建议二选一：
- 在此处注释里明确说明"使用 backgroundLayer 时应将 DisplayOptions 的背景色设为透明"；
- 或在
PAGScene::draw()中当_backgroundLayer != nullptr时，把 DisplayList 背景色临时置为 Transparent 后再 render，避免覆盖。
| lastBackgroundDensity = density; | ||
| } | ||
| scene->setBackgroundLayer(backgroundLayer); | ||
| } |
There was a problem hiding this comment.
每帧重复设置 backgroundColor / backgroundLayer。
当前 draw() 每帧都会无条件调用 setBackgroundColor(...) 和 setBackgroundLayer(...)，即使状态没变化。这两个状态本质上只在 setBackgroundColor() / clearBackgroundColor() 时变更。
建议把状态注入操作收敛到那两个入口里执行，draw() 只负责绘制；或者加上"上一次状态"标记，只在变化时 set。属于代码整洁与微优化，非阻塞。
3 participants
在 PAGScene 中新增 setBackgroundLayer 接口，支持在 surface 坐标系下绘制自定义背景层（不受 zoomScale/contentOffset 影响）。pagx-viewer 利用此接口，在未设置自定义背景色时默认绘制棋盘格背景，与设置背景色互斥切换。