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

Merged
Thomas Barlow merged 6 commits from Mysteryem/blender-addons:fbx_fix_extra_fullweights_error into main 2023-11-05 10:16:56 +01:00
Member

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
4930034304 Fix error importing BlendShapeChannels with extraneous FullWeights
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
@ -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.
Author
Member

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.
Author
Member

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
@ -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")
Author
Member

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
@ -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)
Author
Member

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

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
2cefb6ca1f Add more checks for determining whether to import FullWeights as vertex weights
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.
Thomas Barlow added 1 commit 2023-10-20 04:28:22 +02:00
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
Owner

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).
Author
Member

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

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
9ff5b81433 Fix error importing BlendShapeChannels with extraneous FullWeights
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.
1ea4d1f110 Add more checks for determining whether to import FullWeights as vertex weights
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.
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
Mysteryem 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 deleted branch fbx_fix_extra_fullweights_error 2023-11-05 10:16:57 +01:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender-addons#104956
No description provided.