XR editor adaptation #2165
XR editor adaptation #2165
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe updates introduce significant enhancements across the codebase, focusing on asset management, XR feature tracking, and improved session management. Key changes include new exports for types and classes, the addition of methods like Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (1)
packages/xr/src/XRManagerExtended.ts (1)
Line range hint
86-91:
Add a return statement for no matching feature.The method should return
nullif no matching feature is found.+ return null;Tools
Biome
[error] 102-102: The update clause in this loop moves the variable in the wrong direction.
(lint/correctness/useValidForDirection)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- packages/core/src/index.ts (1 hunks)
- packages/loader/src/SceneLoader.ts (2 hunks)
- packages/loader/src/index.ts (2 hunks)
- packages/loader/src/resource-deserialize/index.ts (2 hunks)
- packages/loader/src/resource-deserialize/resources/parser/CustomParser.ts (1 hunks)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts (2 hunks)
- packages/loader/src/resource-deserialize/resources/schema/SceneSchema.ts (1 hunks)
- packages/xr-webxr/src/WebXRDevice.ts (1 hunks)
- packages/xr-webxr/src/WebXRSession.ts (1 hunks)
- packages/xr/src/XRManagerExtended.ts (4 hunks)
- packages/xr/src/component/XRTrackedComponent.ts (1 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (5 hunks)
- packages/xr/src/index.ts (2 hunks)
- packages/xr/src/loader/IXRScene.ts (1 hunks)
- packages/xr/src/loader/XRCustomParser.ts (1 hunks)
- packages/xr/src/loader/XRReferenceImageDecoder.ts (1 hunks)
- packages/xr/src/loader/XRReferenceImageLoader.ts (1 hunks)
Additional context used
Biome
packages/xr/src/loader/XRReferenceImageDecoder.ts
[error] 5-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/xr/src/loader/XRCustomParser.ts
[error] 42-42: 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/xr/src/XRManagerExtended.ts
[error] 102-102: The update clause in this loop moves the variable in the wrong direction.
(lint/correctness/useValidForDirection)
Additional comments not posted (46)
packages/loader/src/resource-deserialize/resources/parser/CustomParser.ts (1)
1-8: LGTM!The introduction of the abstract class
CustomParserand its methodonSceneParseis appropriate. The implementation is minimal and serves as a placeholder for subclasses to override.packages/xr/src/component/XRTrackedComponent.ts (1)
1-23: LGTM!The class
XRTrackedComponentis well-structured, with clear encapsulation of properties and appropriate getters and setters. The use of TypeScript generics ensures type safety.packages/xr/src/loader/XRReferenceImageLoader.ts (1)
1-16: LGTM!The class
XRReferenceImageLoaderis well-implemented, following the asynchronous pattern for loading and decodingXRReferenceImageassets.packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts (3)
4-4: Import statement approved.The import of
ISceneis necessary and correct.
14-19: Return type update approved.The
parsemethod's return type change toPromise<ParserContext<IScene, Scene>>is appropriate and aligns with its functionality.
Line range hint
21-24:
Constructor changes approved.The constructor correctly initializes
data,context, andscene.packages/xr/src/loader/IXRScene.ts (5)
1-5: Import statements approved.The import statements for necessary types are correct and required.
6-15:IXRSceneConfiginterface approved.The interface extensions with XR-specific properties are logical and necessary for XR scene configuration.
17-19:IXRSceneinterface approved.The interface extensions to include
IXRSceneConfigare logical and necessary for XR scene handling.
21-43: Feature schema interfaces approved.The new interfaces for XR feature schemas are well-defined and necessary for XR feature handling.
45-48:IAnchorinterface approved.The
IAnchorinterface is well-defined and necessary for anchor handling in XR.packages/loader/src/resource-deserialize/index.ts (3)
3-10: Newly added exports approved.The new exports for
decoderMap, various decoders, parsers, and utilities are logical and necessary for the module's functionality.
27-36: Newly added exports approved.The new exports for
CustomParser,ParserContext, various loaders, schemas, and utilities are logical and necessary for the module's functionality.
Line range hint
1-36:
Overall file structure approved.The file structure is logical and well-organized, appropriate for the module's functionality.
packages/loader/src/index.ts (3)
22-22: New import approved.The import of
CustomParseris necessary and correctly included.
26-26: New export approved.The export of
PrefabLoaderis necessary and correctly included.
36-47: New functionregisterCustomParserapproved.The function for registering custom parsers is logical and necessary for the module's functionality.
packages/loader/src/resource-deserialize/resources/schema/SceneSchema.ts (2)
18-18: Addition ofsceneproperty inISceneinterface approved.The addition of the
sceneproperty in theISceneinterface is well-defined and fits within the existing structure.
22-57: Addition ofISceneConfiginterface approved.The
ISceneConfiginterface is well-defined and consistent with the overall schema design.packages/xr/src/index.ts (3)
6-6: Addition ofXRTrackedComponentexport approved.The addition of the
XRTrackedComponentexport is consistent with the overall export structure and follows best practices.
44-44: Addition ofXRCustomParserexport approved.The addition of the
XRCustomParserexport is consistent with the overall export structure and follows best practices.
45-46: Addition ofXRReferenceImageDecoderandXRReferenceImageLoaderexports approved.The addition of the
XRReferenceImageDecoderandXRReferenceImageLoaderexports are consistent with the overall export structure and follow best practices.packages/core/src/index.ts (6)
1-1: Addition ofCanvasexport approved.The addition of the
Canvasexport is consistent with the overall export structure and follows best practices.
9-10: Addition ofBoolUpdateFlag,Camera, andScriptexports approved.The addition of the
BoolUpdateFlag,Camera, andScriptexports are consistent with the overall export structure and follow best practices.Also applies to: 17-17
12-12: Addition ofDependentModeanddependentComponentsexports approved.The addition of the
DependentModeanddependentComponentsexports are consistent with the overall export structure and follow best practices.
13-14: Addition ofEngineConfigurationandEngineSettingstype exports approved.The addition of the
EngineConfigurationandEngineSettingstype exports are consistent with the overall export structure and follow best practices.
33-35: Addition ofBackgroundandLayerexports approved.The addition of the
BackgroundandLayerexports are consistent with the overall export structure and follow best practices.
39-40: Addition ofBackgroundModeandBackgroundTextureFillModeexports approved.The addition of the
BackgroundModeandBackgroundTextureFillModeexports are consistent with the overall export structure and follow best practices.packages/xr-webxr/src/WebXRDevice.ts (1)
28-37: Changes inisSupportedFeaturemethod approved.The updated switch statement correctly handles all
XRFeatureTypecases and follows best practices.packages/xr/src/loader/XRCustomParser.ts (5)
1-29: Imports look good.The imports include necessary modules for the functionality of the
XRCustomParserclass.
70-84: MethodaddImageTrackinglooks good.The method correctly handles the addition of image tracking features.
86-92: MethodaddPlaneTrackinglooks good.The method correctly handles the addition of plane tracking features.
94-107: MethodaddAnchorTrackinglooks good.The method correctly handles the addition of anchor tracking features.
116-119: MethodsetCameralooks good.The method correctly handles the attachment of cameras to the camera manager.
packages/loader/src/SceneLoader.ts (2)
Line range hint
1-22:
Imports look good.The imports include necessary modules for the functionality of the
SceneLoaderclass.
31-32: Verify the return type ofSceneParser.parse.The return type of
SceneParser.parsehas been updated to includeParserContext. Ensure this change is consistent throughout the codebase.Verification successful
Verify the return type of
SceneParser.parse.The previous search only showed the usage of
SceneParser.parse. To confirm the return type includesParserContext, we need to locate and inspect the function definition.
Verification successful.
The return type of
SceneParser.parsehas been correctly updated to includeParserContextasPromise<ParserContext<IScene, Scene>>.
packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts: static parse(engine: Engine, sceneData: IScene): Promise<ParserContext<IScene, Scene>>Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the return type of `SceneParser.parse` has been updated to include `ParserContext`. # Test: Search for the function definition. Expect: The return type includes `ParserContext`. rg --type ts -A 5 $'SceneParser.parse'Length of output: 550
Script:
#!/bin/bash # Description: Search for the definition of `SceneParser.parse` and verify its return type includes `ParserContext`. # Test: Locate the definition of `SceneParser.parse`. rg --type ts 'parse' -A 5 | grep -B 5 'SceneParser'Length of output: 6127
packages/xr-webxr/src/WebXRSession.ts (2)
Line range hint
1-5:
Imports look good.The imports include necessary modules for the functionality of the
WebXRSessionclass.
142-143: MethodremoveEventListenerlooks good.The method correctly handles the removal of event listeners, including the "end" event.
packages/xr/src/feature/trackable/XRTrackableFeature.ts (5)
1-2: Imports look good.The imports include necessary modules for the functionality of the
XRTrackableFeatureclass.
19-21: Properties look good.The properties are necessary for managing tracked components.
63-66: MethodgetTrackedComponentByTrackIdlooks good.The method correctly retrieves a tracked component by its track ID.
215-228: Method_createOrUpdateTrackedComponentslooks good.The method correctly creates or updates tracked components.
231-244: Method_createTrackedComponentslooks good.The method correctly creates tracked components.
packages/xr/src/XRManagerExtended.ts (3)
63-74: Add error handling for unsupported features and duplicate additions.The changes ensure that unsupported features are not added and prevent duplicate feature additions. This improves the robustness of the feature addition process.
293-299: Interface method signature alignment.The interface method signature correctly includes the optional
outparameter.
317-317: Interface method signature alignment.The interface method signature correctly includes the constructor params of the feature.
| export class XRReferenceImageDecoder { | ||
| static decode(engine: Engine, bufferReader: BufferReader): Promise<XRReferenceImage> { | ||
| return new Promise((resolve, reject) => { | ||
| const physicalWidth = bufferReader.nextUint16(); | ||
| bufferReader.nextUint8(); | ||
| const img = new Image(); | ||
| img.onload = () => { | ||
| resolve(new XRReferenceImage("", img, physicalWidth)); | ||
| }; | ||
| img.src = URL.createObjectURL(new window.Blob([bufferReader.nextImagesData(1)[0]])); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid classes with only static members.
The XRReferenceImageDecoder class contains only static members. It would be better to use a simple function instead.
import { BufferReader, Engine } from "@galacean/engine";
import { XRReferenceImage } from "../feature/trackable/image/XRReferenceImage";
export const decodeXRReferenceImage = (engine: Engine, bufferReader: BufferReader): Promise<XRReferenceImage> => {
return new Promise((resolve, reject) => {
const physicalWidth = bufferReader.nextUint16();
bufferReader.nextUint8();
const img = new Image();
img.onload = () => {
resolve(new XRReferenceImage("", img, physicalWidth));
};
img.src = URL.createObjectURL(new window.Blob([bufferReader.nextImagesData(1)[0]]));
});
};Tools
Biome
[error] 5-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/xr/src/XRManagerExtended.ts
Outdated
| override getFeatures(out?: XRFeature[]): XRFeature[] { | ||
| const { _features: features } = this; | ||
| const length = features.length; | ||
| if (out) { | ||
| out.length = 0; | ||
| out.length = length; | ||
| } else { | ||
| out = []; | ||
| out = new Array<XRFeature>(length); | ||
| } | ||
| const { _features: features } = this; | ||
| for (let i = 0, n = features.length; i < n; i--) { | ||
| const feature = features[i]; | ||
| feature instanceof type && out.push(feature); | ||
| for (let i = 0; i < length; i--) { | ||
| out[i] = features[i]; |
There was a problem hiding this comment.
Fix the typo in the loop.
The loop decrements the index instead of incrementing it, which will cause an infinite loop.
- for (let i = 0; i < length; i--) {
+ for (let i = 0; i < length; i++) {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.
| override getFeatures(out?: XRFeature[]): XRFeature[] { | |
| const { _features: features } = this; | |
| const length = features.length; | |
| if (out) { | |
| out.length = 0; | |
| out.length = length; | |
| } else { | |
| out = []; | |
| out = new Array<XRFeature>(length); | |
| } | |
| const { _features: features } = this; | |
| for (let i = 0, n = features.length; i < n; i--) { | |
| const feature = features[i]; | |
| feature instanceof type && out.push(feature); | |
| for (let i = 0; i < length; i--) { | |
| out[i] = features[i]; | |
| override getFeatures(out?: XRFeature[]): XRFeature[] { | |
| const { _features: features } = this; | |
| const length = features.length; | |
| if (out) { | |
| out.length = length; | |
| } else { | |
| out = new Array<XRFeature>(length); | |
| } | |
| for (let i = 0; i < length; i++) { | |
| out[i] = features[i]; |
Tools
Biome
[error] 102-102: The update clause in this loop moves the variable in the wrong direction.
(lint/correctness/useValidForDirection)
There was a problem hiding this comment.
@cptbtptpbcptdtptp, thank you for fixing the loop issue. Everything looks good now!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| addHitTest(xrManager: XRManager, schema: IHitTestSchema): void { | ||
| if (!xrManager.isSupportedFeature(XRHitTest)) { | ||
| Logger.error("Plane Tracking is not supported."); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Update error message.
The error message on line 111 should be updated to reflect the correct feature.
- Logger.error("Plane Tracking is not supported.");
+ Logger.error("Hit Test is not supported.");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.
| addHitTest(xrManager: XRManager, schema: IHitTestSchema): void { | |
| if (!xrManager.isSupportedFeature(XRHitTest)) { | |
| Logger.error("Plane Tracking is not supported."); | |
| return; | |
| } | |
| } | |
| addHitTest(xrManager: XRManager, schema: IHitTestSchema): void { | |
| if (!xrManager.isSupportedFeature(XRHitTest)) { | |
| Logger.error("Hit Test is not supported."); | |
| return; | |
| } | |
| } |
There was a problem hiding this comment.
@cptbtptpbcptdtptp, thank you for the update! If there's anything else you need, feel free to ask. Happy coding! 🐰
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| override async onSceneParse(engine: Engine, context: ParserContext<IScene, Scene>, data: IXRScene): Promise<void> { | ||
| const { xrManager } = engine; | ||
| if (!xrManager) { | ||
| Logger.error("XRManager is not found in the engine."); | ||
| return; | ||
| } | ||
| const { xr } = data.scene; | ||
| const { origin, camera, leftCamera, rightCamera, features } = xr; | ||
| const { entityMap } = context; | ||
| origin && (xrManager.origin = entityMap.get(origin)); | ||
| const { cameraManager } = xrManager; | ||
| this.setCamera(cameraManager, XRTrackedInputDevice.Camera, entityMap.get(camera)); | ||
| this.setCamera(cameraManager, XRTrackedInputDevice.LeftCamera, entityMap.get(leftCamera)); | ||
| this.setCamera(cameraManager, XRTrackedInputDevice.RightCamera, entityMap.get(rightCamera)); | ||
|
|
||
| for (let i = 0, n = features.length; i < n; i++) { | ||
| const feature = features[i]; | ||
| if (!feature.enable) continue; | ||
| switch (feature.type) { | ||
| case XRFeatureType.ImageTracking: | ||
| await this.addImageTracking(engine, xrManager, <IImageTrackingSchema>feature); | ||
| break; | ||
| case XRFeatureType.PlaneTracking: | ||
| this.addPlaneTracking(xrManager, <IPlaneTrackingSchema>feature); | ||
| break; | ||
| case XRFeatureType.AnchorTracking: | ||
| this.addAnchorTracking(xrManager, <IAnchorTrackingSchema>feature); | ||
| break; | ||
| case XRFeatureType.HitTest: | ||
| this.addHitTest(xrManager, <IHitTestSchema>feature); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor assignment in expression.
The assignment in an expression on line 42 should be avoided for better readability.
- origin && (xrManager.origin = entityMap.get(origin));
+ if (origin) {
+ xrManager.origin = entityMap.get(origin);
+ }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.
| override async onSceneParse(engine: Engine, context: ParserContext<IScene, Scene>, data: IXRScene): Promise<void> { | |
| const { xrManager } = engine; | |
| if (!xrManager) { | |
| Logger.error("XRManager is not found in the engine."); | |
| return; | |
| } | |
| const { xr } = data.scene; | |
| const { origin, camera, leftCamera, rightCamera, features } = xr; | |
| const { entityMap } = context; | |
| origin && (xrManager.origin = entityMap.get(origin)); | |
| const { cameraManager } = xrManager; | |
| this.setCamera(cameraManager, XRTrackedInputDevice.Camera, entityMap.get(camera)); | |
| this.setCamera(cameraManager, XRTrackedInputDevice.LeftCamera, entityMap.get(leftCamera)); | |
| this.setCamera(cameraManager, XRTrackedInputDevice.RightCamera, entityMap.get(rightCamera)); | |
| for (let i = 0, n = features.length; i < n; i++) { | |
| const feature = features[i]; | |
| if (!feature.enable) continue; | |
| switch (feature.type) { | |
| case XRFeatureType.ImageTracking: | |
| await this.addImageTracking(engine, xrManager, <IImageTrackingSchema>feature); | |
| break; | |
| case XRFeatureType.PlaneTracking: | |
| this.addPlaneTracking(xrManager, <IPlaneTrackingSchema>feature); | |
| break; | |
| case XRFeatureType.AnchorTracking: | |
| this.addAnchorTracking(xrManager, <IAnchorTrackingSchema>feature); | |
| break; | |
| case XRFeatureType.HitTest: | |
| this.addHitTest(xrManager, <IHitTestSchema>feature); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| } | |
| override async onSceneParse(engine: Engine, context: ParserContext<IScene, Scene>, data: IXRScene): Promise<void> { | |
| const { xrManager } = engine; | |
| if (!xrManager) { | |
| Logger.error("XRManager is not found in the engine."); | |
| return; | |
| } | |
| const { xr } = data.scene; | |
| const { origin, camera, leftCamera, rightCamera, features } = xr; | |
| const { entityMap } = context; | |
| if (origin) { | |
| xrManager.origin = entityMap.get(origin); | |
| } | |
| const { cameraManager } = xrManager; | |
| this.setCamera(cameraManager, XRTrackedInputDevice.Camera, entityMap.get(camera)); | |
| this.setCamera(cameraManager, XRTrackedInputDevice.LeftCamera, entityMap.get(leftCamera)); | |
| this.setCamera(cameraManager, XRTrackedInputDevice.RightCamera, entityMap.get(rightCamera)); | |
| for (let i = 0, n = features.length; i < n; i++) { | |
| const feature = features[i]; | |
| if (!feature.enable) continue; | |
| switch (feature.type) { | |
| case XRFeatureType.ImageTracking: | |
| await this.addImageTracking(engine, xrManager, <IImageTrackingSchema>feature); | |
| break; | |
| case XRFeatureType.PlaneTracking: | |
| this.addPlaneTracking(xrManager, <IPlaneTrackingSchema>feature); | |
| break; | |
| case XRFeatureType.AnchorTracking: | |
| this.addAnchorTracking(xrManager, <IAnchorTrackingSchema>feature); | |
| break; | |
| case XRFeatureType.HitTest: | |
| this.addHitTest(xrManager, <IHitTestSchema>feature); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| } |
Tools
Biome
[error] 42-42: 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.
@cptbtptpbcptdtptp, thank you for applying the change. If there are no further modifications or issues, we can proceed with the review. Let me know if there's anything else you need assistance with.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/xr/src/XRManagerExtended.ts (4 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (5 hunks)
- packages/xr/src/loader/XRCustomParser.ts (1 hunks)
- packages/xr/src/loader/XRSceneSchema.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/xr/src/XRManagerExtended.ts
- packages/xr/src/feature/trackable/XRTrackableFeature.ts
Additional context used
Biome
packages/xr/src/loader/XRCustomParser.ts
[error] 36-36: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (15)
packages/xr/src/loader/XRSceneSchema.ts (7)
1-1: LGTM! Imports are appropriate.The new imports for
IReferable,IVector3, andIVector4are necessary for the new interfaces.
3-4: LGTM! Imports are appropriate.The new imports for
XRFeatureTypeandXRPlaneModeare necessary for the new interfaces.
6-16: LGTM! New declarations are appropriate.The new declarations extend the
ISceneConfiginterface to include XR configurations, which is necessary for XR feature support.
19-22: LGTM! New interface is appropriate.The new interface
IXRFeatureSchemaprovides a base schema for XR features, which is necessary for defining specific XR feature schemas.
24-27: LGTM! New interface is appropriate.The new interface
IAnchorTrackingSchemaextendsIXRFeatureSchemaand adds propertiesanchorsandprefab, which are necessary for defining the schema for anchor tracking in XR.
29-32: LGTM! New interface is appropriate.The new interface
IImageTrackingSchemaextendsIXRFeatureSchemaand adds propertiesimagesandprefab, which are necessary for defining the schema for image tracking in XR.
34-44: LGTM! New interfaces are appropriate.The new interfaces
IHitTestSchema,IPlaneTrackingSchema, andIAnchorare necessary for defining the schema for hit testing, plane tracking, and anchor properties in XR.packages/xr/src/loader/XRCustomParser.ts (8)
1-23: LGTM! Imports are appropriate.The new imports for various XR-related classes and types are necessary for the new methods and functionalities added in this file.
25-62: LGTM! New class and its methods are appropriate.The new class
XRCustomParserprovides necessary functionalities for handling XR scene data and configuring various XR features.Tools
Biome
[error] 36-36: 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-78: LGTM! New method is appropriate.The new method
_addImageTrackingchecks for feature support and adds image tracking if supported. The use of promises to get resources is appropriate.
80-86: LGTM! New method is appropriate.The new method
_addPlaneTrackingchecks for feature support and adds plane tracking if supported.
88-101: LGTM! New method is appropriate.The new method
_addAnchorTrackingchecks for feature support and adds anchor tracking if supported. It also adds anchors to the anchor tracking feature.
103-108: LGTM! New method is appropriate.The new method
_addHitTestchecks for feature support and adds hit test if supported.
110-113: LGTM! New method is appropriate.The new method
_setCameraattaches the camera to the specified device if the entity has a camera component.
116-116: LGTM! New type is appropriate.The new type
CameraDeviceis necessary for the_setCameramethod to specify camera devices.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
packages/xr/src/feature/trackable/XRTrackableFeature.ts (1)
190-216: Remove or replaceconsole.logstatements.The
console.logstatements should be removed or replaced with proper logging.- console.log("add", added[i].id); - console.log("updated", updated[i].id); - console.log("remove", id);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- packages/loader/src/index.ts (2 hunks)
- packages/loader/src/resource-deserialize/index.ts (2 hunks)
- packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (4 hunks)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts (3 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (6 hunks)
- packages/xr/src/index.ts (2 hunks)
- packages/xr/src/loader/XRReferenceImageDecoder.ts (1 hunks)
- packages/xr/src/loader/XRSceneExtendParser.ts (1 hunks)
- packages/xr/src/loader/XRSceneSchema.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/loader/src/index.ts
- packages/loader/src/resource-deserialize/index.ts
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts
- packages/xr/src/loader/XRSceneSchema.ts
Additional context used
Biome
packages/xr/src/loader/XRReferenceImageDecoder.ts
[error] 5-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/xr/src/loader/XRSceneExtendParser.ts
[error] 35-35: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (4)
packages/xr/src/index.ts (1)
5-6: LGTM!The added exports are consistent with the existing structure.
packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (3)
1-3: Import statements look good.The new import statements for
GLTFResourceandPrefabResourceare appropriate and necessary for the new functionality.
66-68: Method_extendParselooks good.The method
_extendParseis correctly implemented to return a resolved promise. It serves as a placeholder for future extensions.
40-40: Ensure_extendParseis correctly invoked.The method
_extendParseis correctly bound in the constructor and invoked in thestartmethod.Also applies to: 57-57
| export class XRReferenceImageDecoder { | ||
| static decode(engine: Engine, bufferReader: BufferReader): Promise<XRReferenceImage> { | ||
| return new Promise((resolve, reject) => { | ||
| const physicalWidth = bufferReader.nextFloat32(); | ||
| bufferReader.nextUint8(); | ||
| const img = new Image(); | ||
| img.onload = () => { | ||
| resolve(new XRReferenceImage("", img, physicalWidth)); | ||
| }; | ||
| img.src = URL.createObjectURL(new window.Blob([bufferReader.nextImagesData(1)[0]])); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid classes with only static members.
The XRReferenceImageDecoder class contains only static members. It would be better to use a simple function instead.
import { BufferReader, Engine } from "@galacean/engine";
import { XRReferenceImage } from "../feature/trackable/image/XRReferenceImage";
export const decodeXRReferenceImage = (engine: Engine, bufferReader: BufferReader): Promise<XRReferenceImage> => {
return new Promise((resolve, reject) => {
const physicalWidth = bufferReader.nextFloat32();
bufferReader.nextUint8();
const img = new Image();
img.onload = () => {
resolve(new XRReferenceImage("", img, physicalWidth));
};
img.src = URL.createObjectURL(new window.Blob([bufferReader.nextImagesData(1)[0]]));
});
};Tools
Biome
[error] 5-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (8 hunks)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts (3 hunks)
- packages/xr/src/component/XRTrackedComponent.ts (1 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (6 hunks)
- packages/xr/src/loader/XRSceneExtendParser.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts
- packages/xr/src/component/XRTrackedComponent.ts
- packages/xr/src/feature/trackable/XRTrackableFeature.ts
Additional context used
Biome
packages/xr/src/loader/XRSceneExtendParser.ts
[error] 25-118: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 35-35: 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] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 49-49: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 52-52: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 55-55: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (18)
packages/xr/src/loader/XRSceneExtendParser.ts (6)
63-81: LGTM!The function
_addImageTrackingis correctly implemented.
83-89: LGTM!The function
_addPlaneTrackingis correctly implemented.
91-104: LGTM!The function
_addAnchorTrackingis correctly implemented.
106-112: LGTM!The function
_addHitTestis correctly implemented.
114-117: LGTM!The function
_setCamerais correctly implemented.
120-120: LGTM!The type
CameraDeviceis correctly defined.packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (12)
Line range hint
1-47:
LGTM!The constructor is correctly implemented.
Line range hint
48-54:
LGTM!The function
startis correctly implemented.
Line range hint
55-55:
LGTM!The function
_handleRootEntityis correctly defined as abstract.
Line range hint
56-56:
LGTM!The function
_clearAndResolveis correctly defined as abstract.
Line range hint
64-83:
LGTM!The function
_parseEntitiesis correctly implemented.
Line range hint
84-104:
LGTM!The function
_parseComponentsis correctly implemented.
Line range hint
105-138:
LGTM!The function
_parsePrefabModificationis correctly implemented.
Line range hint
139-165:
LGTM!The function
_parsePrefabRemovedEntitiesis correctly implemented.
Line range hint
166-192:
LGTM!The function
_parsePrefabRemovedComponentsis correctly implemented.
Line range hint
193-198:
LGTM!The function
_organizeEntitiesis correctly implemented.
Line range hint
199-210:
LGTM!The function
_getEntityByConfigis correctly implemented.
Line range hint
211-217:
LGTM!The function
_parseEntityis correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/xr/src/loader/XRSceneExtendParser.ts (1 hunks)
Additional context used
Biome
packages/xr/src/loader/XRSceneExtendParser.ts
[error] 26-136: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 36-36: 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] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 47-47: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 53-53: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 56-56: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 82-82: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (5)
packages/xr/src/loader/XRSceneExtendParser.ts (5)
88-101: LGTM!The
_addPlaneTrackingfunction is well-structured and does not contain any issues.
103-122: LGTM!The
_addAnchorTrackingfunction is well-structured and does not contain any issues.
124-130: LGTM!The
_addHitTestfunction is well-structured and does not contain any issues.
132-135: LGTM!The
_setCamerafunction is well-structured and does not contain any issues.
25-25: LGTM!The
registerSceneExtendParserdecorator usage is appropriate and does not contain any issues.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/xr/src/loader/XRReferenceImageLoader.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/xr/src/loader/XRReferenceImageLoader.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/index.ts (1 hunks)
Additional comments not posted (2)
packages/core/src/index.ts (2)
1-1: General Approval: Export statements appear correctly modified and organized.The export statements have been reorganized and modified as per the PR's objectives. This reorganization helps in better module management and clarity for developers using this package.
Also applies to: 3-3, 9-10, 12-12, 15-15, 17-17, 22-25, 27-28, 33-33, 35-35, 36-36, 37-37, 38-38, 40-40, 47-47, 48-48, 65-65
1-1: Verification of Specific Export Changes:Based on the AI-generated context, the changes in the exports have been implemented as described. This includes modifications and reordering of export statements to align with the new functionalities and structural changes in the core package.
Also applies to: 9-10, 12-12, 15-15, 17-17, 22-25, 27-28, 33-33, 35-35, 36-36, 37-37, 38-38, 40-40, 47-47, 48-48, 65-65
Verification successful
Verification of Specific Export Changes:
The export statements in
packages/core/src/index.tsmatch the provided code snippet and the review comment. The following export statements are present:
export { Canvas } from "./Canvas";export { BoolUpdateFlag } from "./BoolUpdateFlag";export { DependentMode, dependentComponents } from "./ComponentsDependencies";export { Entity } from "./Entity";export { Script } from "./Script";export { Loader } from "./asset/Loader";export { BackgroundMode } from "./enums/BackgroundMode";export { FogMode } from "./enums/FogMode";export { XRManager } from "./xr/XRManager";These confirm the changes mentioned in the review comment.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify specific export changes in the codebase. # Expect: Export statements should match the provided code. rg --type js -A 5 $'export { Canvas } from "./Canvas";' rg --type js -A 5 $'export { BoolUpdateFlag } from "./BoolUpdateFlag";' rg --type js -A 5 $'export { DependentMode, dependentComponents } from "./ComponentsDependencies";' rg --type js -A 5 $'export { Entity } from "./Entity";' rg --type js -A 5 $'export { Script } from "./Script";' rg --type js -A 5 $'export { Loader } from "./asset/Loader";' rg --type js -A 5 $'export { BackgroundMode } from "./enums/BackgroundMode";' rg --type js -A 5 $'export { FogMode } from "./enums/FogMode";' rg --type js -A 5 $'export { XRManager } from "./xr/XRManager";'Length of output: 1856
Script:
#!/bin/bash # Description: Verify specific export changes in the codebase. # Expect: Export statements should match the provided code. rg --type js -A 5 $'export \\{ Canvas \\} from "./Canvas";' rg --type js -A 5 $'export \\{ BoolUpdateFlag \\} from "./BoolUpdateFlag";' rg --type js -A 5 $'export \\{ DependentMode, dependentComponents \\} from "./ComponentsDependencies";' rg --type js -A 5 $'export \\{ Entity \\} from "./Entity";' rg --type js -A 5 $'export \\{ Script \\} from "./Script";' rg --type js -A 5 $'export \\{ Loader \\} from "./asset/Loader";' rg --type js -A 5 $'export \\{ BackgroundMode \\} from "./enums/BackgroundMode";' rg --type js -A 5 $'export \\{ FogMode \\} from "./enums/FogMode";' rg --type js -A 5 $'export \\{ XRManager \\} from "./xr/XRManager";'Length of output: 635
Script:
#!/bin/bash # Description: Verify the presence of export statements in the relevant files. # Search for all export statements in the relevant files rg --type js 'export ' packages/core/src/index.tsLength of output: 2753
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/loader/src/resource-deserialize/resources/schema/SceneSchema.ts (1 hunks)
Additional comments not posted (2)
packages/loader/src/resource-deserialize/resources/schema/SceneSchema.ts (2)
19-21: Review of theISceneinterface changes.The refactoring of the
ISceneinterface to includeISceneConfigandfilesproperties is a good structural improvement. It helps in better organizing the scene-related configurations and makes the interface more modular.
23-68: Review of theISceneConfiginterface.This interface is well-structured and covers a comprehensive range of scene settings including background, ambient settings, shadows, fog, and post-processing. The use of optional properties for features like
shadowandfogprovides flexibility in scene configuration.
- Background Configuration: The properties are well-defined, and the use of optional properties for texture-related settings allows for flexibility in defining backgrounds.
- Ambient Settings: The detailed configuration options for ambient light and specular settings are beneficial for fine-tuning scene lighting.
- Shadow Settings: The optional shadow configuration supports various shadow resolutions and cascades, which is crucial for performance adjustments based on the scene complexity.
- Fog Settings: The fog configuration is straightforward and uses simple properties to define fog behavior.
- Post Processing: The detailed settings for post-processing effects like bloom and tonemapping are valuable for achieving specific visual effects.
Overall, the
ISceneConfiginterface is comprehensive and well-designed to cater to a variety of scene configurations.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/resource-deserialize/resources/scene/SceneParser.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/xr/src/XRManagerExtended.ts (14 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (8 hunks)
- packages/xr/src/session/XRSessionManager.ts (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts
- packages/xr/src/session/XRSessionManager.ts
Additional context used
Biome
packages/xr/src/XRManagerExtended.ts
[error] 259-259: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
Additional comments not posted (9)
packages/xr/src/XRManagerExtended.ts (9)
16-16: LGTM!The static property
_listenersPoolis correctly initialized.
113-115: LGTM!The
enterXRmethod correctly initializes and displays the XR virtual world.
155-157: LGTM!The
_updatemethod correctly updates the session manager, input manager, and camera manager.
204-206: LGTM!The
_onSessionStopmethod correctly stops all features.
215-217: LGTM!The
_onSessionInitmethod correctly initializes all features.
227-229: LGTM!The
_onSessionStartmethod correctly starts the camera manager and all features.
Line range hint
239-243:
LGTM!The
_onSessionExitmethod correctly exits the camera manager and all features.
Line range hint
87-92:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
getFeaturematch the new signature.
65-77: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
addFeaturematch the new signature.Verification successful
Function usage verified successfully.
The
addFeaturemethod calls in the following files match the new signature and are consistent with the updated method definition:
examples/xr-ar-planeTracking.tsexamples/xr-ar-imageTracking.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `addFeature` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'addFeature'Length of output: 73
Script:
#!/bin/bash # Description: Verify all function calls to `addFeature` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type ts -A 5 $'addFeature'Length of output: 1929
Script:
#!/bin/bash # Description: Verify the context and arguments of `addFeature` calls in example files. # Extract more lines around the addFeature calls in example files for detailed verification. rg --type ts -A 10 -B 5 $'addFeature' examples/xr-ar-planeTracking.ts examples/xr-ar-imageTracking.tsLength of output: 2441
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts
packages/xr/src/XRManagerExtended.ts
Outdated
|
|
||
| /** Input manager for XR. */ | ||
| override inputManager: XRInputManager; | ||
| /** Session manager for XR. */ | ||
| override sessionManager: XRSessionManager; | ||
| /** Camera manager for XR. */ | ||
| override cameraManager: XRCameraManager; | ||
| /** All initialized features at this moment. */ |
There was a problem hiding this comment.
Opt this comments, Too colloquial！
| this._listeners.push(listener); | ||
| addChangedListener(fn: (added: readonly T[], updated: readonly T[], removed: readonly T[]) => void): void { | ||
| const { _listeners: listeners } = this; | ||
| if (!listeners.find((listener) => listener.fn === fn)) { |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/xr/src/XRManagerExtended.ts (14 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (5 hunks)
- packages/xr/src/session/XRSessionManager.ts (8 hunks)
- packages/xr/src/session/XRSessionState.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/xr/src/XRManagerExtended.ts
- packages/xr/src/feature/trackable/XRTrackableFeature.ts
- packages/xr/src/session/XRSessionManager.ts
- packages/xr/src/session/XRSessionState.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/xr/src/XRManagerExtended.ts (15 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (6 hunks)
- packages/xr/src/session/XRSessionManager.ts (7 hunks)
Additional context used
Learnings (1)
packages/xr/src/session/XRSessionManager.ts (1)
Learnt from: cptbtptpbcptdtptp PR: galacean/engine#2165 File: packages/xr/src/session/XRSessionManager.ts:0-0 Timestamp: 2024-07-18T09:47:39.965Z Learning: Consider handling modifications during iteration in the `set state` method of the `XRSessionManager` class to avoid mutation issues by creating a copy of the `_listeners` array.
Additional comments not posted (25)
packages/xr/src/feature/trackable/XRTrackableFeature.ts (6)
14-15: Enhance type safety with default generic parameters.Specifying default types for the generic parameters
TandKenhances type safety and usability.
25-25: Refactor_listenersproperty for better listener management.The
_listenersproperty now stores objects containing a function reference and adestroyedflag, allowing for better management of listener removal and preventing memory leaks.
32-35: Prevent duplicate listeners inaddChangedListener.The updated
addChangedListenermethod checks for existing listeners before adding, ensuring that duplicate listeners are not registered.
44-49: Mark listeners as destroyed inremoveChangedListener.The
removeChangedListenermethod now marks listeners asdestroyedinstead of directly removing them, allowing for deferred cleanup.
119-132: Optimize event notification with a temporary listener array.The logic in the event notification section utilizes a temporary listener array, optimizing the iteration and invocation of listeners while preventing issues from modifying the listener array during iteration.
143-143: Reset all relevant properties during session exit.The
_onSessionExitmethod now resets all relevant properties, ensuring a clean state when the session exits.packages/xr/src/session/XRSessionManager.ts (9)
20-20: Add_listenersproperty for managing state change listeners.The
_listenersproperty is introduced to hold functions that respond to state changes, enhancing the event-driven capabilities of the class.
116-121: AddaddChangedListenermethod for registering state change listeners.The
addChangedListenermethod facilitates the registration of listeners for session state changes.
127-136: AddremoveChangedListenermethod for deregistering state change listeners.The
removeChangedListenermethod facilitates the deregistration of listeners for session state changes.
141-157: Implement_setStatemethod for updating session state and notifying listeners.The
_setStatemethod updates the session state and notifies all registered listeners, streamlining the state management process.
82-82: Updaterunmethod to use_setState.The
runmethod now uses_setStateto update the session state, ensuring consistency in state management.
104-104: Updatestopmethod to use_setState.The
stopmethod now uses_setStateto update the session state, ensuring consistency in state management.
176-176: Update_initializemethod to use_setState.The
_initializemethod now uses_setStateto update the session state, ensuring consistency in state management.
234-234: Update_onSessionExitmethod to use_setState.The
_onSessionExitmethod now uses_setStateto update the session state, ensuring consistency in state management.
246-248: Reset listeners and other properties during destruction.The
_onDestroymethod now resets the_listenersarray and other properties, ensuring a clean state when the session manager is destroyed.packages/xr/src/XRManagerExtended.ts (10)
16-16: Add_listenersPoolproperty for managing listener arrays.The
_listenersPoolproperty is introduced to manage arrays of listeners, optimizing memory usage and performance.
25-25: Addfeaturesproperty for tracking initialized XR features.The
featuresproperty is added to track initialized XR features, simplifying the management and retrieval of features.
65-77: RefactoraddFeaturemethod for improved feature management.The
addFeaturemethod now checks for feature support before adding a feature and returns the instance type of the added feature, ensuring robust feature management.
87-87: RefactorgetFeaturemethod for improved feature retrieval.The
getFeaturemethod now retrieves features from thefeaturesarray, simplifying the interface and ensuring consistency.
113-115: UpdateenterXRmethod to use_setState.The
enterXRmethod now uses_setStateto update the session state during initialization, ensuring consistency in state management.
154-154: Update_updatemethod to iterate overfeaturesarray.The
_updatemethod now iterates over thefeaturesarray, ensuring that all initialized features are updated correctly.
203-203: Update_onSessionStopmethod to iterate overfeaturesarray.The
_onSessionStopmethod now iterates over thefeaturesarray, ensuring that all initialized features are stopped correctly.
214-214: Update_onSessionInitmethod to iterate overfeaturesarray.The
_onSessionInitmethod now iterates over thefeaturesarray, ensuring that all initialized features are initialized correctly.
226-226: Update_onSessionStartmethod to iterate overfeaturesarray.The
_onSessionStartmethod now iterates over thefeaturesarray, ensuring that all initialized features are started correctly.
238-238: Update_onSessionExitmethod to iterate overfeaturesarray and reset state.The
_onSessionExitmethod now iterates over thefeaturesarray to exit all features and resets the state, ensuring a clean exit process.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (6 hunks)
- packages/xr/src/session/XRSessionManager.ts (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts
- packages/xr/src/session/XRSessionManager.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/xr/src/XRManagerExtended.ts (15 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/xr/src/XRManagerExtended.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/core/src/utils/SafeLoopArray.ts (1 hunks)
- packages/core/src/utils/index.ts (1 hunks)
- packages/xr/src/XRManagerExtended.ts (15 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (5 hunks)
- packages/xr/src/session/XRSessionManager.ts (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/xr/src/XRManagerExtended.ts
Additional context used
Learnings (1)
packages/xr/src/session/XRSessionManager.ts (1)
Learnt from: cptbtptpbcptdtptp PR: galacean/engine#2165 File: packages/xr/src/session/XRSessionManager.ts:0-0 Timestamp: 2024-07-18T09:47:39.965Z Learning: Consider handling modifications during iteration in the `set state` method of the `XRSessionManager` class to avoid mutation issues by creating a copy of the `_listeners` array.
Biome
packages/xr/src/feature/trackable/XRTrackableFeature.ts
[error] 42-42: 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] 126-126: 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/xr/src/session/XRSessionManager.ts
[error] 126-126: 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] 229-229: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (12)
packages/core/src/utils/index.ts (1)
4-4: Export statement forSafeLoopArraylooks good.The added export statement correctly makes
SafeLoopArrayavailable for import in other parts of the application.packages/core/src/utils/SafeLoopArray.ts (1)
41-50: MethodfindAndRemovelooks good but could be optimized.The method correctly iterates through the array in reverse order and removes elements based on the provided filter function. However, the use of
splicewithin a loop can be inefficient for large arrays. Consider using a more efficient approach if performance becomes an issue.packages/xr/src/feature/trackable/XRTrackableFeature.ts (5)
14-17: Default generic types forXRTrackableFeaturelook good.The addition of default generic types enhances type safety and usability.
26-26: Initialization of_listenerswithSafeLoopArraylooks good.The
_listenersproperty is correctly initialized with an instance ofSafeLoopArray.
33-33: Adding listener function to_listenerslooks good.The listener function is correctly added to the
_listenersarray.
111-113: Listener invocation logic looks good but ensure proper cleanup.The logic for invoking listeners while checking the
destroyedflag is correct. Ensure that the cleanup of destroyed listeners is handled appropriately to prevent memory leaks.
1-1: Ensure proper import paths forSafeLoopArray.The import statement for
SafeLoopArrayshould be verified to ensure the correct module path is used.packages/xr/src/session/XRSessionManager.ts (5)
82-82: Verify the definition and usage of_setState.Ensure that
_setStateis defined and used correctly within the file.Verification successful
The
_setStatemethod is correctly defined and used within theXRSessionManagerclass.
- Definition of
_setStatefound inpackages/xr/src/session/XRSessionManager.ts.- Multiple usages of
_setStatewithin the same file confirm its appropriate application.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `_setState`. # Test: Search for the definition of `_setState`. Expect: Definition within `XRSessionManager`. rg --type typescript -A 5 $'_setState'Length of output: 72
Script:
#!/bin/bash # Description: Verify the definition and usage of `_setState`. # Test: Search for the definition of `_setState`. Expect: Definition within `XRSessionManager`. rg --type ts -A 5 $'_setState'Length of output: 2807
112-118: Verify the implementation ofaddChangedListener.Ensure that the method correctly adds listeners and verify the type
IXRListener.Verification successful
The implementation of
addChangedListenerand the usage of the typeIXRListenerare correct.
- The
addChangedListenermethod correctly adds a listener to the_listenersarray.- The type
IXRListeneris appropriately defined and used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `addChangedListener` and the type `IXRListener`. # Test: Search for the usage of `addChangedListener`. Expect: Correct implementation. rg --type typescript -A 5 $'addChangedListener'Length of output: 81
Script:
#!/bin/bash # Description: Verify the implementation of `addChangedListener` and the type `IXRListener`. # Test: Search for the implementation and usage of `addChangedListener`. rg 'addChangedListener' -t ts -A 5 # Test: Search for the type `IXRListener`. rg 'IXRListener' -t ts -A 5Length of output: 4354
3-3: Verify the usage ofIXRListener.Ensure that
IXRListeneris correctly defined and used within the file.Verification successful
Verified the usage of
IXRListener.
- The interface
IXRListeneris correctly defined inpackages/xr/src/XRManagerExtended.ts.- The import and usage of
IXRListenerinpackages/xr/src/session/XRSessionManager.tsare correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `IXRListener`. # Test: Search for the definition of `IXRListener`. Expect: Definition in `XRManagerExtended`. rg --type typescript -A 5 $'interface IXRListener'Length of output: 86
Script:
#!/bin/bash # Description: Verify the definition and usage of `IXRListener`. # Test: Search for the definition of `IXRListener`. Expect: Definition in `XRManagerExtended`. rg --type ts -A 5 $'interface IXRListener' # Test: Search for the usage of `IXRListener` within XRSessionManager.ts. rg --type ts 'IXRListener' packages/xr/src/session/XRSessionManager.tsLength of output: 685
104-104: Verify the usage of_setStatein thestopmethod.Ensure that
_setStateis used correctly within thestopmethod.Verification successful
The usage of
_setStatein thestopmethod is correct.The call to
_setState(XRSessionState.Paused)within thestopmethod is appropriate and consistent with the method's purpose of transitioning the session state to paused after stopping the platform session.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `_setState` within the `stop` method. # Test: Search for the usage of `_setState` within the `stop` method. Expect: Correct usage. rg --type typescript -A 5 $'stop()' | rg '_setState'Length of output: 86
Script:
#!/bin/bash # Description: Verify the usage of `_setState` within the `stop` method. # Test: Search for the usage of `_setState` within the `stop` method. Expect: Correct usage. rg --type-add 'ts:*.ts' -t ts -A 5 'stop()' | rg '_setState'Length of output: 147
Script:
#!/bin/bash # Description: Retrieve the full implementation of the `stop` method in XRSessionManager.ts. # Test: Capture the full `stop` method to verify the usage of `_setState`. rg --type-add 'ts:*.ts' -t ts -A 20 'stop()' packages/xr/src/session/XRSessionManager.tsLength of output: 1577
20-20: Verify the usage of_listeners.Ensure that the
_listenersarray is used correctly and verify the typeSafeLoopArray.Verification successful
Verify the methods used on
_listenersinXRSessionManager.ts.Ensure that the methods
push,findAndRemove, andgetLoopArrayare correctly defined inSafeLoopArray.
Verified the usage of
_listenersand the definition ofSafeLoopArray.The
_listenersarray inXRSessionManager.tsis used correctly with methodspush,findAndRemove, andgetLoopArray, which are properly defined in theSafeLoopArrayclass.
pushmethod is defined to add items to the array.findAndRemovemethod is defined to find and remove items based on a filter.getLoopArraymethod is defined to get a readonly array.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `SafeLoopArray`. # Test: Search for the definition of `SafeLoopArray`. Expect: Definition in the codebase. rg --type typescript -A 5 $'class SafeLoopArray'Length of output: 84
Script:
#!/bin/bash # Description: Verify the definition and usage of `SafeLoopArray`. # Test: Search for the definition of `SafeLoopArray`. Expect: Definition in the codebase. rg --type ts -A 5 $'class SafeLoopArray' # Test: Search for the usage of `_listeners` in `XRSessionManager.ts`. Expect: Usage in the file. rg '_listeners' packages/xr/src/session/XRSessionManager.tsLength of output: 767
Script:
#!/bin/bash # Description: Verify the methods used on `_listeners` in `XRSessionManager.ts`. # Test: Search for the definition of `push` method in `SafeLoopArray`. rg 'push' packages/core/src/utils/SafeLoopArray.ts # Test: Search for the definition of `findAndRemove` method in `SafeLoopArray`. rg 'findAndRemove' packages/core/src/utils/SafeLoopArray.ts # Test: Search for the definition of `getLoopArray` method in `SafeLoopArray`. rg 'getLoopArray' packages/core/src/utils/SafeLoopArray.tsLength of output: 365
| this._listeners.findAndRemove((value) => { | ||
| return value.fn === listener ? (value.destroyed = true) : false; | ||
| }); |
There was a problem hiding this comment.
Avoid assignment within expression in removeChangedListener.
The assignment within the expression should be avoided to improve readability and maintainability.
- return value.fn === listener ? (value.destroyed = true) : false;
+ if (value.fn === listener) {
+ value.destroyed = true;
+ return true;
+ }
+ return false;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._listeners.findAndRemove((value) => { | |
| return value.fn === listener ? (value.destroyed = true) : false; | |
| }); | |
| this._listeners.findAndRemove((value) => { | |
| if (value.fn === listener) { | |
| value.destroyed = true; | |
| return true; | |
| } | |
| return false; | |
| }); |
Tools
Biome
[error] 42-42: 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)
| this._requestTrackings.length = this._tracked.length = this._added.length = this._updated.length = this._removed.length = 0; | ||
| this._listeners.findAndRemove((value) => { | ||
| return (value.destroyed = true); | ||
| }); |
There was a problem hiding this comment.
Avoid assignment within expression in _onSessionExit.
The assignment within the expression should be avoided to improve readability and maintainability.
- return (value.destroyed = true);
+ value.destroyed = true;
+ return true;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._requestTrackings.length = this._tracked.length = this._added.length = this._updated.length = this._removed.length = 0; | |
| this._listeners.findAndRemove((value) => { | |
| return (value.destroyed = true); | |
| }); | |
| this._requestTrackings.length = this._tracked.length = this._added.length = this._updated.length = this._removed.length = 0; | |
| this._listeners.findAndRemove((value) => { | |
| value.destroyed = true; | |
| return true; | |
| }); |
Tools
Biome
[error] 126-126: 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)
| /** | ||
| * @internal | ||
| */ | ||
| _setState(value: XRSessionState) { | ||
| this._state = value; | ||
| const listeners = this._listeners.getLoopArray(); | ||
| for (let i = 0, n = listeners.length; i < n; i++) { | ||
| !listeners[i].destroyed && listeners[i].fn(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider handling modifications during iteration in _setState.
The current implementation directly iterates over the _listeners array. If any listener modifies this array (e.g., removes itself), it could lead to skipping listeners or other unintended effects.
_setState(value: XRSessionState) {
this._state = value;
const listeners = this._listeners.getLoopArray().slice(); // Create a copy to avoid mutation issues
for (let i = 0, n = listeners.length; i < n; i++) {
!listeners[i].destroyed && listeners[i].fn(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.
| /** | |
| * @internal | |
| */ | |
| _setState(value: XRSessionState) { | |
| this._state = value; | |
| const listeners = this._listeners.getLoopArray(); | |
| for (let i = 0, n = listeners.length; i < n; i++) { | |
| !listeners[i].destroyed && listeners[i].fn(value); | |
| } | |
| } | |
| _setState(value: XRSessionState) { | |
| this._state = value; | |
| const listeners = this._listeners.getLoopArray().slice(); // Create a copy to avoid mutation issues | |
| for (let i = 0, n = listeners.length; i < n; i++) { | |
| !listeners[i].destroyed && listeners[i].fn(value); | |
| } | |
| } |
| _onDestroy(): void { | ||
| this._listeners.findAndRemove((value) => { | ||
| return (value.destroyed = true); | ||
| }); | ||
| this._raf = this._caf = null; | ||
| } |
There was a problem hiding this comment.
Avoid assignment in expression in _onDestroy.
The use of assignments in expressions is confusing. It's better to separate the assignment from the expression.
_onDestroy(): void {
this._listeners.findAndRemove((value) => {
- return (value.destroyed = true);
+ value.destroyed = true;
+ return true;
});
this._raf = this._caf = null;
}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.
| _onDestroy(): void { | |
| this._listeners.findAndRemove((value) => { | |
| return (value.destroyed = true); | |
| }); | |
| this._raf = this._caf = null; | |
| } | |
| _onDestroy(): void { | |
| this._listeners.findAndRemove((value) => { | |
| value.destroyed = true; | |
| return true; | |
| }); | |
| this._raf = this._caf = null; | |
| } |
Tools
Biome
[error] 229-229: 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)
| /** | ||
| * Remove a listening function of session state changes. | ||
| * @param listener - The listening function | ||
| */ | ||
| removeChangedListener(listener: (state: XRSessionState) => void): void { | ||
| this._listeners.findAndRemove((value) => { | ||
| return value.fn === listener ? (value.destroyed = true) : false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Improve error handling in removeChangedListener.
The current implementation does not handle the case when the listener is not found in the _listeners array. It's a good practice to notify the caller if the operation did not succeed.
removeChangedListener(listener: (state: XRSessionState) => void): void {
const { _listeners: listeners } = this;
const index = listeners.findIndex((value) => value.fn === listener);
if (index >= 0) {
listeners[index].destroyed = true;
} else {
+ throw new Error("Listener not found");
}
}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.
| /** | |
| * Remove a listening function of session state changes. | |
| * @param listener - The listening function | |
| */ | |
| removeChangedListener(listener: (state: XRSessionState) => void): void { | |
| this._listeners.findAndRemove((value) => { | |
| return value.fn === listener ? (value.destroyed = true) : false; | |
| }); | |
| } | |
| /** | |
| * Remove a listening function of session state changes. | |
| * @param listener - The listening function | |
| */ | |
| removeChangedListener(listener: (state: XRSessionState) => void): void { | |
| const { _listeners: listeners } = this; | |
| const index = listeners.findIndex((value) => value.fn === listener); | |
| if (index >= 0) { | |
| listeners[index].destroyed = true; | |
| } else { | |
| throw new Error("Listener not found"); | |
| } | |
| } |
Tools
Biome
[error] 126-126: 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: 3
Outside diff range, codebase verification and nitpick comments (1)
packages/xr/src/session/XRSessionManager.ts (1)
Line range hint
214-227:
Avoid assignment within expression in_onDestroy.The assignment within the expression should be avoided to improve readability and maintainability.
- return (value.destroyed = true); + value.destroyed = true; + return true;Tools
Biome
[error] 226-226: 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 (3)
- packages/xr/src/XRManagerExtended.ts (14 hunks)
- packages/xr/src/feature/trackable/XRTrackableFeature.ts (5 hunks)
- packages/xr/src/session/XRSessionManager.ts (7 hunks)
Additional context used
Learnings (1)
packages/xr/src/session/XRSessionManager.ts (1)
Learnt from: cptbtptpbcptdtptp PR: galacean/engine#2165 File: packages/xr/src/session/XRSessionManager.ts:0-0 Timestamp: 2024-07-18T09:47:39.965Z Learning: Consider handling modifications during iteration in the `set state` method of the `XRSessionManager` class to avoid mutation issues by creating a copy of the `_listeners` array.
Biome
packages/xr/src/feature/trackable/XRTrackableFeature.ts
[error] 41-41: 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)
packages/xr/src/session/XRSessionManager.ts
[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] 226-226: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (11)
packages/xr/src/feature/trackable/XRTrackableFeature.ts (4)
1-3: LGTM!The import statements are necessary and correctly included.
14-17: LGTM!The class declaration with default types for the generic parameters
TandKimproves type safety and usability.
26-33: LGTM!The updates to the
_listenersproperty andaddChangedListenermethod improve listener management and prevent memory leaks.
Line range hint
109-123:
LGTM!The updates to the
_onUpdate,_onSessionStop, and_onSessionExitmethods improve the event-driven capabilities and ensure proper cleanup during session exit.packages/xr/src/session/XRSessionManager.ts (2)
1-3: LGTM!The import statements are necessary and correctly included.
Line range hint
20-118:
LGTM!The updates to the
_listenersproperty and the addition of theaddChangedListenermethod improve listener management and enhance the event-driven capabilities of the class.Tools
Biome
[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/xr/src/XRManagerExtended.ts (5)
Line range hint
20-57:
LGTM!The introduction of the
featuresarray and the refactoring of theaddFeaturemethod improve feature management and ensure that only supported features are added.
62-62: LGTM!The update to the
getFeaturemethod ensures consistency across the class.
82-92: LGTM!The updates to the
enterXRandexitXRmethods streamline the interface and reduce redundancy.
119-119: LGTM!The update to the
_updatemethod ensures consistency across the class.
Line range hint
168-203:
LGTM!The updates to the
_onSessionStop,_onSessionInit,_onSessionStart, and_onSessionExitmethods improve the event-driven capabilities and ensure proper cleanup during session exit.
| if (index >= 0) { | ||
| listeners.splice(index, 1); | ||
| } | ||
| this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false)); |
There was a problem hiding this comment.
Avoid assignment within expression in removeChangedListener.
The assignment within the expression should be avoided to improve readability and maintainability.
- return value.fn === listener ? (value.destroyed = true) : false;
+ if (value.fn === listener) {
+ value.destroyed = true;
+ return true;
+ }
+ return false;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._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false)); | |
| this._listeners.findAndRemove((value) => { | |
| if (value.fn === listener) { | |
| value.destroyed = true; | |
| return true; | |
| } | |
| return false; | |
| }); |
Tools
Biome
[error] 41-41: 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)
| removeChangedListener(listener: (state: XRSessionState) => void): void { | ||
| this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false)); | ||
| } |
There was a problem hiding this comment.
Avoid assignment within expression in removeChangedListener.
The assignment within the expression should be avoided to improve readability and maintainability.
- return value.fn === listener ? (value.destroyed = true) : false;
+ if (value.fn === listener) {
+ value.destroyed = true;
+ return true;
+ }
+ return false;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.
| removeChangedListener(listener: (state: XRSessionState) => void): void { | |
| this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false)); | |
| } | |
| removeChangedListener(listener: (state: XRSessionState) => void): void { | |
| this._listeners.findAndRemove((value) => { | |
| if (value.fn === listener) { | |
| value.destroyed = true; | |
| return true; | |
| } | |
| return false; | |
| }); | |
| } |
Tools
Biome
[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)
| _setState(value: XRSessionState) { | ||
| this._state = value; | ||
| const listeners = this._listeners.getLoopArray(); | ||
| for (let i = 0, n = listeners.length; i < n; i++) { | ||
| !listeners[i].destroyed && listeners[i].fn(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid assignment within expression in _setState.
The assignment within the expression should be avoided to improve readability and maintainability.
- !listeners[i].destroyed && listeners[i].fn(value);
+ if (!listeners[i].destroyed) {
+ listeners[i].fn(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.
| _setState(value: XRSessionState) { | |
| this._state = value; | |
| const listeners = this._listeners.getLoopArray(); | |
| for (let i = 0, n = listeners.length; i < n; i++) { | |
| !listeners[i].destroyed && listeners[i].fn(value); | |
| } | |
| } | |
| _setState(value: XRSessionState) { | |
| this._state = value; | |
| const listeners = this._listeners.getLoopArray(); | |
| for (let i = 0, n = listeners.length; i < n; i++) { | |
| if (!listeners[i].destroyed) { | |
| listeners[i].fn(value); | |
| } | |
| } | |
| } |
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
New Features
XRManagerExtendedwith better feature management and listener support.SafeLoopArrayusing custom filters.Improvements
XRSessionManagerwith new listener functionality.RequestingtoXRSessionState.Removals
PrefabLoaderexport from the loader package.Refactor
_extendParseinHierarchyParserfor enhanced parsing capabilities.