Place media frame content in world space coordinates #4279
Conversation
src/systems/media-frames.js
Outdated
| @@ -250,8 +250,13 @@ AFRAME.registerComponent("media-frame", { | |||
|
|
|||
| snapObject(capturedEl) { | |||
| // TODO this assumes media frames are all in world space | |||
| capturedEl.object3D.position.copy(this.el.object3D.position); | |||
| capturedEl.object3D.rotation.copy(this.el.object3D.rotation); | |||
| const worldPosition = new THREE.Vector3(); | |||
There was a problem hiding this comment.
Not so bad since this isn't run every frame but we should avoid allocating these vectors every time, either by just pulling it out into the root of the file, or in a closure for this function alone. I will often just create a const tmpWorldPos = new THREE.Vector3() in a file to use for this sort of thing.
There was a problem hiding this comment.
Yeah, I thought that being non a tick function would be fine but it makes sense to avoid instancing if possible. Updated.
src/systems/media-frames.js
Outdated
| this.el.object3D.getWorldPosition(worldPosition); | ||
| capturedEl.object3D.position.copy(worldPosition); | ||
| const worldQuat = new THREE.Quaternion(); | ||
| this.el.object3D.updateWorldMatrix(true); |
There was a problem hiding this comment.
You should not need to manually call this, and in fact we don't want to because it will cause extra work. See https://github.com/mozilla/hubs/blob/master/src/utils/threejs-world-update.js#L22. Also worth reading through these rules and this file in general if you havent yet. Not following these rules is source of most of our hubs-specific threejs issues... I do hope we can get rid of it some day, but when we implemented it there was a big performance win, especially due to all the extra Object3Ds aframe introduces into the scene graph.
There was a problem hiding this comment.
Humm, I didn't know about that file. I've remove the updateWorldMatrix call.
src/systems/media-frames.js
Outdated
| @@ -250,8 +250,13 @@ AFRAME.registerComponent("media-frame", { | |||
|
|
|||
| snapObject(capturedEl) { | |||
| // TODO this assumes media frames are all in world space | |||
There was a problem hiding this comment.
This PR fixes this todo so we should remove it.
|
@netpro2k Updated. |
We are currently placing elements in
media-frames based on local coordinates so nestedmedia-frames don't place them correctly.Test scene: https://hubs.mozilla.com/scenes/4zXCrZE