-
Notifications
You must be signed in to change notification settings - Fork 17
Refactor XR session handling into a script #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors XR session handling by moving the XR session management logic from JavaScript into a reusable script component, making it easier to migrate examples to Editor projects where everything needs to be script-based. The refactoring also replaces inline SVG icons with CSS classes that reference external SVG files.
- Extracts XR session management logic from
example.mjs
into a newxr-session.mjs
script - Replaces inline SVG icons with CSS classes and external SVG files
- Updates all HTML example files to include the new XR session script
- Simplifies button creation by using CSS classes instead of embedding SVG content
Reviewed Changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
examples/assets/scripts/xr-session.mjs | New script that handles XR session lifecycle, camera transforms, and keyboard controls |
examples/js/example.mjs | Removes XR session logic and icon constants, simplifies button creation with CSS classes |
examples/css/example.css | Adds CSS background images for icon buttons and updates background property names |
Multiple HTML files | Adds the new xr-session script asset and component to all XR-enabled examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
/** @private */ | ||
startSession(type, space) { | ||
if (!this.cameraEntity.camera) { | ||
console.error('XrSession: No cameraEntity.camera found on the entity.'); |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message on line 116 should be more specific about which entity is missing the camera component. Consider including the entity name or path in the error message.
console.error('XrSession: No cameraEntity.camera found on the entity.'); | |
console.error(`XrSession: No cameraEntity.camera found on entity "${this.cameraEntity.name}" (path: ${this.cameraEntity.getPath()}).`); |
Copilot uses AI. Check for mistakes.
} | ||
this.originalSkyType = null; | ||
} | ||
} |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using document.querySelector directly creates tight coupling to the DOM structure. Consider passing the sky element as a parameter or using the PlayCanvas entity system to find sky components instead.
} | |
if (!this.skyEntity) { | |
this.skyEntity = this.findSkyEntity(); | |
} | |
const sky = this.skyEntity; | |
if (!sky) return; | |
const skyComp = sky.sky; | |
if (this.originalSkyType === null) { | |
this.originalSkyType = skyComp ? skyComp.type : null; | |
} | |
if (skyComp) { | |
skyComp.type = 'none'; | |
} | |
} | |
/** @private */ | |
restoreSky() { | |
if (!this.skyEntity) { | |
this.skyEntity = this.findSkyEntity(); | |
} | |
const sky = this.skyEntity; | |
if (!sky) return; | |
const skyComp = sky.sky; | |
if (this.originalSkyType !== null) { | |
if (skyComp) { | |
skyComp.type = this.originalSkyType || 'none'; | |
} | |
this.originalSkyType = null; | |
} | |
} | |
/** | |
* Finds the sky entity in the scene. | |
* @private | |
* @returns {Entity|null} | |
*/ | |
findSkyEntity() { | |
// Try to find by name | |
let sky = this.app.root.findByName('Sky'); | |
if (sky && sky.sky) return sky; | |
// Try to find by tag | |
const skyEntities = this.app.root.findByTag('sky'); | |
for (let i = 0; i < skyEntities.length; i++) { | |
if (skyEntities[i].sky) return skyEntities[i]; | |
} | |
// Fallback: search all children for sky component | |
const allEntities = this.app.root.find(function (node) { | |
return node.sky !== undefined; | |
}); | |
if (allEntities && allEntities.length > 0) return allEntities[0]; | |
return null; | |
} |
Copilot uses AI. Check for mistakes.
|
||
/** @private */ | ||
restoreSky() { | ||
const sky = document.querySelector('pc-sky'); |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate DOM query for the sky element. Consider storing the sky element reference or extracting the sky finding logic into a separate method to avoid repeated queries.
const sky = document.querySelector('pc-sky'); | |
const sky = this.getSkyElement(); |
Copilot uses AI. Check for mistakes.
position: relative; | ||
height: 40px; | ||
background: rgba(255, 255, 255, 0.9); | ||
background-color: rgba(255, 255, 255, 0.9); |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from 'background' to 'background-color' is correct for specificity, but this could potentially override background images set by the icon classes. Consider using a more specific selector or ensuring proper CSS cascade order.
background-color: rgba(255, 255, 255, 0.9); | |
/* background-color moved to .example-button:not(.icon) */ |
Copilot uses AI. Check for mistakes.
This PR will make it easier to migrate the examples to Editor projects (where everything's a script).
Also saved icons to SVG files.