FBX IO: Fix error importing BlendShapeChannels with extraneous FullWeights #104956

Thomas Barlow · 2023-10-16T06:38:15+02:00

Thomas Barlow commented

2023-10-16 06:38:15 +02:00

Fixes a rare cause of #84111, but does not fix the main issue.

Aside from Shape Keys with Vertex Groups exported by Blender, rarely, a
BlendShapeChannel can have more FullWeights than Shapes assigned to it,
which would fail an assertion that the number of FullWeights equals the
number of vertices moved by the Shape with an allowed exception of when
there is only a single FullWeight.

This assertion was not correct because each FullWeight is the
deformation percentage of the BlendShapeChannel that fully activates
each Shape assigned to the BlendShapeChannel.

This patch keeps track of the Shapes assigned to each BlendShapeChannel
so that the number of FullWeights can be correctly compared against the
number of Shapes. FBX appears to ignore any excess FullWeights, so the
updated assertion allows this too.

Blender exported Shape Keys with Vertex Groups are then handled
separately by initially checking if the number of FullWeights equals the
number of vertices moved by the imported Shape.

The main cause of #84111 is when there is more than one Shape assigned
to a BlendShapeChannel. This is still not supported because it requires
larger changes and allowing it now would cause more issues. Because the
assertion is no longer sufficient to cause an error when there is more
than one Shape, an explicitly thrown RuntimeError has been added to
fulfil the same role.

Combined with !104954, this fixes the import of the .fbx files in #104909.

Fixes a rare cause of #84111, but does not fix the main issue. Aside from Shape Keys with Vertex Groups exported by Blender, rarely, a BlendShapeChannel can have more FullWeights than Shapes assigned to it, which would fail an assertion that the number of FullWeights equals the number of vertices moved by the Shape with an allowed exception of when there is only a single FullWeight. This assertion was not correct because each FullWeight is the deformation percentage of the BlendShapeChannel that fully activates each Shape assigned to the BlendShapeChannel. This patch keeps track of the Shapes assigned to each BlendShapeChannel so that the number of FullWeights can be correctly compared against the number of Shapes. FBX appears to ignore any excess FullWeights, so the updated assertion allows this too. Blender exported Shape Keys with Vertex Groups are then handled separately by initially checking if the number of FullWeights equals the number of vertices moved by the imported Shape. The main cause of #84111 is when there is more than one Shape assigned to a BlendShapeChannel. This is still not supported because it requires larger changes and allowing it now would cause more issues. Because the assertion is no longer sufficient to cause an error when there is more than one Shape, an explicitly thrown RuntimeError has been added to fulfil the same role. --- Combined with !104954, this fixes the import of the .fbx files in #104909.

Thomas Barlow added 1 commit 2023-10-16 06:38:18 +02:00

Fix error importing BlendShapeChannels with extraneous FullWeights 4930034304

Fixes a rare cause of #84111, but does not fix the main issue.

Aside from Shape Keys with Vertex Groups exported by Blender, rarely, a
BlendShapeChannel can have more FullWeights than Shapes assigned to it,
which would fail an assertion that the number of FullWeights equals the
number of vertices moved by the Shape with an allowed exception of when
there is only a single FullWeight.

This assertion was not correct because each FullWeight is the
deformation percentage of the BlendShapeChannel that fully activates
each Shape assigned to the BlendShapeChannel.

This patch keeps track of the Shapes assigned to each BlendShapeChannel
so that the number of FullWeights can be correctly compared against the
number of Shapes. FBX appears to ignore any excess FullWeights, so the
updated assertion allows this too.

Blender exported Shape Keys with Vertex Groups are then handled
separately by initially checking if the number of FullWeights equals the
number of vertices moved by the imported Shape.

The main cause of #84111 is when there is more than one Shape assigned
to a BlendShapeChannel. This is still not supported because it requires
larger changes and allowing it now would cause more issues. Because the
assertion is no longer sufficient to cause an error when there is more
than one Shape, an explicitly thrown RuntimeException has been added to
fulfil the same role.

Thomas Barlow reviewed 2023-10-16 07:04:23 +02:00

io_scene_fbx/import_fbx.py Outdated

						
				@ -1965,0 +1966,4 @@

				        # in the FullWeights array.

				        # XXX - It's possible, though very rare, to get a false positive here and create a Vertex Group when we

				        #       shouldn't. This should only happen if there are extraneous FullWeights or there is a single FullWeight

				        #       and its value is not 100.0.

Thomas Barlow commented

2023-10-16 07:04:23 +02:00

I'm not very happy about this possible false positive.
It should be noted that most software will truncate the FullWeights array to the same length as the number of Shapes, so the Blender Vertex Group weights will not often be preserved. External software can also read the Vertex Group weights as FullWeights values which can have unintended effects.

A few ideas:

Add a new import option that controls whether importing FullWeights as Vertex Group weights is enabled.
- FullWeights as Vertex Groups is a really niche feature that is only really relevant for importing Blender exported .fbx back into Blender, so adding an importer option sounds like overkill.
Only import as Vertex Group weights when the the creator of the .fbx file is Blender.
- If there is external software that has added support for propagating Blender Vertex Group weights, this could break support
Entirely remove support for exporting/importing FullWeights as Vertex Group weights
- Just because it's a really niche feature doesn't mean nobody has been using it
- While there's not much code to change, it's probably too big of a change for 4.0 at this point.

I'm not very happy about this possible false positive. It should be noted that most software will truncate the FullWeights array to the same length as the number of Shapes, so the Blender Vertex Group weights will not often be preserved. External software can also read the Vertex Group weights as FullWeights values which can have unintended effects. A few ideas: - Add a new import option that controls whether importing FullWeights as Vertex Group weights is enabled. - FullWeights as Vertex Groups is a really niche feature that is only really relevant for importing Blender exported .fbx back into Blender, so adding an importer option sounds like overkill. - Only import as Vertex Group weights when the the creator of the .fbx file is Blender. - If there is external software that has added support for propagating Blender Vertex Group weights, this could break support - Entirely remove support for exporting/importing FullWeights as Vertex Group weights - Just because it's a really niche feature doesn't mean nobody has been using it - While there's not much code to change, it's probably too big of a change for 4.0 at this point.

Thomas Barlow commented

2023-10-20 04:33:06 +02:00

I've added an extra check that all the FullWeights are within the [0.0, 100.0] range, which prevents the false positives when importing the .fbx files in #104909

I've added an extra check that all the FullWeights are within the [0.0, 100.0] range, which prevents the false positives when importing the .fbx files in https://projects.blender.org/blender/blender-addons/issues/104909

Thomas Barlow reviewed 2023-10-16 07:17:24 +02:00

io_scene_fbx/import_fbx.py Outdated

						
				@ -1935,0 +1935,4 @@

				        num_shapes_assigned_to_channel = len(shapes_assigned_to_channel)

				        if num_shapes_assigned_to_channel > 1:

				            # See bug report #84111 and design task #104698.

				            raise RuntimeError("FBX in-between Shapes are not currently supported")

Thomas Barlow commented

2023-10-16 07:17:24 +02:00

The short of it is that while we could allow each in-between shape to be imported as a separate shape key right now, any imported animations of the DeformPercent of the BlendShapeChannel the Shapes belong to will not work correctly and the FullWeights value for each Shape will be lost. I think the minimum for in-between shapes support would be importing them and their animations correctly.

Maybe the error message should include text that makes it clear this is a known issue and doesn't need to be reported.

The short of it is that while we could allow each in-between shape to be imported as a separate shape key right now, any imported animations of the DeformPercent of the BlendShapeChannel the Shapes belong to will not work correctly and the FullWeights value for each Shape will be lost. I think the minimum for in-between shapes support would be importing them and their animations correctly. Maybe the error message should include text that makes it clear this is a known issue and doesn't need to be reported.

Thomas Barlow reviewed 2023-10-16 07:20:35 +02:00

io_scene_fbx/import_fbx.py Outdated

						
				@ -3485,1 +3507,4 @@

				                    continue

				                # Track the Shapes connected to each BlendShapeChannel. Duplicates are ignored.

				                shapes_assigned_to_channel = blend_shape_channel_to_shapes.setdefault(bc_uuid, set())

				                shapes_assigned_to_channel.add(s_uuid)

Thomas Barlow commented

2023-10-16 07:20:35 +02:00

When merging this with !104954, the sets can be replaced with lists because only unique Shapes will be found.

Mysteryem marked this conversation as resolved

Thomas Barlow added 3 commits 2023-10-20 04:22:30 +02:00

Merge branch 'blender-v4.0-release' into fbx_fix_extra_fullweights_error 9b30b7834c

# Conflicts:
#	io_scene_fbx/import_fbx.py

Add more checks for determining whether to import FullWeights as vertex weights 2cefb6ca1f

This should eliminate the case of FBX from Unreal with extraneous
FullWeights because those always seem to be negative for some reason.

An alternative check that could be made for these extraneous Unreal
FullWeights is that their absolute values seem to always be far smaller
than the smallest single precision non-zero float (vertex weights are
single-precision). The smallest subnormal single-precision float is
1e-45, but these extraneous FullWeights are usually around 1e-200.
Though if there are systems running Blender where single precision float
is 64 bits, this check wouldn't work.

Include bug report comment on the same line as the exception 0d56171681

This will make it show up in tracebacks.

Thomas Barlow added 1 commit 2023-10-20 04:28:22 +02:00

Missed comment update in the merge 28e5575617

Thomas Barlow changed title from ~~WIP: Fix error importing FBX BlendShapeChannels with extraneous FullWeights~~ to Fix error importing FBX BlendShapeChannels with extraneous FullWeights

2023-10-20 04:28:52 +02:00

Thomas Barlow requested review from Bastien Montagne 2023-10-20 04:34:26 +02:00

Bastien Montagne approved these changes 2023-10-30 16:48:44 +01:00

Bastien Montagne left a comment

Code LGTM.

Not sure I would merge this in 4.0 at this point though... Feels a bit risky for a fairly rare issue? But would let that decision to you ultimately.

(And sorry for the delayed review, BCon23 was eating a bit too much time in the past month).

Code LGTM. Not sure I would merge this in 4.0 at this point though... Feels a bit risky for a fairly rare issue? But would let that decision to you ultimately. (And sorry for the delayed review, BCon23 was eating a bit too much time in the past month).

Thomas Barlow commented

2023-10-31 05:24:29 +01:00

I'll retarget this to the main branch since the 4.0 release is so close now.

Thomas Barlow changed title from ~~Fix error importing FBX BlendShapeChannels with extraneous FullWeights~~ to FBX IO: Fix error importing BlendShapeChannels with extraneous FullWeights

2023-11-05 09:53:13 +01:00

Thomas Barlow force-pushed fbx_fix_extra_fullweights_error from 28e5575617 to 3d823efc57

2023-11-05 09:58:25 +01:00

Compare

Thomas Barlow added 6 commits 2023-11-05 09:58:58 +01:00

Fix error importing BlendShapeChannels with extraneous FullWeights 9ff5b81433

Fixes a rare cause of #84111, but does not fix the main issue.

Aside from Shape Keys with Vertex Groups exported by Blender, rarely, a
BlendShapeChannel can have more FullWeights than Shapes assigned to it,
which would fail an assertion that the number of FullWeights equals the
number of vertices moved by the Shape with an allowed exception of when
there is only a single FullWeight.

This assertion was not correct because each FullWeight is the
deformation percentage of the BlendShapeChannel that fully activates
each Shape assigned to the BlendShapeChannel.

This patch keeps track of the Shapes assigned to each BlendShapeChannel
so that the number of FullWeights can be correctly compared against the
number of Shapes. FBX appears to ignore any excess FullWeights, so the
updated assertion allows this too.

Blender exported Shape Keys with Vertex Groups are then handled
separately by initially checking if the number of FullWeights equals the
number of vertices moved by the imported Shape.

The main cause of #84111 is when there is more than one Shape assigned
to a BlendShapeChannel. This is still not supported because it requires
larger changes and allowing it now would cause more issues. Because the
assertion is no longer sufficient to cause an error when there is more
than one Shape, an explicitly thrown RuntimeException has been added to
fulfil the same role.

Add more checks for determining whether to import FullWeights as vertex weights 1ea4d1f110

This should eliminate the case of FBX from Unreal with extraneous
FullWeights because those always seem to be negative for some reason.

An alternative check that could be made for these extraneous Unreal
FullWeights is that their absolute values seem to always be far smaller
than the smallest single precision non-zero float (vertex weights are
single-precision). The smallest subnormal single-precision float is
1e-45, but these extraneous FullWeights are usually around 1e-200.
Though if there are systems running Blender where single precision float
is 64 bits, this check wouldn't work.

Include bug report comment on the same line as the exception c10fea6546

This will make it show up in tracebacks.

Missed comment update in the merge f1cb952249

Fix bad merge in rebase 33e1373492

Increase FBX version 4bd757f0db

Thomas Barlow changed title from ~~FBX IO: Fix error importing BlendShapeChannels with extraneous FullWeights~~ to FBX IO: Fix error importing BlendShapeChannels with extraneous FullWeights

2023-11-05 09:59:27 +01:00

Thomas Barlow changed target branch from blender-v4.0-release to main

2023-11-05 09:59:27 +01:00

Thomas Barlow merged commit 0ceb884d1f into main

2023-11-05 10:16:56 +01:00

Thomas Barlow referenced this issue from a commit

2023-11-05 10:16:56 +01:00

FBX IO: Fix error importing BlendShapeChannels with extraneous FullWeights

Thomas Barlow deleted branch fbx_fix_extra_fullweights_error

2023-11-05 10:16:57 +01:00

Thomas Barlow referenced this pull request

2023-11-05 10:37:18 +01:00