Fix 3D models Media Frame scaling issues and add support for 3D preview #4655
Conversation
keianhzo
changed the title
keianhzo
changed the title
There was a problem hiding this comment.
lgtm. Thanks for getting this fixed up! If I had implemented this I would have been too lazy to support animated previews, but that's a nice touch :) - Made a few comments, mostly around the matrix world update contract. I think most of my comments are noop changes so no strong opinions on if you bother changing them if you have already done a bunch of testing.
| } | ||
|
|
||
| const DEBUG = qsTruthy("debug"); |
There was a problem hiding this comment.
Fine for this PR but we keep inventing this adhoc all over, we should unify on a good debug system
| clonedMesh.traverse(node => { | ||
| if (node.isMesh) { | ||
| if (node.material) { | ||
| node.material = node.material.clone(); |
There was a problem hiding this comment.
Was going to suggest cloning materials should be an option for cloneObject3D but you have to traverse again anyway to set the transparency...
src/systems/media-frames.js
Outdated
| clonedMesh.position.set(0, 0, 0); | ||
| clonedMesh.quaternion.identity(); | ||
| clonedMesh.matrixNeedsUpdate = true; | ||
| clonedMesh.updateMatrixWorld(true); |
There was a problem hiding this comment.
Been a long time since I looked at the world update patch but it looks like we prefer directly calling updateMatricies in our own code https://github.com/mozilla/hubs/blob/env-settings-fixes/src/utils/threejs-world-update.js#L119
There was a problem hiding this comment.
Though in this case it looks like setFromObject on Box3 ends up already updating matricies anyway... https://github.com/mrdoob/three.js/blob/master/src/math/Box3.js#L196 . Mostly just ends up being noops anyway since most things respect the dirty flags, but just trying to trace through.
Fixes #4089
This PR fixes a couple of issues with media frames and 3D object: