Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebGLMorphtargets: Allow changing number of morph targets #20845

Merged
merged 1 commit into from
Jun 30, 2021

Conversation

zach-capalbo
Copy link
Contributor

Adding a new morph target attribute to a mesh after it has been rendered at least once with a material with morphTargets set to true results in the following error during rendering:

three.js:12235 Uncaught TypeError: Cannot set property '0' of undefined
    at Object.update (three.js:12235)
    at WebGLRenderer.renderBufferDirect (three.js:18471)
    at renderObject (three.js:18802)
    at renderObjects (three.js:18785)
    at WebGLRenderer.render (three.js:18657)

The line at error is shown below (with context, from the chrome web tools source viewer):


			for (var _i2 = 0; _i2 < length; _i2++) {
				var influence = influences[_i2];
				influence[0] = _i2; // <------------- Exception occurs here
				influence[1] = objectInfluences[_i2];
			}

			influences.sort(absNumericalSort);

Based on my understanding of the code, this is because influences is populated once the first time that the geometry is rendered (i.e., when influences === undefined). When a new morph target is added, then influences.length no longer matches objectInfluences.length causing an issue during the iteration of:

		// Collect influences
		for ( let i = 0; i < length; i ++ ) {
			const influence = influences[ i ];

My proposal is, for simplicity's sake, to simply recreate the influence list when there's been a change in the length. It could be reasonable to add / remove elements from the list as well, but that seems to needlessly complicate things in my personal opinion.

Unfortunately, there doesn't seem to me to be any way to reset the influences list short of creating an entirely new geometry and replacing the old one. I'm still working on trying to find a workaround.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 7, 2020

When a new morph target is added,

Can you please describe your use case a bit?

I'm not sure about any side-effects of the additional check. This needs to be properly investigated. A live example that demonstrate the issue would be helpful.

@zach-capalbo
Copy link
Contributor Author

Use case: 3D model editor that allows users to vertex transformations to their models as morph targets and then export as GLB files. This tweet shows the feature I was implementing when I encountered this issue: https://twitter.com/zach_geek/status/1335765647066882050

Since you can add other kinds of attributes to a BufferGeometry (e.g., UV, normal, etc) without any problems, It seemed reasonable to me that you would be able to add morph attributes as well. When I couldn't find any documentation to suggest otherwise, I tracked it down and reported this issue.

I am able to work around it by cloning the existing geometry and adding the new morph targets before assigning the geometry to the mesh. However, this is less than ideal as it requires unnecessary data copying, and requires that references to the geometry be replaced throughout the application.

I agree that an investigation is worthwhile before accepting this change, and I'll set up a live demo as soon as I can. I'd also be happy to look into alternate solutions to this issue as well (e.g., some kind of morphTargetsNeedUpdate, or a console.warn when adding new morph targets, or even just a note in the documentation)

@zach-capalbo
Copy link
Contributor Author

Here is a live demo to demonstrate the issue:

https://glitch.com/edit/#!/dynamic-summer-tie?path=index.html%3A65%3A10

I haven't had a chance to put together a demo of the fix. I will work on that when I get a chance.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 28, 2020

@mrdoob This PR seems okay. It only resets the cached influence list if the number of morph targets has changed. Previously the code assumed the number of morph targets never changed so the cached list was created once and never updated.

@Mugen87 Mugen87 added this to the r125 milestone Jan 15, 2021
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@mrdoob mrdoob modified the milestones: r126, r127 Feb 23, 2021
@mrdoob mrdoob modified the milestones: r127, r128 Mar 30, 2021
@mrdoob mrdoob modified the milestones: r128, r129 Apr 23, 2021
@mrdoob mrdoob modified the milestones: r129, r130 May 27, 2021
@mrdoob mrdoob merged commit f936540 into mrdoob:dev Jun 30, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jun 30, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants