Add scale option to gltf model plus #729
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
robertlong
approved these changes
Nov 13, 2018
src/components/gltf-model-plus.js
Outdated
| x: node.scale.x, | ||
| y: node.scale.y, | ||
| z: node.scale.z | ||
| x: node.scale.x * (scale !== undefined ? scale : 1), |
There was a problem hiding this comment.
Should we make scale a vector rather than a scalar value so that the non-uniform scale of persisted models can be set?
There was a problem hiding this comment.
Or maybe we just don't set a scale property and use the normal transform scale to deserialize that data?
netpro2k
reviewed
Nov 13, 2018
src/components/gltf-model-plus.js
Outdated
| x: node.scale.x, | ||
| y: node.scale.y, | ||
| z: node.scale.z | ||
| x: node.scale.x * (scale !== undefined ? scale : 1), |
There was a problem hiding this comment.
At first glance I thought this was going to scale all the nodes, but then i realized its just not passed through on recursion, probably ok got unexpected at first
src/components/media-loader.js
Outdated
| @@ -195,7 +195,8 @@ AFRAME.registerComponent("media-loader", { | |||
| this.el.setAttribute("gltf-model-plus", { | |||
| src: accessibleUrl, | |||
| contentType: contentType, | |||
| inflate: true | |||
| inflate: true, | |||
| scale: 0.0001 | |||
There was a problem hiding this comment.
So.. its a bit weird that we are going to have this scale property dangling around that actually doesnt reflect the current scale of anything. Maybe just renaming it to initialScale or something would help? but still feels pretty weird that we have this but then also modify the meshes scale later to reverse this.
There was a problem hiding this comment.
Yeah... to be honest, using this scale property to fix this seems like a bit of a hack. Can we do this scaling when the model is loaded but before it's added to the scene? I don't like adding more events, but I also don't really like pushing the complexity into the schema when there is already a scale component.
There was a problem hiding this comment.
we perform the initial sizing and scale in response to the media-loaded event, which happens > 1 frame after the gltf model has been spawned into the scene. if there's a straightforward way to resolve this issue that is cleaner than this approach then that sounds good to me, but my assumption was that this PR I wrote was the best way to do it given the way we deal with media
There was a problem hiding this comment.
i'm going to rename this to modelToWorldScale and push it tmw unless there are objections or suggestions on a cleaner solution
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Address the long standing bug where when you paste a GLTF model you see it at the original scale for one frame (which is potentially huge.) Allows setting a scale modifier on the gltf-model-plus node which will be multiplied against the root object node's scale, and sets that to a small value so the object starts out small (and then is animated to the proper scale.)