Adapt Stats to use engine RenderingStatistics and remove inaccurate hooks#345
Conversation
… and update engine dependency version
…d improve GPU memory tracking
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
WalkthroughThe stats package is refactored to use the Engine instance directly instead of WebGL context. Hook-based instrumentation (TextureHook, ShaderHook, RequestHook) is removed, replaced with metrics derived from engine.renderingStatistics. Peer dependency constraint bumped to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (2)
packages/stats/src/Core.ts (2)
62-66: Return type inconsistency: function returnsundefinedwithout explicit return.The
update()method has return typePerformanceDatabut returnsundefinedimplicitly on lines 65-67 and 70-73. Consider usingPerformanceData | undefinedas the return type for clarity.Proposed fix
- public update(): PerformanceData { + public update(): PerformanceData | undefined {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stats/src/Core.ts` around lines 62 - 66, The update() method in Core.ts has a declared return type of PerformanceData but can return undefined early; change the signature of update() from returning PerformanceData to PerformanceData | undefined (or alternatively ensure it always returns a PerformanceData), update any callers accordingly, and keep the existing early-return branches that currently return nothing; this affects the update() method and related uses of its return value (e.g., places that read its result), so update those call sites to handle the undefined case.
28-34: Accessing internal API via@ts-ignoreis fragile.
engine._hardwareRenderer.glis an internal implementation detail that may change without notice. Consider requesting an official public API from the engine team to access the WebGL context, or document this as a known limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stats/src/Core.ts` around lines 28 - 34, The constructor currently reaches into a private field (engine._hardwareRenderer.gl) using `@ts-ignore` which is fragile; instead add a safe public-facing approach: change the Core constructor signature to accept an optional WebGLRenderingContext/GLContext parameter (e.g., constructor(engine: Engine, gl?: WebGLRenderingContext | WebGL2RenderingContext)), prefer using a hypothetical public accessor like engine.getGL() if it exists, and only fall back to engine._hardwareRenderer.gl when no public API or passed-in gl is available while logging/documenting this fallback; update references in the constructor (the Engine param, the gl assignment, and the call to this.hook(gl)) and remove the `@ts-ignore` so consumers can inject a stable GL or the engine team can add a public getter later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stats/src/Core.ts`:
- Line 77: The compiler error comes from accessing
this.engine.renderingStatistics and its fields (textureMemory, bufferMemory,
totalMemory) while the installed `@galacean/engine` types don't declare
renderingStatistics; fix by either updating the dependency to a release that
includes the renderingStatistics type (verify and bump `@galacean/engine` to the
correct version) or make the access type-safe: in Core.ts check/guard that
this.engine.renderingStatistics exists (use optional chaining and fallback
defaults) or add a local type augmentation for the Engine instance (declare the
renderingStatistics interface with textureMemory/bufferMemory/totalMemory) and
cast this.engine appropriately before reading those properties so TypeScript no
longer complains.
---
Nitpick comments:
In `@packages/stats/src/Core.ts`:
- Around line 62-66: The update() method in Core.ts has a declared return type
of PerformanceData but can return undefined early; change the signature of
update() from returning PerformanceData to PerformanceData | undefined (or
alternatively ensure it always returns a PerformanceData), update any callers
accordingly, and keep the existing early-return branches that currently return
nothing; this affects the update() method and related uses of its return value
(e.g., places that read its result), so update those call sites to handle the
undefined case.
- Around line 28-34: The constructor currently reaches into a private field
(engine._hardwareRenderer.gl) using `@ts-ignore` which is fragile; instead add a
safe public-facing approach: change the Core constructor signature to accept an
optional WebGLRenderingContext/GLContext parameter (e.g., constructor(engine:
Engine, gl?: WebGLRenderingContext | WebGL2RenderingContext)), prefer using a
hypothetical public accessor like engine.getGL() if it exists, and only fall
back to engine._hardwareRenderer.gl when no public API or passed-in gl is
available while logging/documenting this fallback; update references in the
constructor (the Engine param, the gl assignment, and the call to this.hook(gl))
and remove the `@ts-ignore` so consumers can inject a stable GL or the engine team
can add a public getter later.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/stats/README.mdpackages/stats/package.jsonpackages/stats/src/Core.tspackages/stats/src/Monitor.tspackages/stats/src/Stats.ts
….0 or higher across multiple packages
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/draco/package.json`:
- Around line 22-24: The package.json currently lists "@galacean/engine" under
"dependencies"; move it into "peerDependencies" instead to match the monorepo
toolkit convention and avoid bundling a duplicate engine. Remove the
"@galacean/engine" entry from "dependencies" and add the same version constraint
(">=2.0.0-0") under "peerDependencies"; if the package needs the engine at
build/test time, also add a matching entry under "devDependencies" to satisfy
local tooling.
In `@packages/stats/src/Monitor.ts`:
- Around line 68-69: The array initialization for Monitor.items is duplicated
and formatted in a way that triggers prettier; remove the redundant this.items =
[] assignment and replace the single-line array with a properly formatted array
literal assigned to this.items (keep the existing values "fps", "memory",
"totalGraphicsMemory", "textureMemory", "bufferMemory", "drawCall", "triangles",
"webglContext") inside the Monitor constructor or initializer so the assignment
is singular and conforms to prettier formatting for arrays.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 61fd1f50-b43e-4bc6-8334-2cd0240a0b4b
📒 Files selected for processing (26)
package.jsonpackages/auxiliary-lines/package.jsonpackages/controls/package.jsonpackages/custom-gltf-parser/package.jsonpackages/custom-material/package.jsonpackages/draco/package.jsonpackages/dynamic-bone/package.jsonpackages/framebuffer-picker/package.jsonpackages/geometry-sketch/package.jsonpackages/gizmo/package.jsonpackages/input-logger/package.jsonpackages/lines/package.jsonpackages/navigation-gizmo/package.jsonpackages/outline/package.jsonpackages/skeleton-viewer/package.jsonpackages/stats/README.mdpackages/stats/src/Core.tspackages/stats/src/Monitor.tspackages/stats/src/Stats.tspackages/stats/src/hooks/RequestHook.tspackages/stats/src/hooks/ShaderHook.tspackages/stats/src/hooks/TextureHook.tspackages/tween/package.jsonpackages/way-point/package.jsonpackages/xr/package.jsontests/package.json
💤 Files with no reviewable changes (3)
- packages/stats/src/hooks/RequestHook.ts
- packages/stats/src/hooks/TextureHook.ts
- packages/stats/src/hooks/ShaderHook.ts
✅ Files skipped from review due to trivial changes (1)
- packages/auxiliary-lines/package.json
| "dependencies": { | ||
| "@galacean/engine": ">=1.6.0-0" | ||
| "@galacean/engine": ">=2.0.0-0" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check `@galacean/engine` declarations in all package.json files
echo "=== Checking `@galacean/engine` declarations across all packages ==="
echo ""
fd -e json -g 'package.json' packages | sort | while read pkgfile; do
# Extract package name and dependencies info
pkgname=$(cat "$pkgfile" | jq -r '.name // "unknown"')
# Check for `@galacean/engine` in dependencies
dep_version=$(cat "$pkgfile" | jq -r '.dependencies["@galacean/engine"] // "NOT DECLARED"')
# Check for `@galacean/engine` in peerDependencies
peer_version=$(cat "$pkgfile" | jq -r '.peerDependencies["@galacean/engine"] // "NOT DECLARED"')
echo "Package: $pkgname (file: $pkgfile)"
echo " dependencies: $dep_version"
echo " peerDependencies: $peer_version"
echo ""
doneRepository: galacean/engine-toolkit
Length of output: 3079
Move @galacean/engine from dependencies to peerDependencies.
This package declares @galacean/engine as a regular dependency, but all other toolkit packages in the monorepo use peerDependencies. For toolkit packages that integrate with a host application's engine instance, peerDependencies is the standard to avoid duplicate engine instances and version conflicts at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/draco/package.json` around lines 22 - 24, The package.json currently
lists "@galacean/engine" under "dependencies"; move it into "peerDependencies"
instead to match the monorepo toolkit convention and avoid bundling a duplicate
engine. Remove the "@galacean/engine" entry from "dependencies" and add the same
version constraint (">=2.0.0-0") under "peerDependencies"; if the package needs
the engine at build/test time, also add a matching entry under "devDependencies"
to satisfy local tooling.
| this.items = []; | ||
| this.items = ["fps", "memory", "drawCall", "triangles", "textures", "shaders", "size", "webglContext"]; | ||
| this.items = ["fps", "memory", "totalGraphicsMemory", "textureMemory", "bufferMemory", "drawCall", "triangles", "webglContext"]; |
There was a problem hiding this comment.
Fix prettier failure in items initialization.
Line 69 is flagged by prettier/prettier. Reformatting this array (and removing the redundant initial assignment) should clear the lint error.
💡 Proposed fix
- this.items = [];
- this.items = ["fps", "memory", "totalGraphicsMemory", "textureMemory", "bufferMemory", "drawCall", "triangles", "webglContext"];
+ this.items = [
+ "fps",
+ "memory",
+ "totalGraphicsMemory",
+ "textureMemory",
+ "bufferMemory",
+ "drawCall",
+ "triangles",
+ "webglContext"
+ ];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.items = []; | |
| this.items = ["fps", "memory", "drawCall", "triangles", "textures", "shaders", "size", "webglContext"]; | |
| this.items = ["fps", "memory", "totalGraphicsMemory", "textureMemory", "bufferMemory", "drawCall", "triangles", "webglContext"]; | |
| this.items = [ | |
| "fps", | |
| "memory", | |
| "totalGraphicsMemory", | |
| "textureMemory", | |
| "bufferMemory", | |
| "drawCall", | |
| "triangles", | |
| "webglContext" | |
| ]; |
🧰 Tools
🪛 ESLint
[error] 69-69: Replace "fps",·"memory",·"totalGraphicsMemory",·"textureMemory",·"bufferMemory",·"drawCall",·"triangles",·"webglContext" with ⏎······"fps",⏎······"memory",⏎······"totalGraphicsMemory",⏎······"textureMemory",⏎······"bufferMemory",⏎······"drawCall",⏎······"triangles",⏎······"webglContext"⏎····
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stats/src/Monitor.ts` around lines 68 - 69, The array initialization
for Monitor.items is duplicated and formatted in a way that triggers prettier;
remove the redundant this.items = [] assignment and replace the single-line
array with a properly formatted array literal assigned to this.items (keep the
existing values "fps", "memory", "totalGraphicsMemory", "textureMemory",
"bufferMemory", "drawCall", "triangles", "webglContext") inside the Monitor
constructor or initializer so the assignment is singular and conforms to
prettier formatting for arrays.
…0.0 or higher across multiple packages
2 participants
Summary
engine.renderingStatisticsengineinstead of rawglcontextTextureHook,ShaderHook,RequestHook@galacean/engine >= 2.0.0-0Changes
Engineinstead ofgl, readrenderingStatisticsfor memory stats, remove texture/shader/request hook usage_setupMonitor()to passthis.enginedirectly; removehookRequeststatic method andcamerafieldTextureHook.ts,ShaderHook.ts,RequestHook.ts>=1.6.0-0to>=2.0.0-0Test plan
engine.renderingStatisticsSummary by CodeRabbit
Breaking Changes
New Features
Removed Features