refactor(rhi-webgl): extract isOffscreenCanvas helper | feat(rhi-webgl): add WebGLEngine auto-resize support via ResizeObserver | chore(vitest): disable screenshotFailures to avoid taking blank screenshots#3037
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesOffscreenCanvas detection and auto-resize
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WebGLEngine
participant ResizeObserver
participant WebCanvas
Caller->>WebGLEngine: enableAutoResize(pixelRatio?)
WebGLEngine->>ResizeObserver: disconnect() (prior observer, if any)
WebGLEngine->>ResizeObserver: new ResizeObserver(callback)
WebGLEngine->>ResizeObserver: observe(canvas element)
WebGLEngine->>WebGLEngine: on(Shutdown, _cleanupResizeObserver)
Note over ResizeObserver,WebCanvas: canvas client dimensions change
ResizeObserver->>WebCanvas: resizeByClientSize(pixelRatio)
Caller->>WebGLEngine: disableAutoResize()
WebGLEngine->>WebGLEngine: off(Shutdown, _cleanupResizeObserver)
WebGLEngine->>ResizeObserver: disconnect()
Note over WebGLEngine: engine.destroy() triggers Shutdown
WebGLEngine->>WebGLEngine: _cleanupResizeObserver()
WebGLEngine->>ResizeObserver: disconnect()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/src/rhi-webgl/WebGLEngine.test.ts`:
- Around line 140-145: The spy on the _cleanupResizeObserver method is created
after enableAutoResize() has already registered the Shutdown listener, meaning
the listener holds a reference to the original function rather than the spied
version. Move the vi.spyOn call for cleanupResizeObserverSpy to before the
enableAutoResize() call to ensure the registered Shutdown listener captures the
spied function reference, allowing the assertion on
cleanupResizeObserverSpy.toHaveBeenCalledTimes(1) to work correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca7819a5-d724-46f3-ad7d-879ae0800382
📒 Files selected for processing (3)
packages/core/src/input/InputManager.tspackages/rhi-webgl/src/WebGLEngine.tstests/src/rhi-webgl/WebGLEngine.test.ts
|
这里有一个时序问题需要提一下,根据 W3C 规范,浏览器每帧的渲染管线顺序是: 在发生 resize 那帧,引擎绘制 rAF 时使用的还是旧的画布尺寸,随后 ResizeObserver 回调修改画布的尺寸会清空 canvas buffer ,表现就是这一帧会白,但 window 监听 resize 也不可以直接用,因为 window.resize 仅监听浏览器窗口大小变化,所以其他引擎要么完全手动触发,要么轮询检测,避免本帧 resize 与渲染不同步。 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3037 +/- ##
========================================
Coverage 79.17% 79.17%
========================================
Files 914 914
Lines 101666 101723 +57
Branches 11209 11212 +3
========================================
+ Hits 80489 80537 +48
- Misses 20990 20999 +9
Partials 187 187
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
代码处理完毕,vitest问题我会去开一个issue,有部分解决方案和提议,但需要验证。另外,isOffscreenCanvas中的 |
GuoLei1990
left a comment
There was a problem hiding this comment.
复审 @ 9fc8dce847(HEAD 自上轮未变,全部问题已闭环,APPROVE 解锁合并)
自上轮(9fc8dce84)以来 HEAD 一字未变(仍是 9fc8dce847,已 git fetch refs/pull/3037/head 核对),无新 commit。期间唯一动态是 @luo2430 的两条评论:04:23 那条(注释已改 + isOffscreenCanvas 作为跨包 public API 的理由)我上轮 04:58 已答并关闭;05:48 仅 @cptbtptpbcptdtptp 一个 ping,无新内容。无新代码可增量审。
本轮提交 APPROVE 的原因(不是重新审,是清残留 block)
PR 至今仍 reviewDecision: CHANGES_REQUESTED,但这并非还有未修问题——它是我早前三轮 --request-changes(都在 e46c08f28)的残留状态。GitHub 不会因为我后来改提 --comment 就自动撤销前面的 request-changes(COMMENTED 不 supersede CHANGES_REQUESTED),minimize 也只折叠 UI、不解除 merge gate。所以唯一能由我这边干净解锁的方式就是显式 APPROVE。
已逐条闭环(@ 9fc8dce847 代码核对,非信 commit message)
- [P1]
_cleanupResizeObserver的this绑定丢失 / observer 泄漏 → 已修 @b794d54。现为private static _cleanupResizeObserver(engine: WebGLEngine),显式形参接收实例、方法体不读this,从根上绕开EventDispatcher.dispatch的listener.fn(data)把this指向包装对象的问题;once(Shutdown, WebGLEngine._cleanupResizeObserver)/off(...)传同一静态引用,off按fn === fn能正确摘除。CI 全绿即铁证。关闭。 - [P2] cleanup spy 注册太晚 → 已修 @
b794d54(测试整体重写为listenerCount+ observer 实例对比)。关闭。 - [P2]
observe()上方注释撒谎 → 已修 @9fc8dce84。现注释如实写:"ResizeObserver callbacks fire after the current frame's rAF … The current frame may render with stale dimensions (e.g. a flash of incorrect sizing/blank area), and will recover on the next frame."——与 @cptbtptpbcptdtptp 给的Input → rAF → Style/Layout → ResizeObserver → Paint时序一致。关闭。 - [P2]
WebCanvas.isOffscreenCanvas()缺 JSDoc → 已修 @9fc8dce84。补上多行 JSDoc 块 +@returns,与scale/resizeByClientSize一致;作者已明确它有意作为跨包 public API(不愿为单处使用引入新 workspace 依赖),保持裸 public + JSDoc 合理。关闭。 - [P3] enable/disable 方法对 vs 属性式 API →
pixelRatio塞不进裸布尔 setter,理由成立,维持。关闭。 typeof OffscreenCanvas提常量 / vitest 4 个失败 → 已答(前者无收益无先例、维持;后者是InputManager/TextUtils既存失败,与本 PR 无关)。关闭。
结论
approve:唯一历史阻塞项 P1 已从根上修复,两条 P2 + 全部 P3/提问均已闭环,CI 全绿,HEAD 未变、无新问题。提交 APPROVE 以解除我早前 request-changes 造成的 merge gate。


Please check if the PR fulfills these requirements
Other information:
Summary by CodeRabbit
Release Notes
New Features
enableAutoResize()anddisableAutoResize()methods for WebGL applications.Tests