BufferGeometry: Implemented morphTargetsRelative #17649
Conversation
|
This would make implementation of morphtargets in the |
|
I'd like to do some performance profiling to quantify the difference, but my hunch is that this will be a significant parse time improvement for glTF models relying on morph targets, such as https://mrdoob.github.io/rome-gltf/. |
|
Nice update. I'm sure this change simplifies the glTF loader/exporter and reduces memory consumption. I haven't looked into the implementation in the detail yet but I guess serialization/deserialization ( |
|
I missed the BufferGeometryLoader somehow - will fix! |
|
Some performance numbers for context. Testing against the glTF files in this folder, all of which contain morph targets, I found a couple interesting things. First, parsing time was actually dominated by the embedded Data URIs in the With the slower A couple charts, showing the 7 models tested 3x each. Time spent processing morph targets is indicated in red. Y axis is milliseconds parsing, excluding network time.
The important chart here is the one in the bottom left – I'd take that as a pretty good indicator that this makes a measurable difference on parsing time of real models.
|
|
I've updated the change to fix a few stray examples / code that slipped my initial notice, and also fix the computeBoundingBox/Sphere functions to use precise (I think) formulas. @donmccurdy Thanks for gathering the numbers! |
|
@mrdoob per #17649 (comment), this would make GLTFLoader parsing more efficient. What do you think about (a) whether to add the property, and (b) how it should be located and named? |
|
FWIW while I'm not attached to the naming I think the property should be on BufferGeometry - this is because otherwise the data stored in BufferGeometry.morphAttributes becomes impossible to interpret outside of context. For example, if the property lived on the Material, changing materials would break the morphed shape visually if the value isn't fixed up. With this interface, BufferGeometry is "portable" - it can be migrated between objects / meshes / materials without losing the semantics. |
|
Oh and also given that both glTF and FBX loaders use relative data it seems that relative data is an important use case. Some formats like COLLADA support both - interestingly, if you look at COLLADA spec, they use the exact same formulation as this change:
|
This adds the attribute and the necessary plumbing for various JS functions; there's no support for specifying this for the shader to consume yet.
The shader code is now a bit simpler and relies on the precomputed base influence value; JS code sets it up appropriately.
This uses the newly added functionality for loading morph target data in GLTF loader; there's no need to clone attribute data anymore.
On further reflection it shouldn't be necessary - it's probably better to maintain "exact" match between the shader code and CPU-side code that may use (pos - morph) * w directly.
The code in GLTF exporter is probably suboptimal - it's not clear that we need to clone the attribute if it's already relative.
Also fix toNonIndexed to copy the attribute
expandByVector is not correct to call here; if a morph target has a min of [1,1,1], it should *not* change the box.min but it should expand box.max by 1. To avoid doing per-component math we use addVectors to get the corners of the box, and use expandByPoint to union the box with the resulting point.
|
I've rebased this against dev because of a trivial conflict with BufferGeometry.d.ts; should be cleanly mergeable again. @mrdoob please let me know if there's anything else to be done here, as per above this seems beneficial and complete. I could rename morphTargetsRelative to something like morphRelative if that seems more appropriate. |
|
I've added two minor comments. Apart from that, the change looks good! |
Fix default value in documentation
There was a problem hiding this comment.
I'd support making a change like this to allow relative morph targets, especially given #17608 (comment). I'm open to other APIs for it, but this PR seems like the most direct approach from where the API is today.
|
@mrdoob please let me know if any further changes are required |
|
Sorry for the delay. In your experience, do game engines usually support both modes? Or is it usually only relative mode? (Tempted of only supporting relative mode and have a method to convert absolute to relative to help with breakage) Also, does it only apply to vertex positions? Or also to normals and colors somehow? |
|
@mrdoob #17608 (comment) may help (in case you've missed) |
|
Yeah so resummarizing - glTF and FBX work with relative data; COLLADA allows both; keyframed animations from MD2 use absolute data. I think there are two use cases for morph targets - one prevalent one, for blend shapes - morphing between multiple variants of the same mesh where usually only part of the mesh gets deformed and it’s common to apply many shapes with fractional coefficients - think facial expressions (where you always overlay corrections on top of base pose). And one less common one, for morphing between substantially different poses - e.g. if you want to morph between meshes with unrelated structure relative data doesn’t make as much sense. My overall feeling is that relative targets are the most important, and absolute targets could be either not supported or emulated through absolute targets. However, this is a large breaking change. So I would recommend doing this gradually:
The cost of supporting both isn’t very large so I would err on the side of caution, but I don’t know the philosophy of Three.JS wrt breaking changes. |
|
Oh and the morphs are usually relative or absolute for all attributes, including normals. I have not seen morph targets used for other attributes - it’s certainly possible in theory but I don’t think it’s very common. The general form here really is a full weighted sum of all targets, with a custom weight for base target. If the fact that there are many places that have to check the relative state is a concern, I can refactor this to use the general weighting math from the shader on JavaScript side? That would remove most of the conditions. I would prefer to do it in a separate PR but I can do it as part of this one if necessary. |
|
Thanks for summarising! I think I vote for making this a breaking change: support only relative morphtargets and adding a method that converts absolute morphattributes to relative. |
|
Do you mean to do this in one step? This also means that the three.js release with this change will silently break components such as GLTFLoader if they aren't using the version from this change which seems unsatisfying. The original intent for this PR was to make a change that is safe to deploy, so that deprecation can be done separately. |
We normally do not support this scenario anyway. Check out the following notes from the docs (last section):
|
Yeah
Yes, but it also introduces new API ( Seems easier to have a breaking change and point people to a method that fixes it. |
|
We can do it in two steps actually. |
|
Sorry I’ve been short on time the last few days. Two steps sound good - that’s my preference anyway. It is my understanding that JSON export isn’t a long term compatibility target either? Effectively when removing the option, we'll start treating existing JSON exports as relative instead of absolute. |
|
Better to merge it at the beginning of |
mrdoob
changed the title
|
Thanks! |
Any chance |
|
Thank you @zeux! :) |
|
@mrdoob Are you still considering removing the If so I'm not sure that it's worth spending the time to update it. |
|
@looeee well... removing code from it will help make it less intimidating and maybe that way we'll get more eyes on it. |
Well, the code is now about 3 lines shorter so let's see how that goes 😁 |
This change implements an addition to core BufferGeometry class, morphTargetsRelative.
By default, morphTargetsRelative is false which means that the morph targets contain absolute position/normal data. Setting this boolean to true forces three.js to interpret the target data as deltas from the base.
To implement rendering support, instead of duplicating shader code, the base mesh gets a weight that is set to 1 for relative targets and 1 - sum(weights) for absolute targets. The math ends up matching the original math with this:
base + w1 * (target1 - base) + w2 * (target2 - base) = base - w1 * base - w2 * base + w1 * target1 + w2 * target2 = base * (1 - w1 - w2) + w1 * target1 + w2 * target2
In JavaScript code we need to occasionally check the morphTargetsRelative value to determine the correct action.
As a result, glTF loader (in theory FBX loader as well, but I'm not an expert in FBX loader code so I didn't want to change this in this PR) doesn't need to duplicate morph target data. This fixes some issues with morph target format encodings, saves memory in some cases, and removes unnecessary data copying from the loading step.
I've tested this change on built-in examples that exercise morph target support as well as some custom glTF files processed by gltfpack. Also note that the change is backwards-compatible in that using an older version of GLTFLoader.js works as expected (by using the absolute morph target mode).
Fixes #17608.