Added position-warp option - #29
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a position-warp teleportation method to the blink-controls component, providing an alternative locomotion option where the player smoothly animates to the teleport destination rather than instantly appearing there. This follows the Oculus best practices for VR locomotion.
Changes:
- Added
teleportationMethodandpositionWarpDurationschema properties to configure position-warp behavior - Modified teleportation logic to animate position changes when position-warp is enabled
- Delayed rotation application until after position animation completes for position-warp
- Updated README documentation to describe the new properties
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/index.js | Added schema properties and conditional logic to animate position and defer rotation when position-warp method is enabled |
| README.md | Documented the two new configuration properties for position-warp teleportation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rig.setAttribute('animation', { | ||
| property: 'position', | ||
| to: `${newRigLocalPosition.x} 0 ${newRigLocalPosition.z}`, | ||
| easing: 'linear', | ||
| dur: this.data.positionWarpDuration | ||
| }); |
There was a problem hiding this comment.
Setting an animation attribute without removing or managing previous animations could cause conflicts if multiple teleportations are triggered rapidly. A-Frame's animation component will handle multiple animations on the same property by queuing or replacing them, but it's safer to explicitly remove the animation attribute after it completes or before starting a new one to avoid unexpected behavior. Consider adding an 'animationcomplete' event listener to clean up the animation attribute, or ensure that setting a new animation properly replaces the old one.
| rig.setAttribute('animation', { | |
| property: 'position', | |
| to: `${newRigLocalPosition.x} 0 ${newRigLocalPosition.z}`, | |
| easing: 'linear', | |
| dur: this.data.positionWarpDuration | |
| }); | |
| // Remove any existing animation to avoid conflicts when teleporting rapidly. | |
| rig.removeAttribute('animation') | |
| rig.setAttribute('animation', { | |
| property: 'position', | |
| to: `${newRigLocalPosition.x} 0 ${newRigLocalPosition.z}`, | |
| easing: 'linear', | |
| dur: this.data.positionWarpDuration | |
| }) |
| snapTurn: { default: true }, | ||
| rotateOnTeleport: { default: true } | ||
| rotateOnTeleport: { default: true }, | ||
| teleportationMethod: { default: ""}, |
There was a problem hiding this comment.
The schema property uses an empty string as the default value for teleportationMethod, but this is effectively checking for string equality with "position-warp". Consider using a more explicit default or using a oneOf constraint to validate acceptable values. Based on the codebase pattern seen in other schema properties like 'button' (line 44), this property should include a oneOf constraint to restrict it to valid values.
| teleportationMethod: { default: ""}, | |
| teleportationMethod: { default: 'position-warp', oneOf: ['position-warp'] }, |
| snapTurn: { default: true }, | ||
| rotateOnTeleport: { default: true } | ||
| rotateOnTeleport: { default: true }, | ||
| teleportationMethod: { default: ""}, |
There was a problem hiding this comment.
The teleportationMethod property is missing an explicit type declaration. Looking at the schema conventions in this codebase, string properties should include type: 'string' for clarity and consistency (e.g., see lines 54-56 which use type: 'selector'). While A-Frame may infer the type from the default value, being explicit improves code clarity and maintainability.
| | snapTurn | If left/right thumbstick axis performs a 45 deg rotation | true | | ||
| | rotateOnTeleport | Will rotate player on teleport, facing the direction where the arrow is pointing | true | | ||
| | teleportationMethod | Set value to `position-warp` to enable this teleportation option. | | | ||
| | positionWarpDuration | How long the position warp animation should be. | 1000 | |
There was a problem hiding this comment.
The documentation states "How long the position warp animation should be" with a default of 1000, but doesn't specify the unit (milliseconds). For consistency with the incrementalDrawMs property on line 46 which explicitly mentions "ms" in both the property name and description ("How long the ray animation should be | 500"), consider either: (1) adding "in milliseconds" to the description, or (2) renaming the property to positionWarpDurationMs to match the naming convention.
| if (this.data.teleportationMethod === "position-warp") { | ||
| rig.setAttribute('animation', { | ||
| property: 'position', | ||
| to: `${newRigLocalPosition.x} 0 ${newRigLocalPosition.z}`, |
There was a problem hiding this comment.
The Y-coordinate is hardcoded to 0 in the animation target, but this ignores the Y-component from newRigLocalPosition.y. This will cause the rig to always animate to Y=0 regardless of the actual target position's Y-coordinate, which could result in the player ending up at the wrong height. The animation should use the full position vector including the Y-component.
| to: `${newRigLocalPosition.x} 0 ${newRigLocalPosition.z}`, | |
| to: `${newRigLocalPosition.x} ${newRigLocalPosition.y} ${newRigLocalPosition.z}`, |
| setTimeout(() => { | ||
| this.cameraRig.object3D.setRotationFromQuaternion(this.teleportOriginQuaternion) |
There was a problem hiding this comment.
The setTimeout callback is not stored or cleaned up, which could cause issues if the component is removed or another teleportation happens before the timeout completes. If a user rapidly triggers multiple teleportations, multiple setTimeout callbacks will queue up, potentially causing the rotation to be set incorrectly. Consider storing the timeout ID in an instance variable and clearing it before setting a new one, or at the very least clearing it in the remove() lifecycle method.
| setTimeout(() => { | |
| this.cameraRig.object3D.setRotationFromQuaternion(this.teleportOriginQuaternion) | |
| if (this.rotationTimeoutId) { | |
| clearTimeout(this.rotationTimeoutId) | |
| this.rotationTimeoutId = null | |
| } | |
| this.rotationTimeoutId = setTimeout(() => { | |
| this.cameraRig.object3D.setRotationFromQuaternion(this.teleportOriginQuaternion) | |
| this.rotationTimeoutId = null |
Hi Jure,
As anticipated over X (Twitter), this PR implements a simple edit to your component to add the position warp option.
Reference on the Oculus Techniques and Best Practices:
https://developer.oculus.com/resources/locomotion-design-turns-teleportation/#position-warp
I added the
positionWarpDurationproperty to the component's schema so the users can tweak it as they want or need. I initially set its default value to 0.5 sec, but after using this locomotion method in immersive mode for a few minutes, I changed it to 1 sec as it provides more comfort.NOTE: rather than a fixed speed, the fixed animation duration is an intended design choice: the resulting variable movement speed (proportional to the distance of the selected destinations) is indeed what makes this locomotion method feel natural, producing the result of "walking" to closer destinations and "running" to the ones farther away from the initial position of the user. This is, at least, what I wanted and needed to achieve in my prototype for an in-house project 🙂