Entity support add other Transform
#2456
Conversation
cptbtptpbcptdtptp
changed the title
Transform
cptbtptpbcptdtptp
changed the title
TransformTransform
cptbtptpbcptdtptp
changed the title
TransformTransform
cptbtptpbcptdtptp
requested review from
singlecoder,
GuoLei1990,
luzhuang and
johanzhu
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces significant modifications across multiple classes in the codebase, primarily focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/core/src/ComponentsDependencies.ts (2)
55-55: Correct grammatical error in error messageThe error message should use "removing" instead of "remove" for proper grammar.
Apply this diff to correct the error message:
- throw `Should remove ${invDependencies[i].name} before remove ${type.name}`; + throw `Should remove ${invDependencies[i].name} before removing ${type.name}`;
63-80: Consider optimizing nested loops in_getInheritedComponentsThe current implementation uses nested loops over components and inherited components, resulting in O(N*M) time complexity. If the number of components or inherited components grows large, this may impact performance. Consider using a
SetforinheritedComponentsto reduce lookup time.Here's a suggested refactor:
- for (let i = 0, n = components.length; i < n; i++) { - const component = components[i]; - for (let j = 0; j < inheritedComponentsLength; j++) { - if (component instanceof inheritedComponents[j]) { - out.push(component.constructor as ComponentConstructor); - break; - } - } - } + const inheritedSet = new Set<ComponentConstructor>(inheritedComponents); + for (let i = 0, n = components.length; i < n; i++) { + const component = components[i]; + if (inheritedSet.has(component.constructor as ComponentConstructor)) { + out.push(component.constructor as ComponentConstructor); + } + }tests/src/core/CompomentDependencies.test.ts (1)
29-29: Correct grammatical error in error messageThe error message should use "removing" instead of "remove" for proper grammar.
Apply this diff to correct the error message:
- }).throw("Should remove CustomScriptA before remove MeshRenderer"); + }).throw("Should remove CustomScriptA before removing MeshRenderer");packages/core/src/physics/joint/Joint.ts (2)
37-38: LGTM! Consider extracting the flag manager accessThe change simplifies update flag management by accessing it directly from the entity. This is consistent with the optimization pattern seen elsewhere.
Consider extracting the flag manager access to reduce duplication:
- this._connectedColliderInfo.collider?.entity._updateFlagManager.removeListener(this._onConnectedTransformChanged); - value?.entity._updateFlagManager.addListener(this._onConnectedTransformChanged); + const oldFlagManager = this._connectedColliderInfo.collider?.entity._updateFlagManager; + const newFlagManager = value?.entity._updateFlagManager; + oldFlagManager?.removeListener(this._onConnectedTransformChanged); + newFlagManager?.addListener(this._onConnectedTransformChanged);
37-38: Consider documenting the new entity-based flag management patternThe changes consistently move from transform-based to entity-based flag management across multiple files. This architectural change improves efficiency but should be documented for maintainers.
Consider:
- Adding a comment in the Entity class documenting this design decision
- Updating relevant architecture documentation
- Creating a migration guide if this is a breaking change
packages/core/src/Transform.ts (1)
927-936: Consider enhancing TransformTransitionState documentationThe interface documentation could be more detailed about its purpose and usage.
Add more detailed JSDoc comments:
/** + * Represents the transition state of a transform component. + * Used to store and restore transform state during animations or transitions. * @remarks: * The reason for not recording the world matrix is that * the matrix may get wrong local information when decomposing it. + * @example + * ```ts + * const state: TransformTransitionState = {}; + * transform._generateTransitionState(state); + * // ... perform transition + * transform._applyTransitionState(state); + * ``` */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
packages/core/src/Camera.ts(5 hunks)packages/core/src/ComponentsDependencies.ts(6 hunks)packages/core/src/Entity.ts(7 hunks)packages/core/src/Renderer.ts(4 hunks)packages/core/src/Transform.ts(5 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/mesh/SkinnedMeshRenderer.ts(3 hunks)packages/core/src/physics/Collider.ts(1 hunks)packages/core/src/physics/joint/Joint.ts(1 hunks)tests/src/core/CompomentDependencies.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/Transform.ts
[error] 584-585: 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] 585-586: 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] 586-587: 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)
packages/core/src/Entity.ts
[error] 232-232: Do not access Object.prototype method 'isPrototypeOf' from target object.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (30)
packages/core/src/ComponentsDependencies.ts (5)
4-4: LGTM
The export of ComponentConstructor allows it to be used externally, which is appropriate.
14-15: Static properties initialized correctly
The addition of _mutuallyExclusiveMap and _inheritedComponents as static properties is correctly implemented.
36-41: Proper handling of mutually exclusive components
The logic to destroy mutually exclusive components during the add operation is correctly implemented.
111-126: Implementation of _markComponentsMutuallyExclusive is correct
The method _markComponentsMutuallyExclusive correctly handles the registration of mutually exclusive components.
157-188: Addition of mutuallyExclusiveComponents and inherited functions
The new functions enhance component dependency management and are implemented properly.
packages/core/src/mesh/SkinnedMeshRenderer.ts (4)
123-124: Updating world matrix based on rootBone
The modification correctly updates the world matrix to use the rootBone's transform when available, ensuring accurate skinning transformations.
223-223: Correctly updating bounds with rootBone's world matrix
The update ensures that the world bounds are accurately calculated using the rootBone's transform.
287-288: Properly updating listeners for root bone transform changes
Listeners for the root bone's transform updates are correctly managed during skin application.
Line range hint 290-299: Deprecation of rootBone and bones properties
The properties are correctly marked as deprecated with appropriate documentation, guiding users to use this.skin.rootBone and this.skin.bones instead.
packages/core/src/Entity.ts (11)
2-2: Importing ignoreClone correctly
The import statement for ignoreClone is correct.
5-5: Importing ComponentConstructor and ComponentsDependencies
Importing ComponentConstructor and ComponentsDependencies appropriately.
10-11: Adding necessary imports
The imports for Transform, TransformTransitionState, and UpdateFlagManager are correctly added.
22-26: Addition of static properties for component inheritance and matrix
Static properties _inheritedComponents and _matrix are appropriately added to support component inheritance and transformations.
104-106: Adding _updateFlagManager with @ignoreClone
The _updateFlagManager is added with the @ignoreClone decorator, which is appropriate for managing update flags without affecting cloning.
108-109: Adding private properties for transform management
Private properties _transform and _transformTransitionState are correctly added to manage the entity's transform state transitions.
114-135: Implementing transform getter and setter
The getter and setter for transform are properly implemented, handling transition states as expected.
227-237: Enhanced constructor accepts components
The constructor now accepts component constructors, allowing components to be added upon entity creation. This improves usability and flexibility.
🧰 Tools
🪛 Biome (1.9.4)
[error] 232-232: Do not access Object.prototype method 'isPrototypeOf' from target object.
(lint/suspicious/noPrototypeBuiltins)
434-436: Modify createChild to inherit components from parent
The createChild method now ensures that inherited components are added to the child entity, enhancing component inheritance behavior.
471-478: Adding registerWorldChangeFlag method
The registerWorldChangeFlag method allows entities to register for world transform change notifications, which is useful for tracking changes.
528-528: Exclude Transform component during cloning
The cloning logic correctly excludes the Transform component, as it is managed separately.
packages/core/src/index.ts (1)
15-15: Verify the implementation of newly exported functions
The addition of mutuallyExclusiveComponents and inherited exports expands the public API. Let's verify their implementation.
packages/core/src/physics/Collider.ts (1)
40-40: LGTM! Optimization improves code efficiency
The change simplifies the world change flag registration by directly using the entity instance, reducing property access chain depth. This aligns with the PR's optimization objectives.
✅ Verification successful
Let me analyze the usage of registerWorldChangeFlag across the codebase to ensure consistency. I'll check the implementation and context of both patterns.
Let me check the relationship between these components and their inheritance hierarchy to understand if the different patterns are justified.
Let me check the implementation of registerWorldChangeFlag in Entity to understand if the change is safe.
Let me try a different approach to check the Entity and Transform implementations.
Based on the findings, I can now generate the final response:
Change from transform.registerWorldChangeFlag() to entity.registerWorldChangeFlag() is safe
The change is correct and consistent with the codebase patterns:
- Entity directly implements
registerWorldChangeFlag()which creates aBoolUpdateFlag - Other core components (Camera, Collider) already use
entity.registerWorldChangeFlag() - Only the physics-lite package uses
transform.registerWorldChangeFlag()which is a separate implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of world change flag registration across the codebase
# Check for any remaining instances of the old pattern
rg "transform\.registerWorldChangeFlag" --type ts
# Check for adoption of the new pattern
rg "entity\.registerWorldChangeFlag" --type ts
Length of output: 600
Script:
#!/bin/bash
# Check the implementation of registerWorldChangeFlag in both Entity and Transform
ast-grep --pattern 'registerWorldChangeFlag() {
$$$
}'
# Check the LiteColliderShape implementation context
rg -B 5 -A 5 "_transform\.registerWorldChangeFlag" packages/physics-lite/src/shape/LiteColliderShape.ts
Length of output: 425
Script:
#!/bin/bash
# Check the class definitions and inheritance
ast-grep --pattern 'class $_ extends $_ {
$$$
}'
# Check imports and dependencies
rg -l "import.*Transform" --type ts
Length of output: 67501
Script:
#!/bin/bash
# Check Entity's registerWorldChangeFlag implementation
ast-grep --pattern 'class Entity {
$$$
registerWorldChangeFlag() {
$$$
}
$$$
}'
# Check if Transform has any special handling around world change flags
rg -B 3 -A 3 "registerWorldChangeFlag" packages/core/src/Transform.ts
Length of output: 163
Script:
#!/bin/bash
# Search for Entity class implementation with world change flags
rg -B 5 -A 5 "class Entity" packages/core/src/Entity.ts
# Check Transform class implementation around world flags
rg -B 5 -A 5 "worldChangeFlag" packages/core/src/Transform.ts
# Check all usages of registerWorldChangeFlag
rg -B 2 -A 2 "registerWorldChangeFlag" --type ts
Length of output: 2604
packages/core/src/Transform.ts (2)
774-774: LGTM: Improved event dispatching
The change to use entity._updateFlagManager for dispatching transform modification events is a good improvement for centralized event handling.
343-344: Verify entity.transform assignment
The constructor now assigns this transform instance to entity.transform. This change needs verification to ensure it doesn't create circular references or memory leaks.
✅ Verification successful
Let me gather more information about the Transform class and its cleanup handling.
Let me try a different approach to check the Transform class implementation.
Let me try one final verification to check the Component class since Transform extends it.
Transform-Entity relationship is properly managed
Based on the verification results:
- The Transform class extends Component, which properly manages the entity relationship
- The Component class has a protected
_entityfield with proper cleanup (setting to null when destroyed) - The original code shows
entity.transform = nullis called during cleanup in Transform.ts
The bidirectional reference between Entity and Transform is properly managed with appropriate cleanup, preventing memory leaks and circular reference issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential memory leaks in transform assignment
# Check if transform is properly cleaned up in entity
rg -A 5 "entity\.transform\s*=\s*null"
# Check for circular references
ast-grep --pattern 'class Entity {
$$$
transform: Transform
$$$
}'
Length of output: 577
Script:
#!/bin/bash
# Check Transform class implementation for cleanup and entity relationship
ast-grep --pattern 'class Transform {
$$$
_entity: $_
$$$
}'
# Check for destroy/cleanup methods
rg -A 10 "destroy\(\)" packages/core/src/Transform.ts
# Check entity assignment in Transform
rg -B 5 -A 5 "_entity\s*=\s*entity" packages/core/src/Transform.ts
Length of output: 193
Script:
#!/bin/bash
# Get Transform class implementation focusing on entity relationship
rg -A 5 "class Transform" packages/core/src/Transform.ts
# Check entity-related methods and properties
rg -A 3 "_entity:" packages/core/src/Transform.ts
# Look for cleanup/destroy related code
rg -A 3 "this\._entity\s*=" packages/core/src/Transform.ts
Length of output: 492
Script:
#!/bin/bash
# Check Component class implementation for entity handling
rg -A 10 "class Component" packages/core/src/Component.ts
# Check destroy/cleanup in Component
rg -A 5 "destroy\(\)" packages/core/src/Component.ts
# Check entity handling in Component
rg -A 5 "_entity\s*=" packages/core/src/Component.ts
Length of output: 457
packages/core/src/Camera.ts (3)
316-319: LGTM: Simplified transform access
The viewMatrix getter now correctly uses entity.transform instead of maintaining a separate transform reference.
Line range hint 606-610: LGTM: Consistent transform usage
The render method has been updated to use entity.transform consistently.
Line range hint 786-790: LGTM: Improved shader data updates
The _updateShaderData method now correctly uses entity.transform for updating shader properties.
packages/core/src/Renderer.ts (3)
173-173: LGTM: Proper event listener registration
The constructor correctly registers the transform change listener with the entity's update flag manager.
367-367: LGTM: Clean event listener cleanup
The _onDestroy method properly removes the transform change listener.
Line range hint 393-398: LGTM: Consistent transform usage
The _updateTransformShaderData method correctly uses entity.transform for updating shader data.
packages/core/src/Entity.ts
Outdated
| this._inverseWorldMatFlag = this.transform.registerWorldChangeFlag(); | ||
| for (let i = 0, n = components.length; i < n; i++) { | ||
| const type = components[i]; | ||
| if (!Transform.prototype.isPrototypeOf(type.prototype) || !this.transform) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid directly accessing isPrototypeOf from prototype
Using Transform.prototype.isPrototypeOf(type.prototype) may cause issues if isPrototypeOf is overridden. Instead, consider using Object.prototype.isPrototypeOf.call(Transform.prototype, type.prototype) for safety.
Apply this diff to make the change:
- if (!Transform.prototype.isPrototypeOf(type.prototype) || !this.transform) {
+ if (!Object.prototype.isPrototypeOf.call(Transform.prototype, type.prototype) || !this.transform) {📝 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.
| if (!Transform.prototype.isPrototypeOf(type.prototype) || !this.transform) { | |
| if (!Object.prototype.isPrototypeOf.call(Transform.prototype, type.prototype) || !this.transform) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 232-232: Do not access Object.prototype method 'isPrototypeOf' from target object.
(lint/suspicious/noPrototypeBuiltins)
packages/core/src/Transform.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| _generateTransitionState(state: TransformTransitionState): void { | ||
| state.position ? state.position.copyFrom(this._position) : (state.position = this._position.clone()); | ||
| state.rotation ? state.rotation.copyFrom(this._rotation) : (state.rotation = this._rotation.clone()); | ||
| state.scale ? state.scale.copyFrom(this._scale) : (state.scale = this._scale.clone()); | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| _applyTransitionState(state: TransformTransitionState): void { | ||
| this._position.copyFrom(state.position); | ||
| this._rotation.copyFrom(state.rotation); | ||
| this._scale.copyFrom(state.scale); | ||
| } |
There was a problem hiding this comment.
Fix assignments in expressions and consider adding validation
The transition state implementation has the following issues:
- Assignments in expressions can be confusing and should be avoided
- No validation is performed on the input state object
Apply this diff to fix the assignments and add validation:
_generateTransitionState(state: TransformTransitionState): void {
- state.position ? state.position.copyFrom(this._position) : (state.position = this._position.clone());
- state.rotation ? state.rotation.copyFrom(this._rotation) : (state.rotation = this._rotation.clone());
- state.scale ? state.scale.copyFrom(this._scale) : (state.scale = this._scale.clone());
+ if (!state) {
+ throw new Error("TransformTransitionState cannot be null");
+ }
+ if (state.position) {
+ state.position.copyFrom(this._position);
+ } else {
+ state.position = this._position.clone();
+ }
+ if (state.rotation) {
+ state.rotation.copyFrom(this._rotation);
+ } else {
+ state.rotation = this._rotation.clone();
+ }
+ if (state.scale) {
+ state.scale.copyFrom(this._scale);
+ } else {
+ state.scale = this._scale.clone();
+ }
}📝 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.
| /** | |
| * @internal | |
| */ | |
| _generateTransitionState(state: TransformTransitionState): void { | |
| state.position ? state.position.copyFrom(this._position) : (state.position = this._position.clone()); | |
| state.rotation ? state.rotation.copyFrom(this._rotation) : (state.rotation = this._rotation.clone()); | |
| state.scale ? state.scale.copyFrom(this._scale) : (state.scale = this._scale.clone()); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _applyTransitionState(state: TransformTransitionState): void { | |
| this._position.copyFrom(state.position); | |
| this._rotation.copyFrom(state.rotation); | |
| this._scale.copyFrom(state.scale); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _generateTransitionState(state: TransformTransitionState): void { | |
| if (!state) { | |
| throw new Error("TransformTransitionState cannot be null"); | |
| } | |
| if (state.position) { | |
| state.position.copyFrom(this._position); | |
| } else { | |
| state.position = this._position.clone(); | |
| } | |
| if (state.rotation) { | |
| state.rotation.copyFrom(this._rotation); | |
| } else { | |
| state.rotation = this._rotation.clone(); | |
| } | |
| if (state.scale) { | |
| state.scale.copyFrom(this._scale); | |
| } else { | |
| state.scale = this._scale.clone(); | |
| } | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _applyTransitionState(state: TransformTransitionState): void { | |
| this._position.copyFrom(state.position); | |
| this._rotation.copyFrom(state.rotation); | |
| this._scale.copyFrom(state.scale); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 584-585: 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] 585-586: 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] 586-587: 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)
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
packages/core/src/ComponentsDependencies.ts (2)
63-80: Consider optimizing inherited components lookupTo improve performance when checking for inherited components, consider using a
SetforComponentsDependencies._inheritedComponentsto achieve faster lookups.Apply this diff to use a
Set:-static _inheritedComponents: ComponentConstructor[] = []; +static _inheritedComponents: Set<ComponentConstructor> = new Set(); -static _getInheritedComponents(entity: Entity, out: ComponentConstructor[]): void { +static _getInheritedComponents(entity: Entity, out: Set<ComponentConstructor>): void { out.clear(); const components = entity._components; - const inheritedComponents = ComponentsDependencies._inheritedComponents; + const inheritedComponents = ComponentsDependencies._inheritedComponents; for (let i = 0, n = components.length; i < n; i++) { const component = components[i]; - for (let j = 0; j < inheritedComponentsLength; j++) { - if (component instanceof inheritedComponents[j]) { - out.push(component.constructor as ComponentConstructor); - break; - } - } + if (inheritedComponents.has(component.constructor as ComponentConstructor)) { + out.add(component.constructor as ComponentConstructor); + } } }
179-188: Consider using a Set for_inheritedComponentsTo optimize the performance of checking for existing inherited components, consider using a
Setinstead of an array forComponentsDependencies._inheritedComponents.Apply this diff to use a
Set:-static _inheritedComponents: ComponentConstructor[] = []; +static _inheritedComponents: Set<ComponentConstructor> = new Set(); -export function inherited() { return function <T extends ComponentConstructor>(target: T): void { - const inheritedComponents = ComponentsDependencies._inheritedComponents; - inheritedComponents.includes(target) || inheritedComponents.push(target); + ComponentsDependencies._inheritedComponents.add(target); }; }packages/core/src/Renderer.ts (4)
173-173: Usethis.entityinstead ofentityfor consistencyFor clarity and to follow class member conventions, use
this.entitywhen accessing the entity instance.Apply this diff:
-entity._updateFlagManager.addListener(this._onTransformChanged); +this.entity._updateFlagManager.addListener(this._onTransformChanged);
367-367: Usethis.entityinstead ofthis._entityTo adhere to encapsulation principles, use the public getter
this.entityinstead of accessing the private_entityproperty directly.Apply this diff:
-this._entity._updateFlagManager.removeListener(this._onTransformChanged); +this.entity._updateFlagManager.removeListener(this._onTransformChanged);
393-393: Usethis.entityinstead ofthis._entityAccess
this.entity.transform.worldMatrixto maintain consistency and encapsulation within the class.Apply this diff:
-const worldMatrix = this._entity.transform.worldMatrix; +const worldMatrix = this.entity.transform.worldMatrix;
443-443: Usethis.entityinstead ofthis._entityFor better encapsulation, use the public getter when accessing the entity's transform.
Apply this diff:
-shaderData.setMatrix(Renderer._localMatrixProperty, this._entity.transform.localMatrix); +shaderData.setMatrix(Renderer._localMatrixProperty, this.entity.transform.localMatrix);tests/src/core/CompomentDependencies.test.ts (1)
29-29: Correct grammatical error in error messageThe error message contains a grammatical error. Consider changing "before remove" to "before removing" for proper grammar.
Apply this diff to correct the error message:
- }).throw("Should remove CustomScriptA before remove MeshRenderer"); + }).throw("Should remove CustomScriptA before removing MeshRenderer");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
packages/core/src/Camera.ts(5 hunks)packages/core/src/ComponentsDependencies.ts(6 hunks)packages/core/src/Entity.ts(7 hunks)packages/core/src/Renderer.ts(4 hunks)packages/core/src/Transform.ts(5 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/mesh/SkinnedMeshRenderer.ts(3 hunks)packages/core/src/physics/Collider.ts(1 hunks)packages/core/src/physics/joint/Joint.ts(1 hunks)tests/src/core/CompomentDependencies.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/Transform.ts
[error] 584-585: 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] 585-586: 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] 586-587: 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)
packages/core/src/Entity.ts
[error] 232-232: Do not access Object.prototype method 'isPrototypeOf' from target object.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (13)
packages/core/src/ComponentsDependencies.ts (6)
4-4: LGTM!
The type definition for ComponentConstructor is correctly exported and will enhance type safety across the module.
14-15: Initialization of static properties is appropriate
The addition of _mutuallyExclusiveMap and _inheritedComponents properties is well-structured and will aid in managing component dependencies.
36-41: Logic for handling mutually exclusive components is correct
The code correctly removes mutually exclusive components before adding a new component, ensuring that only one of the mutually exclusive components exists on an entity at any time.
55-55: Updated error message improves clarity
The error message in _removeCheck has been updated to "Should remove X before remove Y," enhancing clarity and providing accurate guidance during component removal.
111-126: Logic for marking mutually exclusive components is correct
The _markComponentsMutuallyExclusive method effectively updates the mutually exclusive components map, ensuring consistency and preventing duplication.
157-177: Function overloading for mutuallyExclusiveComponents is well-implemented
The overloaded functions for mutuallyExclusiveComponents correctly handle both single and multiple component declarations, enhancing usability.
packages/core/src/index.ts (1)
15-15: LGTM!
The addition of mutuallyExclusiveComponents and inherited exports enhances component management capabilities.
packages/core/src/physics/Collider.ts (1)
40-40: LGTM!
Initializing _updateFlag with entity.registerWorldChangeFlag() aligns with the updated entity methods and ensures proper update flag registration.
packages/core/src/physics/joint/Joint.ts (1)
37-38: LGTM! Simplified transform update listener management.
The change streamlines the property access chain by directly accessing the update flag manager from the entity, which is consistent with the optimization pattern.
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
LGTM! The change aligns with the codebase's update flag management pattern.
The verification confirms that accessing _updateFlagManager directly from entity is the consistent pattern across the codebase. The search results show that:
- Entities own their
_updateFlagManagerinstances - Components and other objects consistently access the manager directly from their owning entity
- No instances of accessing
_updateFlagManagerthrough transform were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of accessing _updateFlagManager through transform
rg "transform\._updateFlagManager"
Length of output: 36
Script:
#!/bin/bash
# Let's search for any patterns of accessing _updateFlagManager through different paths
rg "_updateFlagManager" -A 2 -B 2
# Also search for similar listener management patterns
ast-grep --pattern 'addListener($_)'
Length of output: 21065
packages/core/src/Camera.ts (4)
424-426: LGTM! Consistent entity-level flag registration.
The change properly initializes change flags at the entity level, which aligns with the optimization pattern seen across the codebase.
316-316: LGTM! Simplified transform access in viewMatrix calculation.
Direct access to entity's transform improves code clarity while maintaining the optimization pattern.
606-606: LGTM! Consistent transform access in render method.
Direct access to entity's transform ensures consistent transform state during rendering operations.
786-786: LGTM! Consistent transform access in shader operations.
Direct entity transform access is properly maintained across shader-related operations, ensuring consistency with the optimization pattern.
Also applies to: 804-804
| @@ -284,6 +284,8 @@ export class SkinnedMeshRenderer extends MeshRenderer { | |||
| this._onSkinUpdated(SkinUpdateFlag.BoneCountChanged, skinBoneCount); | |||
| } | |||
| if (lastRootBone !== rootBone) { | |||
| lastRootBone?._updateFlagManager.removeListener(this._onTransformChanged); | |||
| rootBone._updateFlagManager.addListener(this._onTransformChanged); | |||
There was a problem hiding this comment.
Add null check for rootBone before adding listener
Accessing rootBone._updateFlagManager without ensuring rootBone is not null can result in runtime errors. Add a null check before adding the listener.
Apply this diff:
+if (rootBone) {
rootBone._updateFlagManager.addListener(this._onTransformChanged);
+}📝 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.
| rootBone._updateFlagManager.addListener(this._onTransformChanged); | |
| if (rootBone) { | |
| rootBone._updateFlagManager.addListener(this._onTransformChanged); | |
| } |
| const rootBone = this.skin?.rootBone; | ||
| const worldMatrix = rootBone ? rootBone.transform.worldMatrix : this.entity.transform.worldMatrix; |
There was a problem hiding this comment.
Ensure rootBone and its transform are not null before access
Accessing rootBone.transform without null checking can lead to runtime errors if rootBone or its transform is null. Add a null check to prevent potential issues.
Apply this diff to add the null check:
const rootBone = this.skin?.rootBone;
-const worldMatrix = rootBone ? rootBone.transform.worldMatrix : this.entity.transform.worldMatrix;
+const worldMatrix = rootBone && rootBone.transform ? rootBone.transform.worldMatrix : this.entity.transform.worldMatrix;📝 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.
| const rootBone = this.skin?.rootBone; | |
| const worldMatrix = rootBone ? rootBone.transform.worldMatrix : this.entity.transform.worldMatrix; | |
| const rootBone = this.skin?.rootBone; | |
| const worldMatrix = rootBone && rootBone.transform ? rootBone.transform.worldMatrix : this.entity.transform.worldMatrix; |
| @@ -219,7 +220,7 @@ | |||
| protected override _updateBounds(worldBounds: BoundingBox): void { | |||
| const rootBone = this.skin?.rootBone; | |||
| if (rootBone) { | |||
| BoundingBox.transform(this._localBounds, this._transform.worldMatrix, worldBounds); | |||
| BoundingBox.transform(this._localBounds, rootBone.transform.worldMatrix, worldBounds); | |||
There was a problem hiding this comment.
Add null check for rootBone.transform before transforming bounds
Ensure that rootBone and its transform are not null before using them to transform bounds to prevent runtime exceptions.
Apply this diff:
const rootBone = this.skin?.rootBone;
if (rootBone && rootBone.transform) {
BoundingBox.transform(this._localBounds, rootBone.transform.worldMatrix, worldBounds);
} else {
super._updateBounds(worldBounds);
}Committable suggestion skipped: line range outside the PR's diff.
packages/core/src/Entity.ts
Outdated
| const child = new Entity(this.engine, name); | ||
| const inheritedComponents = Entity._inheritedComponents; | ||
| ComponentsDependencies._getInheritedComponents(this, inheritedComponents); | ||
| const child = new Entity(this.engine, name, ...inheritedComponents); |
There was a problem hiding this comment.
Ensure correct usage of inherited components in createChild
Passing ...inheritedComponents directly may cause issues if inheritedComponents is modified elsewhere. Consider creating a new array to prevent side effects.
Apply this diff:
-const child = new Entity(this.engine, name, ...inheritedComponents);
+const child = new Entity(this.engine, name, ...inheritedComponents.slice());📝 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.
| const child = new Entity(this.engine, name, ...inheritedComponents); | |
| const child = new Entity(this.engine, name, ...inheritedComponents.slice()); |
packages/core/src/Entity.ts
Outdated
| this._inverseWorldMatFlag = this.transform.registerWorldChangeFlag(); | ||
| for (let i = 0, n = components.length; i < n; i++) { | ||
| const type = components[i]; | ||
| if (!Transform.prototype.isPrototypeOf(type.prototype) || !this.transform) { |
There was a problem hiding this comment.
Avoid using isPrototypeOf; use instanceof instead
Accessing Object.prototype methods like isPrototypeOf can be unreliable. Use the instanceof operator or === for accurate type checks.
Apply this diff to fix the issue:
-if (!Transform.prototype.isPrototypeOf(type.prototype) || !this.transform) {
+if (!(type.prototype instanceof Transform) || !this.transform) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 232-232: Do not access Object.prototype method 'isPrototypeOf' from target object.
(lint/suspicious/noPrototypeBuiltins)
packages/core/src/Entity.ts
Outdated
| static _inheritedComponents: ComponentConstructor[] = []; | ||
| /** @internal */ | ||
| static _matrix: Matrix = new Matrix(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential naming conflict with _inheritedComponents
Both Entity and ComponentsDependencies declare a static property named _inheritedComponents, which can lead to confusion and bugs due to naming conflicts. Consider renaming one of them for clarity.
Apply this diff to rename Entity's property:
-static _inheritedComponents: ComponentConstructor[] = [];
+static _entityInheritedComponents: ComponentConstructor[] = [];And update all references to Entity._inheritedComponents accordingly.
Committable suggestion skipped: line range outside the PR's diff.
packages/core/src/Transform.ts
Outdated
| _generateTransitionState(state: TransformTransitionState): void { | ||
| state.position ? state.position.copyFrom(this._position) : (state.position = this._position.clone()); | ||
| state.rotation ? state.rotation.copyFrom(this._rotation) : (state.rotation = this._rotation.clone()); | ||
| state.scale ? state.scale.copyFrom(this._scale) : (state.scale = this._scale.clone()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor assignments within expressions to improve code readability
Assignments within expressions can be confusing and reduce code clarity. Consider refactoring the ternary operations into standard if-else statements for better readability and maintainability.
Apply this diff to refactor the code:
_generateTransitionState(state: TransformTransitionState): void {
- state.position ? state.position.copyFrom(this._position) : (state.position = this._position.clone());
- state.rotation ? state.rotation.copyFrom(this._rotation) : (state.rotation = this._rotation.clone());
- state.scale ? state.scale.copyFrom(this._scale) : (state.scale = this._scale.clone());
+ if (state.position) {
+ state.position.copyFrom(this._position);
+ } else {
+ state.position = this._position.clone();
+ }
+ if (state.rotation) {
+ state.rotation.copyFrom(this._rotation);
+ } else {
+ state.rotation = this._rotation.clone();
+ }
+ if (state.scale) {
+ state.scale.copyFrom(this._scale);
+ } else {
+ state.scale = this._scale.clone();
+ }
}📝 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.
| _generateTransitionState(state: TransformTransitionState): void { | |
| state.position ? state.position.copyFrom(this._position) : (state.position = this._position.clone()); | |
| state.rotation ? state.rotation.copyFrom(this._rotation) : (state.rotation = this._rotation.clone()); | |
| state.scale ? state.scale.copyFrom(this._scale) : (state.scale = this._scale.clone()); | |
| } | |
| _generateTransitionState(state: TransformTransitionState): void { | |
| if (state.position) { | |
| state.position.copyFrom(this._position); | |
| } else { | |
| state.position = this._position.clone(); | |
| } | |
| if (state.rotation) { | |
| state.rotation.copyFrom(this._rotation); | |
| } else { | |
| state.rotation = this._rotation.clone(); | |
| } | |
| if (state.scale) { | |
| state.scale.copyFrom(this._scale); | |
| } else { | |
| state.scale = this._scale.clone(); | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 584-585: 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] 585-586: 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] 586-587: 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)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/core/src/ComponentsDependencies.ts(1 hunks)packages/core/src/Entity.ts(8 hunks)packages/core/src/Transform.ts(6 hunks)packages/core/src/physics/joint/Joint.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/physics/joint/Joint.ts
- packages/core/src/ComponentsDependencies.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/Transform.ts
[error] 584-585: 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] 585-586: 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] 586-587: 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)
packages/core/src/Entity.ts
[error] 227-227: Do not access Object.prototype method 'isPrototypeOf' from target object.
(lint/suspicious/noPrototypeBuiltins)
[error] 245-245: Do not access Object.prototype method 'isPrototypeOf' from target object.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (6)
packages/core/src/Entity.ts (3)
99-104: LGTM: Properties are well-organized and properly decorated
The new properties are correctly defined with appropriate visibility modifiers and the @ignoreClone decorator is correctly applied to _updateFlagManager.
430-433: LGTM: Transform inheritance is handled correctly
The createChild method correctly handles transform inheritance by passing the transform constructor type.
222-232:
Improve transform handling in constructor
The transform handling logic has potential issues:
- Using
isPrototypeOfis not recommended for type checking - The transform check could be more explicit
Apply this diff to improve the code:
constructor(engine: Engine, name?: string, ...components: ComponentConstructor[]) {
super(engine);
this.name = name;
for (let i = 0, n = components.length; i < n; i++) {
const type = components[i];
- if (!Transform.prototype.isPrototypeOf(type.prototype) || !this.transform) {
+ if (!(type.prototype instanceof Transform) || !this.transform) {
this.addComponent(type);
}
}
this.transform || this.addComponent(Transform);
this._inverseWorldMatFlag = this.registerWorldChangeFlag();
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 227-227: Do not access Object.prototype method 'isPrototypeOf' from target object.
(lint/suspicious/noPrototypeBuiltins)
packages/core/src/Transform.ts (3)
343-344: LGTM: Entity transform assignment is correct
The constructor correctly establishes the bidirectional relationship between Entity and Transform.
935-944: LGTM: Well-designed transition state interface
The TransformTransitionState interface is well-designed and properly documented. The remarks about excluding the world matrix provide valuable context for future maintenance.
580-596:
Improve transition state implementation
The transition state methods have several issues:
- Assignments in expressions reduce readability
- No validation of input state
- No error handling for clone operations
This issue was previously identified. Please refer to the existing review comment for the recommended fix.
🧰 Tools
🪛 Biome (1.9.4)
[error] 584-585: 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] 585-586: 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] 586-587: 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)
packages/core/src/Entity.ts
Outdated
| /** | ||
| * The transform of the entity. | ||
| */ | ||
| get transform(): Transform { | ||
| return this._transform; | ||
| } | ||
|
|
||
| set transform(value: Transform) { | ||
| const pre = this._transform; | ||
| if (value !== pre) { | ||
| let state = this._transformTransitionState; | ||
| if (pre) { | ||
| state ||= this._transformTransitionState = {}; | ||
| pre._generateTransitionState(state); | ||
| } | ||
| if (value) { | ||
| state && value._applyTransitionState(state); | ||
| } | ||
| this._transform = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to transform setter
The transform state transition logic needs validation and error handling.
Apply this diff to improve robustness:
set transform(value: Transform) {
const pre = this._transform;
if (value !== pre) {
+ if (value && !(value instanceof Transform)) {
+ throw new Error("Value must be an instance of Transform");
+ }
let state = this._transformTransitionState;
if (pre) {
state ||= this._transformTransitionState = {};
+ try {
pre._generateTransitionState(state);
+ } catch (e) {
+ console.error("Failed to generate transition state:", e);
+ }
}
if (value) {
+ try {
state && value._applyTransitionState(state);
+ } catch (e) {
+ console.error("Failed to apply transition state:", e);
+ }
}
this._transform = value;
}
}📝 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.
| /** | |
| * The transform of the entity. | |
| */ | |
| get transform(): Transform { | |
| return this._transform; | |
| } | |
| set transform(value: Transform) { | |
| const pre = this._transform; | |
| if (value !== pre) { | |
| let state = this._transformTransitionState; | |
| if (pre) { | |
| state ||= this._transformTransitionState = {}; | |
| pre._generateTransitionState(state); | |
| } | |
| if (value) { | |
| state && value._applyTransitionState(state); | |
| } | |
| this._transform = value; | |
| } | |
| } | |
| /** | |
| * The transform of the entity. | |
| */ | |
| get transform(): Transform { | |
| return this._transform; | |
| } | |
| set transform(value: Transform) { | |
| const pre = this._transform; | |
| if (value !== pre) { | |
| if (value && !(value instanceof Transform)) { | |
| throw new Error("Value must be an instance of Transform"); | |
| } | |
| let state = this._transformTransitionState; | |
| if (pre) { | |
| state ||= this._transformTransitionState = {}; | |
| try { | |
| pre._generateTransitionState(state); | |
| } catch (e) { | |
| console.error("Failed to generate transition state:", e); | |
| } | |
| } | |
| if (value) { | |
| try { | |
| state && value._applyTransitionState(state); | |
| } catch (e) { | |
| console.error("Failed to apply transition state:", e); | |
| } | |
| } | |
| this._transform = value; | |
| } | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2456 +/- ##
===========================================
+ Coverage 67.86% 68.34% +0.48%
===========================================
Files 907 908 +1
Lines 94311 94377 +66
Branches 7890 7930 +40
===========================================
+ Hits 64001 64503 +502
+ Misses 30060 29624 -436
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/core/src/Entity.ts (1)
107-127: ValidateTransformtransitions in thetransformsetterThe
transformsetter manages transition states between the previous and newTransform. Ensure that this logic correctly handles cases wherestatemight be undefined and that errors are properly caught.Consider adding error handling to manage potential exceptions during
_generateTransitionStateand_applyTransitionStatecalls.packages/core/src/Transform.ts (1)
Line range hint
888-945: Enhance deprecation notice with migration guideWhile the deprecation is well-marked and the new interface is well-documented, consider adding:
- Migration path from registerWorldChangeFlag to the new approach
- Example usage of TransformTransitionState interface
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/core/src/Camera.ts(5 hunks)packages/core/src/Entity.ts(8 hunks)packages/core/src/Renderer.ts(6 hunks)packages/core/src/Transform.ts(6 hunks)packages/core/src/mesh/SkinnedMeshRenderer.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/Transform.ts
[error] 584-585: 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] 585-586: 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] 586-587: 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 (21)
packages/core/src/Renderer.ts (5)
72-72: Introduction of _transformEntity property
The addition of the _transformEntity property replaces the previous _transform property, shifting from directly referencing the Transform component to referencing the Entity. This change aligns with the refactoring to manage transformations via the Entity, improving consistency and maintainability.
369-369: Confirm listener removal when _transformEntity is null
Setting _transformEntity to null should correctly remove any listeners attached to the previous entity to prevent memory leaks or unexpected behavior.
395-395: Accessing this._entity.transform without null checks
The code assumes that this._entity.transform is always available. Since Renderer depends on Transform (as indicated by @dependentComponents(Transform, DependentMode.CheckOnly)), this is acceptable. Ensure that all Entity instances used with Renderer have a Transform component to prevent runtime errors.
Also applies to: 445-445
468-474: Properly updating listeners in _setTransformEntity
The _setTransformEntity method correctly manages listeners by removing them from the previous entity and adding them to the new one. This ensures that the Renderer reacts appropriately to transform changes.
175-175: Ensure _setTransformEntity handles null entities appropriately
When calling _setTransformEntity(entity), passing null as the entity could lead to potential issues if not handled correctly. Ensure that methods relying on _transformEntity properly handle cases where it may be null.
Run the following script to check for usages of _transformEntity where null handling may be required:
packages/core/src/mesh/SkinnedMeshRenderer.ts (2)
123-124: Ensure rootBone and its transform are not null before access
Accessing rootBone.transform without verifying that both rootBone and its transform are not null can lead to runtime errors. Add a null check to prevent potential issues.
223-223: Add null check for rootBone.transform before transforming bounds
Ensure that rootBone and its transform are not null before using them to transform bounds to prevent runtime exceptions.
packages/core/src/Entity.ts (6)
99-99: Initialize _updateFlagManager in the constructor
The addition of _updateFlagManager ensures that update flags are managed for the Entity. Confirm that it is properly initialized and utilized throughout the class.
220-230: Ensure proper addition of components in the constructor
When creating a new Entity, the constructor now accepts components to add. The logic checks if the Transform component is already present before adding it. Verify that this logic works as intended and does not unintentionally skip adding necessary components.
428-431: Ensure correct inheritance of Transform in createChild
When creating a child entity, the logic checks for a Transform to inherit. Ensure that the child entity correctly inherits or creates a Transform component.
466-472: Implement registerWorldChangeFlag method properly
The new registerWorldChangeFlag method allows for listening to world pose changes. Verify that this method correctly registers and returns a BoolUpdateFlag.
523-523: Use ComponentConstructor type for component addition
In the _parseCloneEntity method, casting to <ComponentConstructor> ensures type safety when adding components during cloning.
810-811: Define ComponentConstructor type
The addition of the ComponentConstructor type improves code clarity and type safety when working with components.
packages/core/src/Camera.ts (4)
424-426: Update Camera constructor to use entity's Transform
The constructor now registers world change flags using the entity's Transform. This removes redundancy and ensures that the camera's transformation data is sourced from the associated entity.
606-606: Ensure virtualCamera properties are updated correctly
In the render method, the virtualCamera's viewProjectionMatrix and position are updated using the entity's Transform. Confirm that this correctly reflects the camera's state.
786-789: Optimize shader data updates with cached properties
The _updateShaderData method now uses the entity's Transform to set shader properties. This streamlines the process and ensures consistency.
804-804: Correct calculation of inverse view-projection matrix
The _getInvViewProjMat method multiplies the world matrix with the inverse projection matrix to obtain the inverse view-projection matrix. Ensure that this calculation is accurate and that the matrices are up-to-date.
packages/core/src/Transform.ts (4)
343-344: LGTM: Early establishment of transform-entity relationship
The direct assignment in the constructor ensures immediate setup of the bidirectional reference between Entity and Transform.
580-596: Previous review comment is still applicable
The transition state implementation still contains assignments in expressions and lacks input validation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 584-585: 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] 585-586: 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] 586-587: 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)
614-615: LGTM: Proper cleanup of entity reference
Properly nulling the transform reference during destruction prevents memory leaks and stale references.
774-774: LGTM: Centralized update flag management
Using the entity's _updateFlagManager for dispatching updates aligns with the optimization goals.
| @@ -265,7 +266,7 @@ export class SkinnedMeshRenderer extends MeshRenderer { | |||
| } | |||
| break; | |||
| case SkinUpdateFlag.RootBoneChanged: | |||
| this._setTransform((<Entity>value).transform); | |||
| this._setTransformEntity(<Entity>value); | |||
There was a problem hiding this comment.
Handle listener registration when rootBone changes
In the _onSkinUpdated and _applySkin methods, listener removal and addition for rootBone._updateFlagManager should account for cases where rootBone may be null, to prevent runtime errors.
Apply this diff to add null checks:
// In _onSkinUpdated method
case SkinUpdateFlag.RootBoneChanged:
+ if (this._transformEntity) {
this._setTransformEntity(<Entity>value);
+ }
this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume;
break;
// In _applySkin method
if (lastRootBone !== rootBone) {
+ lastRootBone?._updateFlagManager.removeListener(this._onTransformChanged);
+ rootBone?._updateFlagManager.addListener(this._onTransformChanged);
this._onSkinUpdated(SkinUpdateFlag.RootBoneChanged, rootBone);
}Also applies to: 288-289
packages/core/src/Entity.ts
Outdated
| @@ -210,6 +240,7 @@ | |||
| type: T, | |||
| ...args: ComponentArguments<T> | |||
| ): InstanceType<T> { | |||
| type.prototype instanceof Transform && this.transform?.destroy(); | |||
There was a problem hiding this comment.
Avoid destroying existing Transform instances when adding components
In the addComponent method, the line type.prototype instanceof Transform && this.transform?.destroy(); may inadvertently destroy a valid Transform. This could lead to unintended side effects.
Consider removing or modifying this line to prevent destroying existing Transform components.
Apply this diff:
-type.prototype instanceof Transform && this.transform?.destroy();
+if (type.prototype instanceof Transform && !this.transform) {
+ // If the entity does not already have a Transform, proceed to add one.
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/core/src/mesh/MeshRenderer.ts (1)
Line range hint
186-190: Refactor conditional expressions for improved readabilityConsider replacing the logical AND
&&used for conditional execution in the_onMeshChangedmethod with explicitifstatements. This enhances code clarity and maintainability.Apply this diff to enhance readability:
- type & MeshModifyFlags.Bounds && (this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume); - type & MeshModifyFlags.VertexElements && (this._dirtyUpdateFlag |= MeshRendererUpdateFlags.VertexElementMacro); + if (type & MeshModifyFlags.Bounds) { + this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume; + } + if (type & MeshModifyFlags.VertexElements) { + this._dirtyUpdateFlag |= MeshRendererUpdateFlags.VertexElementMacro; + }packages/core/src/Entity.ts (2)
107-127: Improve transform state transition handlingThe transform getter/setter implementation is good, but the state object creation can be improved.
Consider this cleaner approach:
set transform(value: Transform) { const pre = this._transform; if (value !== pre) { let state = this._transformTransitionState; if (pre) { - state ||= this._transformTransitionState = {}; + if (!state) { + state = {}; + this._transformTransitionState = state; + } pre._generateTransitionState(state); } if (value) { state && value._applyTransitionState(state); } this._transform = value; } }
497-499: Maintain consistency in transform state handlingSimilar to the transform setter, the state initialization can be improved.
Consider this more consistent approach:
- const transitionState = (this._transformTransitionState ||= {}); + if (!this._transformTransitionState) { + this._transformTransitionState = {}; + } + const transitionState = this._transformTransitionState; srcTransform._generateTransitionState(transitionState); cloneTransform._applyTransitionState(transitionState);🧰 Tools
🪛 Biome (1.9.4)
[error] 497-497: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/core/src/Entity.ts(10 hunks)packages/core/src/Renderer.ts(6 hunks)packages/core/src/mesh/MeshRenderer.ts(1 hunks)packages/core/src/mesh/SkinnedMeshRenderer.ts(2 hunks)packages/core/src/particle/ParticleGenerator.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/Entity.ts
[error] 497-497: 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 (13)
packages/core/src/mesh/MeshRenderer.ts (1)
105-105: Update to use _transformEntity is appropriate
This change ensures that the transformation uses the correct entity's world matrix, aligning with the updated component management strategy.
packages/core/src/mesh/SkinnedMeshRenderer.ts (2)
208-208: LGTM: Simplified bounds transformation logic
The change to use entity's transform directly for bounds calculation is cleaner and more consistent with the new transform management approach.
251-251: Verify null safety when setting transform entity
While the change to use _setTransformEntity is correct, ensure null safety when the root bone changes.
✅ Verification successful
Let me check the implementation of _setTransformEntity to ensure null safety.
Let me try a different pattern to find the method implementation.
Based on the code analysis, I can now generate the final response:
Null safety is properly handled for root bone transformations
The code has appropriate null safety measures:
- Root bone has a fallback to the entity:
rootBone = value?.rootBone ?? this.entity - The value passed to
_setTransformEntityis explicitly type-cast toEntity - Changes to root bone are properly tracked with null-safe optional chaining
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential null checks around rootBone usage
rg -A 3 "rootBone.*=.*null"
rg -A 3 "(!|\\?).*rootBone"
Length of output: 1323
Script:
#!/bin/bash
# Check the implementation of _setTransformEntity
ast-grep --pattern 'private _setTransformEntity($_) {
$$$
}'
Length of output: 64
Script:
#!/bin/bash
# Search for _setTransformEntity implementation with rg
rg -A 10 "_setTransformEntity" packages/core/src/mesh/SkinnedMeshRenderer.ts
Length of output: 463
packages/core/src/Renderer.ts (3)
62-64: LGTM: Added entity-based transform reference
The new _transformEntity property properly implements the shift to entity-based transform management.
469-475: LGTM: Well-implemented transform entity management
The _setTransformEntity method properly handles:
- Listener cleanup for previous entity
- Listener setup for new entity
- Null safety checks
396-396: Verify shader data consistency with the new transform access
The change to use _transformEntity.transform is correct, but we should verify shader data consistency.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about shader data consistency. The results show all instances where transform data is used with shader data across the codebase, including the Renderer, Camera, and particle system's MainModule.
Transform-shader data consistency is maintained across the codebase
The change to use _transformEntity.transform.worldMatrix is consistent with other transform-shader data interactions in the codebase:
- In Renderer: Local and world matrices are correctly set using the transform entity
- In Camera: World matrix and transform properties are properly accessed
- In MainModule (particle system): World position, rotation, and scale are consistently accessed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other shader data updates using transform
rg -A 3 "shaderData.*transform"
ast-grep --pattern 'shaderData.setMatrix($_, $transformExpr)'
Length of output: 2101
packages/core/src/Entity.ts (6)
98-99: LGTM: Added update flag management
The _updateFlagManager addition provides proper infrastructure for managing entity updates.
101-102: LGTM: Added transform state management
The private properties properly encapsulate transform state and transition management.
223-229: LGTM: Robust component initialization
The component initialization logic properly:
- Handles Transform components specially
- Ensures Transform component existence
- Uses safe type checking
428-431: LGTM: Proper transform inheritance in child creation
The child entity creation properly inherits the parent's transform type, maintaining transform consistency in the entity hierarchy.
466-472: LGTM: Clean world change notification API
The registerWorldChangeFlag method provides a clean interface for tracking world transform changes.
483-486: LGTM: Consistent transform handling in entity cloning
The clone creation logic maintains consistency with the new transform management approach.
packages/core/src/particle/ParticleGenerator.ts (1)
249-249: Consider the implications of accessing private property _entity.
The change from entity to _entity suggests a shift towards using private internal state. While this might improve performance by avoiding potential getter overhead, it creates a tighter coupling between ParticleGenerator and ParticleRenderer's internal implementation.
Let's verify the consistency of this pattern across the codebase:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/src/RenderPipeline/RenderQueue.ts (1)
189-189: Consider documenting the architectural changeThe shift from
entity.transformto_transformEntity.transformappears to be part of a larger architectural change in transform handling. This change might benefit from documentation explaining the new pattern and its benefits.Consider:
- Adding a comment explaining why
_transformEntityis preferred overentity- Documenting any performance implications
- Updating relevant architecture documentation to reflect this pattern
tests/src/core/Transform.test.ts (1)
135-135: Use optional chaining to simplify property accessUsing optional chaining improves code readability and safely accesses properties that might be undefined.
Apply this diff to simplify the expression:
- this.size && this.size.copyFrom(state.size); + this.size?.copyFrom(state.size);🧰 Tools
🪛 Biome (1.9.4)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/RenderPipeline/RenderQueue.ts(1 hunks)tests/src/core/Transform.test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tests/src/core/Transform.test.ts
[error] 122-122: 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] 123-123: 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] 124-124: 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] 125-125: 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] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
packages/core/src/RenderPipeline/RenderQueue.ts (1)
189-189: Verify the consistency of transform access pattern
The change from entity.transform to _transformEntity.transform aligns with the broader refactoring pattern, but we should verify this is consistently applied across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
The change to _transformEntity.transform is consistent with the codebase
The search results confirm that _transformEntity.transform is only used in this specific location for front face inversion, while all other transform-related code in the codebase uses the standard entity.transform pattern. This is acceptable because:
- The
_transformEntityusage is localized to the render pipeline's internal implementation - No other code depends on this specific front face inversion check
- The change aligns with the broader refactoring pattern mentioned in the summary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old pattern and verify the new pattern is consistently used
# Check for old pattern usage
echo "Checking for old pattern usage..."
rg "entity\.transform\._isFrontFaceInvert" --type ts
# Check for new pattern usage
echo "Checking for new pattern usage..."
rg "_transformEntity\.transform\._isFrontFaceInvert" --type ts
# Look for any mixed usage patterns
echo "Checking for any transform-related entity references..."
rg "entity\.transform" --type ts -A 2 -B 2
Length of output: 42832
tests/src/core/Transform.test.ts (2)
63-112: Test cases for subclassing Transform are well-structured
The new test cases effectively validate the behavior of Transform and its subclass SubClassOfTransform, including instantiation, property inheritance, cloning, and component management.
115-138: Implementation of SubClassOfTransform is appropriate
The subclass correctly extends Transform and overrides the _generateTransitionState and _applyTransitionState methods to handle the additional size property.
🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: 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] 123-123: 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] 124-124: 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] 125-125: 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] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/src/core/Transform.test.ts
Outdated
| state.position ? state.position.copyFrom(this.position) : (state.position = this.position.clone()); | ||
| state.rotation ? state.rotation.copyFrom(this.rotation) : (state.rotation = this.rotation.clone()); | ||
| state.scale ? state.scale.copyFrom(this.scale) : (state.scale = this.scale.clone()); | ||
| state.size ? state.size.copyFrom(this.size) : (state.size = this.size.clone()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to avoid assignments within ternary expressions for clarity
Assigning values within ternary expressions can reduce code readability and may lead to confusion. It's better to refactor these statements to improve clarity.
Apply this diff to refactor the code:
- state.position ? state.position.copyFrom(this.position) : (state.position = this.position.clone());
- state.rotation ? state.rotation.copyFrom(this.rotation) : (state.rotation = this.rotation.clone());
- state.scale ? state.scale.copyFrom(this.scale) : (state.scale = this.scale.clone());
- state.size ? state.size.copyFrom(this.size) : (state.size = this.size.clone());
+ if (state.position) {
+ state.position.copyFrom(this.position);
+ } else {
+ state.position = this.position.clone();
+ }
+ if (state.rotation) {
+ state.rotation.copyFrom(this.rotation);
+ } else {
+ state.rotation = this.rotation.clone();
+ }
+ if (state.scale) {
+ state.scale.copyFrom(this.scale);
+ } else {
+ state.scale = this.scale.clone();
+ }
+ if (state.size) {
+ state.size.copyFrom(this.size);
+ } else {
+ state.size = this.size.clone();
+ }📝 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.
| state.position ? state.position.copyFrom(this.position) : (state.position = this.position.clone()); | |
| state.rotation ? state.rotation.copyFrom(this.rotation) : (state.rotation = this.rotation.clone()); | |
| state.scale ? state.scale.copyFrom(this.scale) : (state.scale = this.scale.clone()); | |
| state.size ? state.size.copyFrom(this.size) : (state.size = this.size.clone()); | |
| if (state.position) { | |
| state.position.copyFrom(this.position); | |
| } else { | |
| state.position = this.position.clone(); | |
| } | |
| if (state.rotation) { | |
| state.rotation.copyFrom(this.rotation); | |
| } else { | |
| state.rotation = this.rotation.clone(); | |
| } | |
| if (state.scale) { | |
| state.scale.copyFrom(this.scale); | |
| } else { | |
| state.scale = this.scale.clone(); | |
| } | |
| if (state.size) { | |
| state.size.copyFrom(this.size); | |
| } else { | |
| state.size = this.size.clone(); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: 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] 123-123: 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] 124-124: 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] 125-125: 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)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/src/core/Transform.test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tests/src/core/Transform.test.ts
[error] 122-122: 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] 123-123: 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] 124-124: 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] 125-125: 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] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
tests/src/core/Transform.test.ts (4)
1-18: LGTM: Test setup and imports are well structured
The test setup follows best practices with proper initialization of WebGLEngine, Scene, and Entity in the beforeAll hook.
64-112: LGTM: Comprehensive test coverage for Transform subclassing
The test case thoroughly covers all important scenarios:
- Constructor-based creation
- Child entity creation
- Entity cloning
- Component addition/replacement
- Property preservation
Each scenario includes proper assertions for both instance type checking and property verification.
122-125: Refactor to avoid assignments within ternary expressions
The assignments within ternary expressions reduce code readability and maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: 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] 123-123: 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] 124-124: 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] 125-125: 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)
64-112: Test coverage aligns well with PR objectives
The test suite effectively validates the optimization of Transform's addition and replacement logic through comprehensive scenarios, supporting the PR's main objectives.
tests/src/core/Transform.test.ts
Outdated
| this.position.copyFrom(state.position); | ||
| this.rotation.copyFrom(state.rotation); | ||
| this.scale.copyFrom(state.scale); | ||
| this.size && this.size.copyFrom(state.size); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use optional chaining for null-safe property access
Replace the unsafe null check with optional chaining for better type safety.
- this.size && this.size.copyFrom(state.size);
+ this.size?.copyFrom(state.size);📝 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.size && this.size.copyFrom(state.size); | |
| this.size?.copyFrom(state.size); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/src/core/Transform.test.ts (1)
131-136: Add state validation to prevent runtime errorsThe method should validate the state object and its required properties before applying them.
Consider adding validation:
_applyTransitionState(state: SubClassOfTransformTransitionState): void { + if (!state || !state.position || !state.rotation || !state.scale) { + throw new Error("Invalid transition state: missing required properties"); + } this._position.copyFrom(state.position); this._rotation.copyFrom(state.rotation); this._scale.copyFrom(state.scale); state.size && this.size.copyFrom(state.size); }packages/core/src/Entity.ts (2)
117-130: Add error handling to transform setterThe transform state transition logic should handle potential errors during state generation and application.
Add error handling:
set transform(value: Transform) { const pre = this._transform; if (value !== pre) { + if (value && !(value instanceof Transform)) { + throw new Error("Value must be an instance of Transform"); + } let state = this._transformTransitionState; if (pre) { state ||= this._transformTransitionState = {}; + try { pre._generateTransitionState(state); + } catch (e) { + console.error("Failed to generate transition state:", e); + } } if (value) { + try { state && value._applyTransitionState(state); + } catch (e) { + console.error("Failed to apply transition state:", e); + } } this._transform = value; } }
437-440: Improve type safety in transform constructor castingThe type casting of transform constructor could be made more type-safe.
Consider using type guards:
- ? new Entity(this.engine, name, transform.constructor as ComponentConstructor) + ? new Entity( + this.engine, + name, + isTransformConstructor(transform.constructor) ? transform.constructor : Transform + ) // Add type guard function: function isTransformConstructor( ctor: any ): ctor is new (entity: Entity) => Transform { return ctor?.prototype instanceof Transform; }packages/core/src/Transform.ts (1)
884-892: Enhance deprecation notice with migration guideThe deprecation notice should guide users to the new API.
Add migration information to the deprecation notice:
/** * @deprecated + * Use entity.registerWorldChangeFlag() instead. + * This method will be removed in the next major version. * Listen for changes in the world pose of this `Entity`. * @returns Change flag */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/core/src/Entity.ts(10 hunks)packages/core/src/Transform.ts(4 hunks)tests/src/core/Transform.test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tests/src/core/Transform.test.ts
[error] 122-122: 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] 123-123: 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] 124-124: 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] 125-125: 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)
packages/core/src/Entity.ts
[error] 506-506: 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)
packages/core/src/Transform.ts
[error] 582-583: 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] 583-584: 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] 584-585: 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 (8)
tests/src/core/Transform.test.ts (3)
64-112: Well-structured test coverage!
The test case thoroughly verifies the Transform inheritance behavior, covering entity creation, child entities, cloning, and component replacement scenarios. This ensures the robustness of the new transform management system.
139-144: Interface is correctly used in transition state management
This interface defines the shape of the transition state object used in _generateTransitionState and _applyTransitionState methods. The optional properties allow for partial state updates.
122-125: 🛠️ Refactor suggestion
Refactor assignments in expressions for better readability
The assignments within ternary expressions can be confusing and make the code harder to maintain.
Apply this diff to improve readability:
- state.position ? state.position.copyFrom(this._position) : (state.position = this._position.clone());
- state.rotation ? state.rotation.copyFrom(this._rotation) : (state.rotation = this._rotation.clone());
- state.scale ? state.scale.copyFrom(this._scale) : (state.scale = this._scale.clone());
- state.size ? state.size.copyFrom(this.size) : (state.size = this.size.clone());
+ if (state.position) {
+ state.position.copyFrom(this._position);
+ } else {
+ state.position = this._position.clone();
+ }
+ if (state.rotation) {
+ state.rotation.copyFrom(this._rotation);
+ } else {
+ state.rotation = this._rotation.clone();
+ }
+ if (state.scale) {
+ state.scale.copyFrom(this._scale);
+ } else {
+ state.scale = this._scale.clone();
+ }
+ if (state.size) {
+ state.size.copyFrom(this.size);
+ } else {
+ state.size = this.size.clone();
+ }🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: 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] 123-123: 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] 124-124: 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] 125-125: 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)
packages/core/src/Entity.ts (2)
475-481: LGTM! Clean implementation of world change flag registration
The method provides a clear way to register and track world transform changes using the flag manager.
230-234: Verify transform component initialization logic
The condition !(type.prototype instanceof Transform) || !this.transform might allow multiple Transform instances if not handled carefully.
Run this script to check for multiple Transform instances:
✅ Verification successful
The previous pattern didn't yield results, so let's examine the Entity class implementation and Transform usage patterns to better understand the component initialization logic and potential issues with multiple Transform instances.
After analyzing the codebase, I can now generate the final response regarding the transform component initialization logic:
Transform component initialization is correctly implemented
The implementation in Entity.ts ensures proper Transform component initialization through multiple safeguards:
- The condition
!(type.prototype instanceof Transform) || !this.transformprevents adding duplicate Transform components since it only adds non-Transform components or adds Transform when one doesn't exist - The subsequent line
this.transform || this.addComponent(Transform)ensures a Transform component is always added if none exists - The Transform class itself maintains proper parent-child relationships and transform hierarchies through its internal state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential multiple Transform instance creation
ast-grep --pattern 'new Entity($_, $_, Transform)'
Length of output: 52
Script:
#!/bin/bash
# Let's search for Entity constructor implementation and Transform component usage
ast-grep --pattern 'class Entity {
$$$
constructor($$$) {
$$$
}
$$$
}'
# Also search for Transform component usage patterns
rg "Transform" -A 3 -B 3
Length of output: 68517
packages/core/src/Transform.ts (3)
581-585: Refactor assignments in expressions for better readability
The assignments within ternary expressions should be refactored for better maintainability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 582-583: 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] 583-584: 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] 584-585: 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)
770-770: LGTM! Proper delegation of flag management
The change correctly moves flag management responsibility to the entity's _updateFlagManager.
933-942: Well-documented interface design
The interface clearly defines the transform state structure with helpful remarks explaining why world matrix is excluded.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/Entity.ts (2)
209-217: Consider simplifying the component initialization logicThe current implementation has nested conditions that could be simplified for better readability.
Consider this alternative approach:
-for (let i = 0, n = components.length; i < n; i++) { - const type = components[i]; - if (type.prototype instanceof Transform) { - this._transform || this.addComponent(type); - } else { - this.addComponent(type); - } -} -this._transform || this.addComponent(Transform); +const transformType = components.find(type => type.prototype instanceof Transform); +if (transformType) { + this.addComponent(transformType); +} +components + .filter(type => !(type.prototype instanceof Transform)) + .forEach(type => this.addComponent(type)); +this._transform || this.addComponent(Transform);
777-783: Consider adding validation in _setTransformThe transform setter should validate the input to prevent potential issues.
Consider this enhancement:
private _setTransform(value: Transform) { + if (value && !(value instanceof Transform)) { + throw new Error("Value must be an instance of Transform"); + } const pre = this._transform; if (value !== pre) { pre && value?._copyFrom(pre); this._transform = value; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/Entity.ts(11 hunks)
🔇 Additional comments (5)
packages/core/src/Entity.ts (5)
98-99: LGTM: UpdateFlagManager initialization
The addition of _updateFlagManager centralizes the management of update flags, improving the organization of state tracking.
106-111: LGTM: Transform accessor implementation
The getter provides clean access to the private _transform field, following encapsulation best practices.
420-423: LGTM: Improved child entity creation
The changes ensure that child entities inherit the correct transform type from their parent, maintaining consistency in the entity hierarchy.
458-464: LGTM: World transform change tracking
The new registerWorldChangeFlag method provides a clean API for tracking world transform changes, properly utilizing the UpdateFlagManager.
235-239: Verify transform replacement logic
The transform replacement logic needs to ensure proper cleanup and state transfer.
✅ Verification successful
Let me gather more information about the Transform class and its usage to verify the cleanup and state transfer logic.
Let me check one more aspect to understand the Transform's state copying and initialization.
Transform replacement logic is properly implemented
The code implements a safe transform replacement with proper cleanup and state transfer:
- The old transform is stored and destroyed after the new one is set
- State transfer is handled via
_copyFromin_setTransform - Parent-child relationships are maintained through
_parentChangeand transform dirty flags - Proper cleanup is done through the
destroy()call on the previous transform
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential memory leaks or cleanup issues in transform replacement
rg -A 5 "destroy\(\)" | grep -C 5 "Transform"
Length of output: 524
Script:
#!/bin/bash
# Check Transform class implementation and usage
ast-grep --pattern 'class Transform {
$$$
}'
# Check for Transform-related state management
rg -A 5 "_setTransform"
Length of output: 3969
Script:
#!/bin/bash
# Check Transform's copyFrom implementation and initialization
ast-grep --pattern 'class Transform {
$$$
_copyFrom($_) {
$$$
}
$$$
}'
# Check for Transform's parent change handling
rg "_parentChange" -A 5
Length of output: 780
GuoLei1990
changed the title
TransformTransform
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
Release Notes
New Features
Transformfor improved testing scenarios.Bug Fixes
Improvements
MeshRendererclass.Refactor