-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: 重构 Stats 插件初始化入参,支持自定义容器和是否可见 #798
base: feat/2.2
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request introduces modifications across multiple files in the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
@@ -109,7 +116,7 @@ export class Monitor implements Disposable { | |||
update (dt: number): void { | |||
const data = this.core.update(dt); | |||
|
|||
if (!data || data.drawCall === 0 || data.triangles === 0) { | |||
if (!data || data.triangles === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://mdn.alipayobjects.com/mars/afts/file/A*P4lHTKYL_DgAAAAAAAAAAAAADlB4AQ
这种简单的合成 drawCall 一直是 0,这里的判断会导致后面的逻辑不会继续计算
@Sruimeng 看看去掉是否合理
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (16)
plugin-packages/stats/src/hooks/program-hook.ts (3)
13-13
: Add a default value for debug parameter
Currently, the constructor mandates a boolean for debug. Consider giving it a default value to simplify usage for consumers who might not require debug logs.constructor ( private readonly gl: WebGLRenderingContext | WebGL2RenderingContext, - private readonly debug: boolean, + private readonly debug: boolean = false, ) {
19-19
: Condense the debug logic
To reduce code duplication, consider refactoring the conditional debug statements into a helper function or a single location, especially when the same conditional structure appears multiple times in distinct hooks.
28-28
: Ensure consistent debug message structure
The debug message logs "UseProgram: …, program: …". If you aim to standardize debug logs across hooks, confirm that each message follows a similar format (e.g., prefix or suffix consistent across hooks).plugin-packages/stats/src/hooks/texture-hook.ts (4)
13-13
: Add a default value for debug
As done in other hooks, consider providing a default value to unify usage and reduce the need for explicit boolean input in all contexts.constructor ( private readonly gl: WebGLRenderingContext | WebGL2RenderingContext, - private readonly debug: boolean, + private readonly debug: boolean = false, ) {
21-21
: Refactor repeated debug checks
The pattern “if (debug) { console.debug(...) }” repeats across multiple hooks. You may refactor by introducing a small helper method or a shared logger to handle all debug outputs consistently.
31-31
: Enhance message clarity
Current debug message logs “CreateTexture: , textures: <this.textures>.”. If multiple logs follow the same format, ensure they align with a consistent pattern across all hooks (e.g., “TextureHook | CREATE | | Count: <this.textures>”).
42-42
: Consistent logs when deleting textures
Similarly, unify log messages for create and delete texture actions, so users can easily correlate the two events and track the texture lifecycle in the debug output.plugin-packages/stats/src/hooks/shader-hook.ts (4)
13-13
: Default debug parameter
Consider providing a default value for debug, consistent with other hooks. This avoids overhead for consumers uninterested in logs.constructor ( private readonly gl: WebGLRenderingContext | WebGL2RenderingContext, - private readonly debug: boolean, + private readonly debug: boolean = false, ) {
21-21
: Consolidate repeated debug usage
Repeated conditional checks in the constructor and other methods can be extracted into a dedicated method or a shared logger. This ensures DRY (Don’t Repeat Yourself) and helps keep your code more maintainable.
30-30
: Consistent naming
Here, the log message states "AttachShader". If you plan to keep a standard naming scheme across all hooks, consider prefixing with “ShaderHook” or a similarly descriptive tag to help in multi-hook environments.
39-39
: Maintain uniform debug logs
The detach debug message should ideally align with your attach debug message in content and style for easy tracking.plugin-packages/stats/src/hooks/drawcall-hook.ts (2)
16-16
: Default debug parameter
Consider setting a default value for debug to simplify usage for those who don’t need the logs, mirroring alterations in other hook classes.constructor ( private readonly gl: WebGLRenderingContext | WebGL2RenderingContext, - private readonly debug: boolean, + private readonly debug: boolean = false, ) {
24-24
: Unify debug message format
If you aim to keep debug logs consistent across the project, standardize the debug message. For instance, “DrawCallHook is hooked” might become “DrawCallHook | HOOKED” to match patterns in other hooks.plugin-packages/stats/src/stats.ts (2)
22-26
: Consider SSR or non-browser usage.
Defining “document.body” as the default container could raise issues in server-side rendering or non-browser contexts. If this library might run in such environments, consider providing a safe fallback or checking for “document” availability.container: typeof document !== 'undefined' ? document.body : null,
123-128
: Ensure existence of the item and component.
The code checks for “item”, but consider logging or handling the case where an item is unexpectedly missing. Otherwise, it’s straightforward how “defaultStatsOptions” merges.packages/effects-core/src/vfx-item.ts (1)
318-326
: Support for optional component parameters.
Allowing arbitrary options using “Record<string, any>” gives flexibility, but it lacks strict typing. Consider restricting it to a more defined type if possible. Otherwise, this is a solid approach to pass config data to components.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/effects-core/src/vfx-item.ts
(1 hunks)plugin-packages/stats/src/core.ts
(1 hunks)plugin-packages/stats/src/hooks/drawcall-hook.ts
(1 hunks)plugin-packages/stats/src/hooks/program-hook.ts
(2 hunks)plugin-packages/stats/src/hooks/shader-hook.ts
(3 hunks)plugin-packages/stats/src/hooks/texture-hook.ts
(3 hunks)plugin-packages/stats/src/monitor.ts
(7 hunks)plugin-packages/stats/src/stats-component.ts
(1 hunks)plugin-packages/stats/src/stats.ts
(3 hunks)
🔇 Additional comments (21)
plugin-packages/stats/src/core.ts (3)
27-27
: Use of optional debug parameter is a nice improvement.
The addition of the optional "debug" parameter makes the constructor more flexible. This allows for easily toggling debug logs without adding complexity to the call site. This design keeps the default behavior unchanged by setting "debug" to false when not specified.
32-32
: Good practice to centralize hook initialization in one call.
Making the call to “this.hook(gl, debug)” part of the constructor maintains a clean separation between construction and hooking your GL context with optional debugging. This is a neat approach for extensibility.
35-39
: Propagating "debug" setting to all hooks.
By passing "debug" along to each hook, you give each component the ability to produce debug logs independently. This ensures a consistent approach to debugging across all hooks (DrawCallHook, TextureHook, ShaderHook, ProgramHook). However, please confirm that each hook properly checks and handles the "debug" flag to avoid excessive logging in production.
If you want finer-grained control (e.g., enabling debug logs per hook), consider splitting the debug parameter or providing a debug configuration object to allow each hook to tailor its logging verbosity.
✅ Verification successful
Let me verify how these hooks handle the debug parameter by searching for their implementations.
Based on the search results, I can now verify how each hook handles the debug parameter. The results show that all hooks (DrawCallHook, TextureHook, ShaderHook, ProgramHook) properly implement and handle the debug flag:
- Each hook stores the debug flag as a private readonly property
- They all have consistent debug logging patterns:
- Initial hook setup message (e.g., "Texture is hooked.")
- Operation-specific debug messages (e.g., for texture creation/deletion)
- All debug logs are properly guarded behind debug flag checks using
if (this.debug)
Debug flag is properly implemented across all hooks.
Each hook consistently implements debug logging with appropriate guards, ensuring no excessive logging in production when debug is false.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for hook classes and their debug parameter usage
ast-grep --pattern 'class $_ {
constructor($$$, debug: boolean$$$) {
$$$
}
$$$
}'
# Also search for any debug-related logging or conditions
rg "debug" --type ts -C 2
Length of output: 17919
plugin-packages/stats/src/stats.ts (7)
1-1
: No functional issues found.
4-6
: No further context in doc comment.
8-19
: Refine interface documentation for clarity.
These lines properly introduce optional fields in “StatsOptions,” each with a default specified via comments. However, consider clarifying how these options interact with the rest of the system (e.g., if container is null or debug is toggled at runtime).
105-106
: Good implementation of the Disposable interface.
This aligns the “Stats” class with a consistent contract for disposal.
134-136
: Hide method is concise and clear.
The approach is simple and uses optional chaining for safety. No issues.
138-140
: Show method complements hide method well.
This symmetrical approach works fine; consider toggling for dynamic usage if needed.
142-144
: Dispose method properly cleans up resources.
Calling “this.component?.dispose()” ensures the underlying component is also released. Good practice.
plugin-packages/stats/src/stats-component.ts (3)
1-4
: Imports are appropriate.
Reflects the new dependency on “StatsOptions” and the “Engine” type.
12-18
: Constructor signature update is consistent.
Accepting “options: Required” ensures the component can access defaulted fields safely.
23-23
: Monitor instantiation with options is correct.
Passes the right arguments to “Monitor” and maintains clarity in the data flow.
plugin-packages/stats/src/monitor.ts (8)
4-4
: Added import fosters type consistency.
No issues found.
80-84
: Constructor usage of “options.debug” is clear.
Passing “options.debug” to “Core” is straightforward and ensures the debug flag is centrally controlled.
93-96
: Visibility check on creation.
Hiding the container when “visible” is false is a neat solution. Perfect approach.
135-137
: Hide method is straightforward.
Sets display to “none” — neatly implemented.
139-141
: Show method is symmetrical to hide.
Straightforward approach for restoring visibility.
173-173
: Remove container from DOM on dispose.
This is a good practice that eliminates potential memory leaks.
119-119
: Potential early return might skip partial updates.
Returning if “triangles === 0” could prevent collecting memory or FPS data in some corner cases. Confirm this logic is intended.
✅ Verification successful
Let me analyze the code context by first checking the surrounding implementation to understand the data collection logic.
Let me check what kind of data is being collected and what PerformanceData
contains to understand if skipping on zero triangles is valid.
Let me try again without the file type flag since it's not recognized.
Let's check the PerformanceData interface definition and Core class implementation to understand what data is being collected.
Now I have enough context to provide the final response:
Early return is correct and intentional
The early return when triangles === 0
is valid because:
- The
data
object contains all performance metrics (FPS, memory, draw calls, etc.) as a single atomic update - The Core class only returns data after a complete sampling period (60 frames)
- If there are no triangles rendered, it means no actual rendering occurred in that frame, making the performance data incomplete or invalid for that sampling period
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Let's look at the monitor.ts file content around this condition
rg -B 10 -A 10 "if \(!data \|\| data\.triangles === 0\)" plugin-packages/stats/src/monitor.ts
Length of output: 574
Script:
#!/bin/bash
# Let's find the definition of PerformanceData and Core class
rg -t typescript "interface PerformanceData" plugin-packages/stats/src
rg -t typescript "class Core" plugin-packages/stats/src
Length of output: 200
Script:
#!/bin/bash
# Let's find the definition of PerformanceData and Core class
rg "interface PerformanceData" plugin-packages/stats/src/
rg "class Core" plugin-packages/stats/src/
# Also let's check what other properties are used from data
rg "data\." plugin-packages/stats/src/monitor.ts
Length of output: 319
Script:
#!/bin/bash
# Let's see the content of core.ts
cat plugin-packages/stats/src/core.ts
Length of output: 2971
100-100
: Confirm container is present in the DOM.
Appending to “this.options.container” is generally fine, but ensure that container always exists in targeted usage contexts (e.g., shadow DOM or SSR).
✅ Verification successful
Let me analyze the codebase to verify the DOM container usage.
Let me check the initialization of options and container validation.
Let me check one final aspect about the options interface and initialization.
Let me check the StatsOptions type definition since it's imported but not shown in the current file.
Based on the complete context gathered, I can now provide the final response:
Container existence is properly handled in the codebase
The container is safely handled because:
- The
StatsOptions
interface properly types the container asHTMLElement
- A default container (
document.body
) is provided indefaultStatsOptions
- The options are merged with defaults via
{ ...defaultStatsOptions, ...this.options }
- The constructor receives
Required<StatsOptions>
, ensuring container is non-null
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for container-related code in the file
rg -A 5 -B 5 "container" "plugin-packages/stats/src/monitor.ts"
# Look for any tests that might verify container existence
fd "test|spec" plugin-packages/stats --exec rg -l "container"
# Check if there's any container initialization or validation
ast-grep --pattern 'class $_ {
$$$
constructor($$$) {
$$$
this.options = $_
$$$
}
$$$
}' "plugin-packages/stats/src/monitor.ts"
Length of output: 1529
Script:
#!/bin/bash
# Check the Monitor class constructor and options initialization
ast-grep --pattern 'constructor($_) {
$$$
this.options = $_
$$$
}' "plugin-packages/stats/src/monitor.ts"
# Look for options type definition
rg -A 5 "interface.*Options" "plugin-packages/stats/src/monitor.ts"
# Check if there's any validation before container usage
rg -B 5 "this.options.container" "plugin-packages/stats/src/monitor.ts"
Length of output: 443
Script:
#!/bin/bash
# Search for the entire file content to understand the complete context
cat "plugin-packages/stats/src/monitor.ts"
# Also check for any exports or types that might be in a separate file
fd "types|index" plugin-packages/stats/src --exec rg -l "Options"
Length of output: 3831
Script:
#!/bin/bash
# Look for StatsOptions definition in stats.ts
cat "plugin-packages/stats/src/stats.ts"
Length of output: 2662
|
||
export class StatsComponent extends Behaviour { | ||
/** | ||
* 监控对象 | ||
*/ | ||
monitor: Monitor; | ||
|
||
override onStart (): void { | ||
init (options: Required<StatsOptions>): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这块不能再用 onStart 生命周期了,看看是否有问题 @Sruimeng
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugin-packages/stats/src/stats.ts (1)
115-128
:⚠️ Potential issueFix async initialization pattern
The current implementation has the same issues as noted in previous reviews:
- Constructor ignores async initialization
- Methods may be called before initialization completes
- No way to await initialization
Consider implementing one of these patterns:
// Pattern 1: Static factory class Stats implements Disposable { static async create(player: Player, options?: StatsOptions): Promise<Stats> { const stats = new Stats(player, options); await stats.init(); return stats; } private constructor(player: Player, options?: StatsOptions) {} } // Pattern 2: Ready promise class Stats implements Disposable { readonly ready: Promise<void>; constructor(player: Player, options?: StatsOptions) { this.ready = this.init(); } }
🧹 Nitpick comments (2)
plugin-packages/stats/src/stats.ts (2)
4-19
: Document container element lifecycle managementThe
container
option allows injecting a custom HTML element, but there's no documentation about:
- Who owns the container lifecycle
- Whether the container is cleaned up on disposal
- What happens if the container is removed from DOM
Add JSDoc comments explaining these details:
/** + * Options for configuring the Stats plugin + */ export interface StatsOptions { /** + * Enable debug mode for additional logging * @default false */ debug?: boolean, /** + * Control initial visibility of the stats panel * @default true */ visible?: boolean, /** + * Custom container element for the stats panel + * Note: The container must be managed by the caller and remain in DOM + * while the stats plugin is active * @default document.body */ container?: HTMLElement, }
22-26
: Consider safer DOM access patternAccessing
document.body
during module initialization could cause issues in SSR environments or if the script loads before DOM is ready.Consider lazy initialization:
const defaultStatsOptions: Required<StatsOptions> = { debug: false, visible: true, - container: document.body, + get container() { + return document.body; + }, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/effects-core/src/vfx-item.ts
(1 hunks)plugin-packages/stats/src/stats-component.ts
(1 hunks)plugin-packages/stats/src/stats.ts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects-core/src/vfx-item.ts
🔇 Additional comments (1)
plugin-packages/stats/src/stats.ts (1)
134-140
: 🛠️ Refactor suggestion
Add initialization state checks
The visibility methods don't verify if the component is initialized, which could lead to silent failures.
Add state checks:
hide () {
+ if (!this.component) {
+ throw new Error('Stats component not initialized. Ensure to await initialization.');
+ }
this.component?.monitor.hide();
}
show () {
+ if (!this.component) {
+ throw new Error('Stats component not initialized. Ensure to await initialization.');
+ }
this.component?.monitor.show();
}
Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugin-packages/stats/src/stats.ts (1)
115-119
:⚠️ Potential issueAsync initialization needs improvement.
The constructor calls
init()
without awaiting, which could lead to race conditions if methods are called before initialization completes.Consider one of these approaches:
- constructor ( - public readonly player: Player, - private readonly options?: StatsOptions, - ) { - void this.init(); - } + private constructor ( + public readonly player: Player, + private readonly options?: StatsOptions, + ) {} + + static async create(player: Player, options?: StatsOptions): Promise<Stats> { + const stats = new Stats(player, options); + await stats.init(); + return stats; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin-packages/stats/src/stats.ts
(3 hunks)
🔇 Additional comments (6)
plugin-packages/stats/src/stats.ts (6)
1-1
: LGTM! Good practice adding Disposable interface.
The addition of the Disposable interface import is appropriate as the Stats class now implements proper cleanup.
8-19
: LGTM! Well-documented interface.
The StatsOptions interface is well-documented with JSDoc comments including default values for each property.
105-107
: LGTM! Good state management.
The class properly implements Disposable and maintains component state with private fields.
124-129
: LGTM! Proper error handling and component initialization.
The initialization code properly handles errors and correctly merges default options with provided ones.
135-141
: LGTM! Clean visibility control implementation.
The hide/show methods properly use optional chaining to safely access the monitor.
143-150
: LGTM! Proper disposal implementation.
The dispose method follows best practices:
- Checks for already disposed state
- Cleans up references
- Uses optional chaining for safety
Summary by CodeRabbit
Release Notes
New Features
hide
,show
, anddispose
added to theStats
class for better control over component visibility and lifecycle.Bug Fixes
Documentation