fix: render sprite incorrectly when config reusable caused by diff algorithm #45
Conversation
WalkthroughThe codebase underwent a refinement where the handling of sprite visibility was improved. A redundant block of code related to sprite size was also cleaned up. In the web demo, a simplification took place with the assignment of a hardcoded URL to a variable and the removal of a now-unnecessary variable assignment, streamlining the initialization process. Changes
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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- packages/effects-core/src/plugins/sprite/sprite-mesh.ts (1 hunks)
- web-packages/demo/src/single.ts (1 hunks)
Additional comments: 5
packages/effects-core/src/plugins/sprite/sprite-mesh.ts (3)
202-210: The early return when
selfDatais not visible ensures that invisible sprites do not process further, which is good for performance. However, ensure that whenselfDatabecomes visible again, all necessary data is correctly initialized, as the early return might skip some initialization steps that are only executed whenselfDatais visible.200-200: The assignment of
uColornow defaults to the values inmainDataArrayifselfData.coloris not provided. Verify that this change is intentional and that there are no cases whereselfData.colorbeingundefinedshould not default to the previous color inmainDataArray.194-213: The
elseblock on lines 204-209 setsmainDataArray[uSizeStart + 2]to -1 whenselfDatais not visible andinitis false. This seems to be a way to indicate that the sprite is not to be rendered. Confirm that setting this specific index to -1 is the correct way to handle this case and that it does not interfere with other parts of the rendering logic.web-packages/demo/src/single.ts (2)
6-6: The
jsonvariable is now assigned a hardcoded URL. Confirm that this is intentional and not a placeholder for testing purposes.9-15: The previous logic for loading a scene with
player.loadScene(json)has been removed. Verify that the scene loading is handled elsewhere or if this logic is no longer required.
| (async () => { | ||
| try { | ||
| const player = createPlayer(); | ||
|
|
||
| const comp = await player.loadScene(json); | ||
| } catch (e) { | ||
| console.error('biz', e); | ||
| } |
There was a problem hiding this comment.
The try block within the async function is empty. This could be an oversight. Either implement the intended logic or remove the async function if it's not needed.
The createPlayer function is defined but not used within the async function. Ensure that it is called appropriately or removed if it's no longer necessary.
| const uSizeStart = start + 4; | ||
| const uQuatStart = start + 8; | ||
| const uColorStart = start + 12; | ||
|
|
||
| if (!selfData.visible && !init) { | ||
| mainDataArray[uSizeStart + 2] = -1; | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const uColor = selfData.color || [mainDataArray[uColorStart], mainDataArray[uColorStart + 1], mainDataArray[uColorStart + 2], mainDataArray[uColorStart + 3]]; | ||
|
|
||
| // if (selfData.startSize) { | ||
| // selfData.transform.scaleBy(1 / selfData.startSize[0], 1 / selfData.startSize[1], 1); | ||
| // } | ||
|
|
||
| const tempPos = new Vector3(); | ||
| const tempQuat = new Quaternion(); | ||
| const tempScale = new Vector3(); |
There was a problem hiding this comment.
The variables tempPos, tempQuat, and tempScale are created but only used within the if (selfData.visible) block. Consider moving their declarations inside the block to limit their scope and improve readability.
| const uColor = selfData.color || [mainDataArray[uColorStart], mainDataArray[uColorStart + 1], mainDataArray[uColorStart + 2], mainDataArray[uColorStart + 3]]; | ||
|
|
||
| selfData.transform.assignWorldTRS(tempPos, tempQuat, tempScale); | ||
| if (selfData.visible) { | ||
| selfData.transform.assignWorldTRS(tempPos, tempQuat, tempScale); | ||
| } else { | ||
| if (!init) { | ||
| mainDataArray[uSizeStart + 2] = -1; | ||
|
|
||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential issue with the logic on lines 202-210. If selfData.visible is false and init is true, the function will continue executing without setting tempPos, tempQuat, and tempScale. This could lead to using uninitialized variables in the subsequent code. Ensure that this case is handled correctly and that there is no possibility of using uninitialized data.
Summary by CodeRabbit
Bug Fixes
Refactor