Skip to content

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

Open
luo2430 wants to merge 4 commits into
galacean:dev/2.0from
luo2430:dev/2.0

Conversation

@luo2430

@luo2430 luo2430 commented Jun 17, 2026

Copy link
Copy Markdown

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:

是否需要将判断提取为一个函数?

屏幕截图 2026-06-16 210312

vitest依旧是4个不通过,与 PR#3034 中提到的完全一致。

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 4 ⎯⎯⎯⎯⎯⎯⎯

 FAIL   @galacean/engine-tests (chromium)  src/core/input/InputManager.test.ts > InputManager > blur and focus
AssertionError: expected false to equal true

- Expected
+ Received

- true
+ false

 ❯ src/core/input/InputManager.test.ts:216:49
    214|     target.dispatchEvent(generateKeyboardEvent("keydown", "KeyA"));
    215|     engine.update();
    216|     expect(inputManager.isKeyDown(Keys.KeyA)).to.eq(true);
       |                                                 ^
    217|     target.dispatchEvent(new Event("blur"));
    218|     expect(inputManager.isKeyDown()).to.eq(false);

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/4]⎯

 FAIL   @galacean/engine-tests (chromium)  src/core/input/InputManager.test.ts > InputManager > change listener target
AssertionError: expected false to equal true

- Expected
+ Received

- true
+ false

 ❯ src/core/input/InputManager.test.ts:236:40
    234|     window.dispatchEvent(generateKeyboardEvent("keydown", "KeyA"));
    235|     engine.update();
    236|     expect(inputManager.isKeyDown()).to.eq(true);
       |                                        ^
    237| 
    238|     window.dispatchEvent(generateWheelEvent(1, 2, 3));

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/4]⎯

 FAIL   @galacean/engine-tests (chromium)  src/core/2d/text/TextUtils.test.ts > TextUtils > measureTextWithWrap
AssertionError: expected 26 to equal 27

- Expected
+ Received

- 27
+ 26

 ❯ src/core/2d/text/TextUtils.test.ts:170:36
    168|       "。"
    169|     ]);
    170|     expect(result.lineHeight).to.be.equal(27);
       |                                    ^
    171|     textRendererTruncate.text = text2;
    172|     result = TextUtils.measureTextWithWrap(

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[3/4]⎯

 FAIL   @galacean/engine-tests (chromium)  src/core/2d/text/TextUtils.test.ts > TextUtils > measureTextWithoutWrap
AssertionError: expected 26 to equal 27

- Expected
+ Received

- 27
+ 26

 ❯ src/core/2d/text/TextUtils.test.ts:412:36
    410|     expect(result.lines).to.be.deep.equal(["趚今天天气很好,阳光明媚。我 在公园里 漫步。"]);
    411|     expect(result.lineWidths).to.be.deep.equal([518]);
    412|     expect(result.lineHeight).to.be.equal(27);
       |                                    ^
    413|     textRendererTruncate.text = text2;
    414|     result = TextUtils.measureTextWithoutWrap(

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[4/4]⎯


 Test Files  2 failed | 109 passed (111)
      Tests  4 failed | 1444 passed (1448)
   Start at  13:40:16
   Duration  23.62s

Summary by CodeRabbit

Release Notes

  • New Features

    • Added automatic canvas resizing support with enableAutoResize() and disableAutoResize() methods for WebGL applications.
  • Tests

    • Added comprehensive test coverage for auto-resize functionality and canvas type detection.
    • Updated test configuration to disable screenshot capture on test failures.

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

WebCanvas gains an isOffscreenCanvas() method that centralizes the repeated inline OffscreenCanvas type checks; three existing guards in WebCanvas and two in WebGLGraphicDevice are updated to use it. WebGLEngine receives enableAutoResize(pixelRatio?) and disableAutoResize() methods backed by a ResizeObserver with EngineEventType.Shutdown cleanup. New tests cover both isOffscreenCanvas() and the full auto-resize lifecycle.

Changes

OffscreenCanvas detection and auto-resize

Layer / File(s) Summary
WebCanvas.isOffscreenCanvas() method and usage refactor
packages/rhi-webgl/src/WebCanvas.ts, tests/src/rhi-webgl/WebCanvas.test.ts
Adds isOffscreenCanvas(): boolean to WebCanvas and replaces three inline typeof OffscreenCanvas/instanceof guards (in scale getter, scale setter, and resizeByClientSize) with calls to the new method. A new test suite verifies the return value for HTMLCanvasElement, OffscreenCanvas, and post-construction field replacements.
WebGLGraphicDevice context fallback updated
packages/rhi-webgl/src/WebGLGraphicDevice.ts
Updates WebGLGraphicDevice.init to allow the WebGL2 gl variable to be undefined and replaces both experimental-webgl2 and experimental-webgl fallback gating with (canvas as WebCanvas).isOffscreenCanvas().
WebGLEngine enableAutoResize / disableAutoResize
packages/rhi-webgl/src/WebGLEngine.ts, tests/src/rhi-webgl/WebGLEngine.test.ts, tests/vitest.config.ts
Adds EngineEventType import, a private _resizeObserver field and static cleanup helper, and public enableAutoResize(pixelRatio?) / disableAutoResize() methods. enableAutoResize skips OffscreenCanvas, disconnects any prior observer, registers a new ResizeObserver calling canvas.resizeByClientSize, and registers a Shutdown listener. Tests verify listener counts, observer lifecycle and replacement, resizeByClientSize invocation after style mutation, and cleanup on engine.destroy(). screenshotFailures is set to false in the Vitest Playwright config.

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()
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Poem

🐰 A canvas that knows what it is — how grand!
No more typeof checks scattered by hand.
isOffscreenCanvas() speaks for the whole,
ResizeObserver watches with vigilant soul.
When shutdown arrives, it tidies the nest,
A well-behaved bunny — clean-up's the best! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is a multi-part compound title that covers three distinct changes (refactor, feat, chore) across different concerns. While each part is related to the actual changes, the combined title reads as a list rather than highlighting the main change, making it difficult to understand the primary purpose at a glance. Consider using a single primary title that emphasizes the main feature (auto-resize support) and brevity. For example: 'feat(rhi-webgl): add WebGLEngine auto-resize support via ResizeObserver' or split into multiple smaller PRs.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@luo2430 luo2430 marked this pull request as draft June 17, 2026 05:53

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5d74c1d and 887ac8c.

📒 Files selected for processing (3)
  • packages/core/src/input/InputManager.ts
  • packages/rhi-webgl/src/WebGLEngine.ts
  • tests/src/rhi-webgl/WebGLEngine.test.ts

Comment thread tests/src/rhi-webgl/WebGLEngine.test.ts Outdated
@luo2430 luo2430 marked this pull request as ready for review June 17, 2026 05:59
GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

@luo2430

luo2430 commented Jun 17, 2026

Copy link
Copy Markdown
Author

@GuoLei1990

this指针问题我考虑到了,但不知为何代码经过修改后我的本地测试可以跑通。

image

我会再去处理这个问题,原本以为已经修复了。

GuoLei1990

This comment was marked as outdated.

@cptbtptpbcptdtptp

Copy link
Copy Markdown
Collaborator

这里有一个时序问题需要提一下,根据 W3C 规范,浏览器每帧的渲染管线顺序是:

 Input Events (包括 window resize event) → rAF callbacks(引擎逻辑) → Style/Layout → ResizeObserver callbacks(本次 PR 添加) → Paint

在发生 resize 那帧,引擎绘制 rAF 时使用的还是旧的画布尺寸,随后 ResizeObserver 回调修改画布的尺寸会清空 canvas buffer ,表现就是这一帧会白,但 window 监听 resize 也不可以直接用,因为 window.resize 仅监听浏览器窗口大小变化,所以其他引擎要么完全手动触发,要么轮询检测,避免本帧 resize 与渲染不同步。

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.87500% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.17%. Comparing base (5d74c1d) to head (9fc8dce).
⚠️ Report is 1 commits behind head on dev/2.0.

Files with missing lines Patch % Lines
packages/rhi-webgl/src/WebCanvas.ts 81.81% 2 Missing ⚠️
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           
Flag Coverage Δ
unittests 79.17% <96.87%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@luo2430 luo2430 changed the title feat(rhi-webgl): add auto-resize support via ResizeObserver and refactor input canvas naming 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 Jun 18, 2026
@luo2430

luo2430 commented Jun 18, 2026

Copy link
Copy Markdown
Author

@GuoLei1990

代码处理完毕,vitest问题我会去开一个issue,有部分解决方案和提议,但需要验证。另外,isOffscreenCanvas中的typeof OffscreenCanvas !== "undefined"是不是可以直接提成一个全局常量或者静态常量,但项目中似乎没有这种先例,这符合项目规范吗?

GuoLei1990

This comment was marked as outdated.

@luo2430

luo2430 commented Jun 18, 2026

Copy link
Copy Markdown
Author

@GuoLei1990

注释已修改。
可以确定的是isOffscreenCanvas会被设定为一个public api,因为有可以跨包使用的地方。但为了这一处使用而添加一个新的工作区依赖我觉得不值得。

image

GuoLei1990

This comment was marked as outdated.

@luo2430

luo2430 commented Jun 18, 2026

Copy link
Copy Markdown
Author

@cptbtptpbcptdtptp

@GuoLei1990 GuoLei1990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

复审 @ 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] _cleanupResizeObserverthis 绑定丢失 / observer 泄漏 → 已修 @ b794d54。现为 private static _cleanupResizeObserver(engine: WebGLEngine),显式形参接收实例、方法体不读 this,从根上绕开 EventDispatcher.dispatchlistener.fn(data)this 指向包装对象的问题;once(Shutdown, WebGLEngine._cleanupResizeObserver) / off(...) 传同一静态引用,offfn === 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 属性式 APIpixelRatio 塞不进裸布尔 setter,理由成立,维持。关闭。
  • typeof OffscreenCanvas 提常量 / vitest 4 个失败 → 已答(前者无收益无先例、维持;后者是 InputManager/TextUtils 既存失败,与本 PR 无关)。关闭。

结论

approve:唯一历史阻塞项 P1 已从根上修复,两条 P2 + 全部 P3/提问均已闭环,CI 全绿,HEAD 未变、无新问题。提交 APPROVE 以解除我早前 request-changes 造成的 merge gate。

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