-
-
Notifications
You must be signed in to change notification settings - Fork 307
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: opt memory for texture of font atlas #2287
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe updates to the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/2d/atlas/FontAtlas.ts (4 hunks)
- packages/core/src/2d/text/SubFont.ts (2 hunks)
Additional context used
Biome
packages/core/src/2d/atlas/FontAtlas.ts
[error] 111-111: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 112-112: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (4)
packages/core/src/2d/text/SubFont.ts (1)
4-4
: Verify the correct handling and initialization ofTexture2D
.The instantiation and configuration of
Texture2D
have been removed. Ensure that the texture is correctly managed and initialized elsewhere to avoid runtime errors.Verification successful
Verified the correct handling and initialization of
Texture2D
.The initialization and configuration of
Texture2D
have been found in multiple locations across the codebase, confirming that texture handling is correctly managed elsewhere.
- Examples:
examples/video-transparent.ts
,examples/video-background.ts
,examples/texture-mipmap.ts
, etc.- Packages:
packages/loader/src/ktx2/KTX2Loader.ts
,packages/loader/src/resource-deserialize/resources/texture2D/TextureDecoder.ts
, etc.- Core:
packages/core/src/Engine.ts
,packages/core/src/2d/atlas/FontAtlas.ts
, etc.- Tests:
tests/src/core/SpriteMask.test.ts
,tests/src/core/SpriteRenderer.test.ts
, etc.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling and initialization of `Texture2D`. # Test: Search for the initialization of `Texture2D` in the codebase. Expect: Initialization and configuration handled elsewhere. rg --type-add 'ts' --type ts 'new Texture2D'Length of output: 104
Script:
#!/bin/bash # Description: Verify the correct handling and initialization of `Texture2D`. # Test: Search for the initialization of `Texture2D` in TypeScript files. Expect: Initialization and configuration handled elsewhere. rg 'new Texture2D' --glob '*.ts' -A 5Length of output: 35926
packages/core/src/2d/atlas/FontAtlas.ts (3)
19-29
: LGTM! But verify the consistency with the texture management strategy.The initialization of
_curSize
and_preSize
variables and the setup of the texture with bilinear filtering are straightforward. Ensure that these changes are consistent with the overall texture management strategy.Verification successful
Verified: The usage of
_curSize
and_preSize
is consistent with the overall texture management strategy.The variables
_curSize
and_preSize
are used within theFontAtlas.ts
file to manage texture sizes and positions, aligning with the intended texture management strategy.
packages/core/src/2d/atlas/FontAtlas.ts
: Lines 19-29Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency with the texture management strategy. # Test: Search for the usage of `_curSize` and `_preSize` in the codebase. Expect: Consistent usage with the overall texture management strategy. rg --type-add 'ts' --type ts '_curSize|_preSize'Length of output: 108
Script:
#!/bin/bash # Description: Verify the consistency with the texture management strategy. # Test: Search for the usage of `_curSize` and `_preSize` in the TypeScript codebase. Expect: Consistent usage with the overall texture management strategy. rg '_curSize|_preSize' --glob '*.ts'Length of output: 547
110-137
: LGTM! But verify the correctness of the UV coordinate updates.The
_resizeTexture
method handles the resizing of the texture and updates UV coordinates. Ensure that the method is correctly implemented and does not introduce any issues with UV coordinate updates.Verification successful
Verified: The UV coordinate updates in
_resizeTexture
are correctly implemented.The
_resizeTexture
method is invoked when the current texture size is exceeded. The UV coordinates are correctly scaled by 0.5, which aligns with the texture size doubling. No issues were found with the UV coordinate updates.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the UV coordinate updates. # Test: Search for the usage of `_resizeTexture` in the codebase. Expect: Correct implementation of UV coordinate updates. rg --type-add 'ts' --type ts '_resizeTexture'Length of output: 103
Script:
#!/bin/bash # Description: Verify the correctness of the UV coordinate updates. # Test: Search for the usage of `_resizeTexture` in the codebase. Expect: Correct implementation of UV coordinate updates. rg '_resizeTexture'Length of output: 165
Script:
#!/bin/bash # Description: Extract surrounding lines of the `_resizeTexture` method call to understand the context. # Extract 10 lines before and after the `_resizeTexture` method call. rg '_resizeTexture' -A 10 -B 10Length of output: 3001
Tools
Biome
[error] 111-111: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 112-112: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
38-61
: LGTM! But verify the correctness and performance of the resizing logic.The changes to the
uploadCharTexture
method improve the handling of texture size and resizing logic. Ensure that the resizing logic is correctly implemented and does not introduce any performance issues.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Texture2D
within the font atlas.