refactor(enum): introduce EngineEventType enum. chore: clean compiledShaders. build: chmod compat for windows.#3034
Merged
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughIntroduces a typed ChangesEngine Event Typing
Vitest Upgrade and Test Infrastructure Migration
Shader Artifact Removal and Build Script Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/rhi-webgl/src/WebGLEngine.ts`:
- Around line 35-40: The JSDoc comment for the method (starting at line 35)
incorrectly states that the method returns a cleanup function, but the actual
method returns void. Update the `@returns` documentation line in the JSDoc comment
to accurately reflect the void return type, removing the reference to a cleanup
function and instead describing that the method sets up automatic canvas
resizing with window resize event listeners.
- Around line 41-49: The autoResizeCanvas method creates a new ResizeObserver
each time it is called without checking if one already exists, causing multiple
observers to stack and remain active until shutdown. Additionally, the canvas
only updates its size after the first observer callback fires, leaving it with
stale dimensions initially. To fix this, store the observer as an instance
variable and disconnect any existing observer before creating a new one to
prevent stacking. Then call this.canvas.resizeByClientSize(pixelRatio)
immediately at the start of the method before setting up the observer to apply
an initial resize synchronously. This makes autoResizeCanvas idempotent and
ensures the canvas is properly sized from the moment the method is called.
In `@packages/shader/.gitignore`:
- Line 1: The .gitignore entry uses an absolute path
packages/shader/compiledShaders/ when it should use a relative path. Since the
.gitignore file is located at packages/shader/.gitignore, paths should be
relative to that location. Change the entry from
packages/shader/compiledShaders/ to simply compiledShaders/ so that git
correctly ignores the compiled shaders directory.
🪄 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: 9cb4aca4-3359-42ad-80fe-c2d78c500a64
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.github/HOW_TO_CONTRIBUTE.mdpackages/core/src/Engine.tspackages/core/src/EngineEventType.tspackages/core/src/index.tspackages/rhi-webgl/src/WebGLEngine.tspackages/shader-compiler/package.jsonpackages/shader/.gitignorepackages/shader/compiledShaders/2D/Sprite.shadercpackages/shader/compiledShaders/2D/SpriteMask.shadercpackages/shader/compiledShaders/2D/Text.shadercpackages/shader/compiledShaders/2D/UIDefault.shadercpackages/shader/compiledShaders/BlinnPhong.shadercpackages/shader/compiledShaders/Blit/Blit.shadercpackages/shader/compiledShaders/Blit/BlitScreen.shadercpackages/shader/compiledShaders/Effect/Particle.shadercpackages/shader/compiledShaders/Effect/ParticleFeedback.shadercpackages/shader/compiledShaders/Effect/Trail.shadercpackages/shader/compiledShaders/Lighting/ScalableAmbientOcclusion.shadercpackages/shader/compiledShaders/PBR.shadercpackages/shader/compiledShaders/Pipeline/DepthOnly.shadercpackages/shader/compiledShaders/Pipeline/ShadowCaster.shadercpackages/shader/compiledShaders/PostProcess/Bloom.shadercpackages/shader/compiledShaders/PostProcess/FinalAntiAliasing.shadercpackages/shader/compiledShaders/PostProcess/FinalSRGB.shadercpackages/shader/compiledShaders/PostProcess/Uber.shadercpackages/shader/compiledShaders/Sky/BackgroundTexture.shadercpackages/shader/compiledShaders/Sky/SkyProcedural.shadercpackages/shader/compiledShaders/Sky/Skybox.shadercpackages/shader/compiledShaders/Unlit.shadercpackages/shader/compiledShaders/index.tspackages/shader/tsconfig.json
💤 Files with no reviewable changes (23)
- packages/shader/compiledShaders/BlinnPhong.shaderc
- packages/shader/compiledShaders/Effect/Particle.shaderc
- packages/shader/compiledShaders/Effect/ParticleFeedback.shaderc
- packages/shader/compiledShaders/2D/Sprite.shaderc
- packages/shader/compiledShaders/Pipeline/DepthOnly.shaderc
- packages/shader/compiledShaders/Sky/Skybox.shaderc
- packages/shader/compiledShaders/PostProcess/FinalSRGB.shaderc
- packages/shader/compiledShaders/Effect/Trail.shaderc
- packages/shader/compiledShaders/Pipeline/ShadowCaster.shaderc
- packages/shader/compiledShaders/PostProcess/Bloom.shaderc
- packages/shader/compiledShaders/Unlit.shaderc
- packages/shader/compiledShaders/Blit/Blit.shaderc
- packages/shader/compiledShaders/Sky/BackgroundTexture.shaderc
- packages/shader/compiledShaders/PBR.shaderc
- packages/shader/compiledShaders/2D/SpriteMask.shaderc
- packages/shader/compiledShaders/PostProcess/Uber.shaderc
- packages/shader/compiledShaders/2D/Text.shaderc
- packages/shader/compiledShaders/Lighting/ScalableAmbientOcclusion.shaderc
- packages/shader/compiledShaders/Blit/BlitScreen.shaderc
- packages/shader/compiledShaders/index.ts
- packages/shader/compiledShaders/Sky/SkyProcedural.shaderc
- packages/shader/compiledShaders/PostProcess/FinalAntiAliasing.shaderc
- packages/shader/compiledShaders/2D/UIDefault.shaderc
Comment on lines
+41
to
+49
| autoResizeCanvas(pixelRatio: number = window.devicePixelRatio): void { | ||
| let observer: ResizeObserver | undefined = new ResizeObserver(() => { | ||
| this.canvas.resizeByClientSize(pixelRatio); | ||
| }); | ||
| observer.observe(this.canvas._webCanvas); | ||
| this.on(EngineEventType.shutdown, () => { | ||
| observer?.disconnect(); | ||
| observer = undefined; | ||
| }); |
There was a problem hiding this comment.
Make autoResizeCanvas idempotent and apply an initial resize immediately.
Repeated calls currently stack observers until shutdown, and size is only updated after the first observer callback. That can cause duplicate resize work and stale canvas sizing.
💡 Proposed fix
export class WebGLEngine extends Engine {
+ private _resizeObserver?: ResizeObserver;
+
/**
* Automatically resize the canvas to fit the client size,
* and keep listening to window resize events.
* `@param` pixelRatio - The pixel ratio used for resizing, defaults to `window.devicePixelRatio`
* `@returns` A cleanup function to remove the resize listener
*/
autoResizeCanvas(pixelRatio: number = window.devicePixelRatio): void {
- let observer: ResizeObserver | undefined = new ResizeObserver(() => {
+ this._resizeObserver?.disconnect();
+ this.canvas.resizeByClientSize(pixelRatio);
+
+ const observer = new ResizeObserver(() => {
this.canvas.resizeByClientSize(pixelRatio);
});
+ this._resizeObserver = observer;
observer.observe(this.canvas._webCanvas);
- this.on(EngineEventType.shutdown, () => {
- observer?.disconnect();
- observer = undefined;
+
+ this.once(EngineEventType.shutdown, () => {
+ if (this._resizeObserver === observer) {
+ this._resizeObserver.disconnect();
+ this._resizeObserver = undefined;
+ } else {
+ observer.disconnect();
+ }
});
}
}🤖 Prompt for 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.
In `@packages/rhi-webgl/src/WebGLEngine.ts` around lines 41 - 49, The
autoResizeCanvas method creates a new ResizeObserver each time it is called
without checking if one already exists, causing multiple observers to stack and
remain active until shutdown. Additionally, the canvas only updates its size
after the first observer callback fires, leaving it with stale dimensions
initially. To fix this, store the observer as an instance variable and
disconnect any existing observer before creating a new one to prevent stacking.
Then call this.canvas.resizeByClientSize(pixelRatio) immediately at the start of
the method before setting up the observer to apply an initial resize
synchronously. This makes autoResizeCanvas idempotent and ensures the canvas is
properly sized from the moment the method is called.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Author
|
@GuoLei1990 好的，autoResizeCanvas和EngineEventType已经重写完了，等会会重新整理git提交。目前我Windows端vitest插件对vitest@2.1.3版本会报错，可能要升级vitest及其套件的版本，4的破坏性变更太多，在考虑3的可能性。另外现在怎么加入galacean的交流群呢？主页上的三个二维码的群都进不去。 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Author
重新提交了几个commit，将不同操作分开了。目前
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
luo2430
changed the title
refactor(enum): introduce EngineEventType enum. chore: clean compiledShaders. build: chmod compat for windows.
Author
|
@GuoLei1990 标题已改，我工作太零散了，老是忘😂 |
GuoLei1990
reviewed
Jun 15, 2026
GuoLei1990
left a comment
GuoLei1990
left a comment
Member
There was a problem hiding this comment.
总结
HEAD 推进到 7fd5df1（merge dup dev/2.0，GLTF 那两文件是上游 #3027 带进来的、不在本 PR 3-dot 净 diff 内，已核对忽略）。本轮唯一新增的 PR 自有 commit 是 8af7c50c test: upgrade vitest to 3.2.6——这是作者在评论里提的 Windows vitest 插件不兼容 2.1.3 的后续处理，把整套 vitest 从 2.1.3 升到 3.2.6。
逐条核对上轮问题，全部仍关闭（autoResizeCanvas 仍移出、EngineEventType 已 PascalCase、shader 产物清理 + node -e chmod 已 LGTM），标题已按建议改为 refactor(enum) + chore + build，"and more" 已去掉。vitest 升级本身迁移正确、内部一致，无 P0/P1。
vitest 3.2.6 升级核对（本轮新增改动）— 均验证 OK
- 配置迁移是官方 vitest 3 路径，1:1 等价：
vitest.workspace.ts（defineWorkspace([...])）→vitest.config.ts（defineConfig({ test: { projects: [...] }})），glob["packages/*", "tests"]原样保留；browser configname: "chromium"→instances: [{ browser: "chromium", launch: {...} }]（vitest 3 browser API 变更）。launch args 完全照搬，无行为变化。 - 版本对齐一致：lockfile 内
vitest/@vitest/{browser,coverage-v8,expect,mocker,runner,snapshot,spy,utils}全部锁到3.2.6，无 major skew——vitest 对@vitest/*同版本要求很硬，混版会运行时崩，这点过了。@vitest/coverage-v8与vitest同步升（3.2.6），--coverage走 v8 默认无显式 config block，与升级前一致。 readShaderTestFile.ts路径包装器正确且完整。vitest 3 的server.commands.readFile解析基准从「测试文件相对」变成「test 项目根（tests/）相对」，故需重写：./shaders/x→src/shader-compiler/shaders/x（相对tests/✓）、../../../packages/...→../packages/...（相对tests/→ 仓库根packages/✓）。已 grep 全部 ~80 处readFile(...)调用点，路径只有这两种前缀（含./expected/...，走./分支，目录存在），全覆盖；fallbackreturn readBrowserFile(path)当前无调用但作安全网无害。四个测试文件都换了新 wrapper、无残留死 import。
备忘（提一次，不阻塞）
- scope 又扩了一项：上轮 PR 已收窄成
refactor + chore + build三件事，本轮 vitest 升级是第四个正交改动，而且是体量最大的一个（661 行 lockfile + browser/workspace config 迁移 + 测试 path 适配）。它跟枚举/shader 清理没有依赖关系，本质是作者本地 Windows dev 环境（VSCode vitest 插件不兼容 2.1.3）的工程化诉求。vitest major 升级影响全仓所有测试运行方式，回滚粒度、CI 验证面都和本 PR 主体不同——更适合单独成一个build(deps): upgrade vitest to 3PR，便于独立过 CI、独立回滚。当前 PR 名为refactor(enum)...，vitest 升级既不在标题也不在三个前缀覆盖范围内。提一次即可，是否拆由你定。 - CI 未实际跑：本 PR 的 CI 处于
action_required（外部贡献者待批准，0s未执行），vitest 3 迁移的「跑得通」无法靠 CI 自证。作者评论里提到「升 3 后有 4 个测试不通过，需与你们结果核实」——这 4 个 fail 是否已随本轮 commit 解决、还是仍存在，需要 maintainer 批准 CI 跑一遍确认。迁移代码本身是对的，但「实际全绿」这一步只能等 CI 放行。
回答作者提问
- OffscreenCanvas 判断抽不抽函数：那段在
autoResizeCanvas里，不在本 PR diff 内（WebGLEngine.ts已随 autoResizeCanvas 一并移出），等它单独成 PR 时再讨论。简短答：typeof OffscreenCanvas !== "undefined" && x instanceof OffscreenCanvas这个 guard 如果只在autoResizeCanvas一处用，没必要抽；若多处复用再抽成_isOffscreenCanvas(canvas)私有 helper 即可。 - 入群问题：不在代码审查范围，建议 PR 里直接 @ 维护者或走官网/Discord。
代码侧无问题，LGTM。vitest 升级若不拆出，合并前请确保 CI 跑通且那 4 个 fail 已澄清。
GuoLei1990
reviewed
Jun 15, 2026
GuoLei1990
left a comment
GuoLei1990
left a comment
Member
There was a problem hiding this comment.
总结
HEAD 推进到 e712794，自上轮 review（7fd5df1）后唯一新增的 PR 自有 commit 是 e71279475 test: use explicit shader fixture paths。这一 commit 删除了上轮我验证并称赞过的 readShaderTestFile.ts 路径包装器（-17 行 + 全部 import），改为在每个 test 文件顶部 const { readFile } = server.commands 直接解构，并把所有调用点的路径换成 vitest 3 项目根（tests/）下的显式真实路径。
逐条核对上轮已关闭问题，全部仍关闭（枚举已 PascalCase、shader 产物清理 + node -e chmod、vitest 3.2.6 迁移、标题已收敛为 refactor(enum) + chore + build），本轮新改动验证正确、机械等价、无新问题，无 P0/P1。
本轮新增改动核对（e71279475）— 均验证 OK
- 路径转换与被删包装器一一等价。被删的 wrapper 做两件映射：
./<x>→src/shader-compiler/<x>、../../../packages/→../packages/。新 commit 把这两套映射的结果直接写到调用点。grep 全部 shader-compiler 测试，现存readFile路径前缀只有三种，全部与 wrapper 输出对齐：src/shader-compiler/shaders/...（原./shaders/...）✓src/shader-compiler/expected/...（原./expected/...）✓../packages/shader/src/Shaders/...（原../../../packages/shader/src/Shaders/...）✓
- 无残留与无遗漏：repo 内已无任何
readShaderTestFile引用（4 个测试文件的 import 全删、文件本身删除）；已无任何未转换的裸.//../../../readFile调用（去掉 wrapper 后这类路径会解析错，已确认零残留）。ShaderValidate.ts不经readFile，不受影响。 - 改动面纯粹：本 commit 只动那 5 个文件，全是路径字符串替换，无新增逻辑、无新增注释，无行为变化。
- 顺带确认：
tests/vitest.config.ts（name: "chromium"→instances:[...]）、tests/package.json（@vitest/browser2.1.3→3.2.6）等仍是上轮8af7c50cvitest 升级的一部分，自上轮未变，无需重审。
方向评判：删 wrapper 是净简化，方向正确
上轮我把这个 wrapper 评为「正确且完整」，但退一步从第一性原理看：这个包装器存在的唯一理由是「让调用方写一个与真实解析路径不同的路径」（caller 写 ./shaders/x，wrapper 偷偷改写成 src/shader-compiler/shaders/x），本质是一处小的路径语义双源——读代码要打开 wrapper 才知道 ./shaders/x 究竟读的是哪个文件。删掉它、让调用点写 server.commands.readFile 实际会用的路径，所见即所读，无指间接、删 17 行 shim，更贴 vitest 3 server.commands.readFile（按 test 项目根解析）的官方语义。
代价仅是路径略冗长（src/shader-compiler/shaders/x vs ./shaders/x）且重复在 ~80 处——但 fixtures 目录不会移动，真要移动一次 find-replace 即可。这是教科书式的 DRY-vs-显式取舍，对测试套件而言显式更优。所以上轮的「wrapper 很好」我收回半步：删它不是回退，是把我自己当时漏判的双源消掉了，方向对。
备忘（沿用上轮，提一次不重复）
- vitest 3 升级仍是本 PR 第四个正交改动，scope 偏宽，更适合独立成
build(deps)PR；本轮的测试 path 适配是其直接附属（vitest 3 改了readFile解析基准才需要），归在一起合理。是否拆由 maintainer 定。 - CI 仍需 maintainer 放行实跑：vitest 3 迁移 + path 适配的「实际全绿」只能靠 CI 自证；作者此前提到升 3 后有 4 个 fail 待核实，本轮 path 适配是否已让其归零，需 CI 跑一遍确认。迁移与本轮 path 改动代码本身是对的。
代码侧无问题，LGTM。
GuoLei1990
approved these changes
Jun 15, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3034 +/- ##
===========================================
+ Coverage 77.49% 78.99% +1.49%
===========================================
Files 914 914
Lines 101746 101631 -115
Branches 10431 11139 +708
===========================================
+ Hits 78853 80279 +1426
+ Misses 22710 21165 -1545
- Partials 183 187 +4
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
EngineEventTypeenum for better type safetyautoResizeCanvasmethod toWebGLEnginewithResizeObserversupportHOW_TO_CONTRIBUTE.mdto usepnpmcommands and Node.js 15+shxdev dependency for cross-platformchmodsupport（否则在windows端build时会因ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL @galacean/engine-shader-compiler@2.0.0-alpha.34 build: rollup -c rollup.config.js && chmod +x bundler/cli.js而失败。shx chmod在windows端不会有任何操作，仅用作兼容）What is the current behavior? (You can also link to an open issue here)
赞！
What is the new behavior (if this is a feature change)?
很赞！！
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
没有
Other information:
感谢开源，愿尽微薄之力。
Summary by CodeRabbit
Refactor
Chores