Refactor: extract some methods of the 2D renderers from the Engine #2391
Conversation
…pu the need value directly
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several static methods for creating materials in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2391 +/- ##
==========================================
Coverage ? 74.97%
==========================================
Files ? 524
Lines ? 27424
Branches ? 4132
==========================================
Hits ? 20560
Misses ? 6829
Partials ? 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (2)
packages/core/src/2d/sprite/SpriteMask.ts (1)
29-39: LGTM! Consider adding JSDoc comments for better documentation.The new static method
_createSpriteMaskMaterialeffectively encapsulates the creation and configuration of the sprite mask material. This aligns well with the PR objectives of extracting rendering-related functionality from theEngineclass.Consider adding JSDoc comments to describe the method's purpose, parameters, and return value. For example:
/** * Creates and configures a Material instance for sprite masking. * @param engine - The engine instance used to create the material. * @returns A configured Material instance for sprite masking. */ static _createSpriteMaskMaterial(engine): Material { // ... existing implementation ... }packages/core/src/2d/sprite/SpriteRenderer.ts (1)
31-58: Specify access modifier for_createSpriteMaterialmethodThe
_createSpriteMaterialmethod does not have an access modifier specified. If this method is intended for internal use, consider marking it asprivateorprotectedto reflect its intended accessibility.Apply this diff to specify the access modifier:
-static _createSpriteMaterial(engine: Engine, maskInteraction: SpriteMaskInteraction): Material { +private static _createSpriteMaterial(engine: Engine, maskInteraction: SpriteMaskInteraction): Material {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/core/src/2d/sprite/SpriteMask.ts (2 hunks)
- packages/core/src/2d/sprite/SpriteRenderer.ts (2 hunks)
- packages/core/src/2d/text/TextRenderer.ts (3 hunks)
- packages/core/src/Engine.ts (2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/core/src/2d/text/TextRenderer.ts
[failure] 12-12:
Replace·BlendFactor,·BlendOperation,·CullMode,·RenderQueueType,·Shader,·ShaderData,·ShaderProperty·with⏎··BlendFactor,⏎··BlendOperation,⏎··CullMode,⏎··RenderQueueType,⏎··Shader,⏎··ShaderData,⏎··ShaderProperty⏎packages/core/src/Engine.ts
[failure] 237-237:
Insert⏎·····
[failure] 238-238:
Replacethis,·SpriteMaskInteraction.VisibleInsideMaskwith⏎······this,⏎······SpriteMaskInteraction.VisibleInsideMask⏎····
[failure] 239-239:
Replacethis,·SpriteMaskInteraction.VisibleOutsideMaskwith⏎······this,⏎······SpriteMaskInteraction.VisibleOutsideMask⏎····
🔇 Additional comments (5)
packages/core/src/2d/sprite/SpriteMask.ts (1)
29-39: Verify the usage of the new_createSpriteMaskMaterialmethod.The new static method
_createSpriteMaskMaterialis not used within this file. To ensure it's properly integrated into the codebase:
- Confirm that this method is used in the
Engineclass or other relevant classes to create the sprite mask material.- Verify that any previous implementation in the
Engineclass for creating sprite mask materials has been replaced with a call to this new method.To help verify the integration, you can run the following script:
This will help ensure that the refactoring has been completed consistently across the codebase.
✅ Verification successful
The
_createSpriteMaskMaterialmethod is properly integrated.
- The new static method
_createSpriteMaskMaterialis utilized within theEngineclass as shown inpackages/core/src/Engine.ts:
this._spriteMaskDefaultMaterial = SpriteMask._createSpriteMaskMaterial(this);- There are no remaining references to old implementations (
createSpriteMaskMaterialor_spriteMaskDefaultMaterial) outside of their declarations, ensuring a clean refactoring.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of _createSpriteMaskMaterial in the codebase rg --type typescript "_createSpriteMaskMaterial" -C 5 # Search for potential old implementations in the Engine class rg --type typescript "class Engine" -A 50 | rg "createSpriteMaskMaterial|_spriteMaskDefaultMaterial"Length of output: 226
Script:
#!/bin/bash # Search for usages of _createSpriteMaskMaterial in .ts and .tsx files rg "_createSpriteMaskMaterial" -g "*.ts" -g "*.tsx" -C 5 # Search for potential old implementations in the Engine class within .ts and .tsx files rg "class Engine" -g "*.ts" -g "*.tsx" -A 50 | rg "createSpriteMaskMaterial|_spriteMaskDefaultMaterial"Length of output: 2515
packages/core/src/2d/text/TextRenderer.ts (3)
Line range hint
12-24: LGTM: Import statements are correctly updated.The new import statements for shader-related enums and the
Materialclass are appropriately added and grouped with related imports. This change supports the new_createTextMaterialmethod.🧰 Tools
🪛 GitHub Check: lint
[failure] 12-12:
Replace·BlendFactor,·BlendOperation,·CullMode,·RenderQueueType,·Shader,·ShaderData,·ShaderProperty·with⏎··BlendFactor,⏎··BlendOperation,⏎··CullMode,⏎··RenderQueueType,⏎··Shader,⏎··ShaderData,⏎··ShaderProperty⏎
36-51: LGTM: Well-implemented_createTextMaterialmethod.This new static method effectively encapsulates the creation of a default text material, improving the separation of concerns by moving this logic from the
Engineclass to theTextRendererclass. The material configuration is appropriate for 2D text rendering:
- Uses the correct "Text" shader.
- Configures alpha blending for proper text transparency.
- Disables depth writing and culling, which is suitable for 2D rendering.
- Sets the render queue to transparent.
- Marks the material as ignored by the garbage collector, which is good for potentially shared resources.
This refactoring enhances modularity and makes the code more maintainable.
Line range hint
1-1000: Summary: Excellent refactoring of text material creation.This change successfully refactors the creation of the default text material from the
Engineclass to theTextRendererclass. The new_createTextMaterialmethod is well-implemented and improves the separation of concerns. This refactoring enhances the modularity and maintainability of the codebase without introducing any apparent negative impacts on the existing functionality.Great job on this clean and focused improvement!
packages/core/src/Engine.ts (1)
10-10: Appropriate import of 2D renderer classesThe addition of
SpriteMask,SpriteMaskInteraction,SpriteRenderer, andTextRendererfrom the"./2d"module aligns with the refactoring objective to encapsulate rendering-related functionalities within their respective classes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/src/Engine.ts (1)
237-250: LGTM: Default sprite materials creation refactoredThe creation of default sprite materials has been successfully refactored to use
SpriteRenderer._createSpriteMaterial, which aligns with the PR objective of extracting 2D renderer methods from the Engine class.Consider adjusting the formatting slightly for consistency:
- this._spriteDefaultMaterial = spriteDefaultMaterials[SpriteMaskInteraction.None] = SpriteRenderer._createSpriteMaterial( - this, - SpriteMaskInteraction.None - ); + this._spriteDefaultMaterial = spriteDefaultMaterials[SpriteMaskInteraction.None] = SpriteRenderer._createSpriteMaterial(this, SpriteMaskInteraction.None);Apply this change to the other two similar blocks as well for consistency.
🧰 Tools
🪛 GitHub Check: lint
[failure] 238-238:
Replace·SpriteRenderer._createSpriteMaterial(⏎······this,⏎······SpriteMaskInteraction.None⏎····with⏎······SpriteRenderer._createSpriteMaterial(this,·SpriteMaskInteraction.Nonepackages/core/src/2d/text/TextRenderer.ts (1)
12-12: Reformat the import statement for better readabilityAccording to the static analysis hint, reformatting the import statement on line 12 enhances readability by listing each imported item on a separate line.
Apply this diff to reformat the import statement:
-import { BlendFactor, BlendOperation, CullMode, RenderQueueType, Shader, ShaderData, ShaderProperty } from "../../shader"; +import { + BlendFactor, + BlendOperation, + CullMode, + RenderQueueType, + Shader, + ShaderData, + ShaderProperty +} from "../../shader";🧰 Tools
🪛 GitHub Check: lint
[failure] 12-12:
Replace·BlendFactor,·BlendOperation,·CullMode,·RenderQueueType,·Shader,·ShaderData,·ShaderProperty·with⏎··BlendFactor,⏎··BlendOperation,⏎··CullMode,⏎··RenderQueueType,⏎··Shader,⏎··ShaderData,⏎··ShaderProperty⏎
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/core/src/2d/sprite/SpriteMask.ts (2 hunks)
- packages/core/src/2d/sprite/SpriteRenderer.ts (2 hunks)
- packages/core/src/2d/text/TextRenderer.ts (3 hunks)
- packages/core/src/Engine.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/2d/sprite/SpriteMask.ts
- packages/core/src/2d/sprite/SpriteRenderer.ts
🧰 Additional context used
🪛 GitHub Check: lint
packages/core/src/2d/text/TextRenderer.ts
[failure] 12-12:
Replace·BlendFactor,·BlendOperation,·CullMode,·RenderQueueType,·Shader,·ShaderData,·ShaderProperty·with⏎··BlendFactor,⏎··BlendOperation,⏎··CullMode,⏎··RenderQueueType,⏎··Shader,⏎··ShaderData,⏎··ShaderProperty⏎packages/core/src/Engine.ts
[failure] 238-238:
Replace·SpriteRenderer._createSpriteMaterial(⏎······this,⏎······SpriteMaskInteraction.None⏎····with⏎······SpriteRenderer._createSpriteMaterial(this,·SpriteMaskInteraction.None
🔇 Additional comments (2)
packages/core/src/Engine.ts (2)
10-10: LGTM: Import statements updated to support refactoringThe new imports for
SpriteMask,SpriteMaskInteraction,SpriteRenderer, andTextRendererfrom "./2d" align with the PR objective of extracting 2D renderer methods from the Engine class.
253-254: LGTM: Default text and sprite mask materials creation refactoredThe creation of default text and sprite mask materials has been successfully refactored to use
TextRenderer._createTextMaterialandSpriteMask._createSpriteMaskMaterialrespectively. This change aligns with the PR objective of extracting 2D renderer methods from the Engine class and improves code organization.
| static _createTextMaterial(engine: Engine): Material { | ||
| const material = new Material(engine, Shader.find("Text")); | ||
| const renderState = material.renderState; | ||
| const target = renderState.blendState.targetBlendState; | ||
| target.enabled = true; | ||
| target.sourceColorBlendFactor = BlendFactor.SourceAlpha; | ||
| target.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha; | ||
| target.sourceAlphaBlendFactor = BlendFactor.One; | ||
| target.destinationAlphaBlendFactor = BlendFactor.OneMinusSourceAlpha; | ||
| target.colorBlendOperation = target.alphaBlendOperation = BlendOperation.Add; | ||
| renderState.depthState.writeEnabled = false; | ||
| renderState.rasterState.cullMode = CullMode.Off; | ||
| renderState.renderQueueType = RenderQueueType.Transparent; | ||
| material.isGCIgnored = true; | ||
| return material; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use TypeScript access modifiers instead of underscore prefixes
The method _createTextMaterial uses an underscore prefix to indicate it's intended for internal use. In TypeScript, it's recommended to use the private or protected access modifiers to enforce encapsulation rather than relying on naming conventions.
Apply this diff to make the method private:
-static _createTextMaterial(engine: Engine): Material {
+private static _createTextMaterial(engine: Engine): Material {Committable suggestion was skipped due to low confidence.
|
see: singlecoder#15 |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Extract some methods of the 2D renderers from the Engine. The code of
Engineshould be clean!Just like
this._textDefaultFontis implemented throughFont.createFromOS, it would be more reasonable for_spriteDefaultMaterialto be implemented through the static method of its renderer class.Summary by CodeRabbit
New Features
RenderQueueMaskTypeto categorize render queue masks.Refactor
Bug Fixes