WIP: FBX: Fix #84111: AssertionError importing shapekeys #104910

Mikhail Matrosov · 2023-09-23T22:56:39+02:00

Mikhail Matrosov commented

2023-09-23 22:56:39 +02:00

First-time contributor

In the bugreport an example file creates millions of duplicated shapekeys. This change avoids it by storing only unique keys in a dictionary.

Mikhail Matrosov added 1 commit 2023-09-23 22:56:42 +02:00

6b5acc2aeb FBX: Fix #104909 : AssertionError importing shapekeys

Mikhail Matrosov changed title from ~~FBX: Fix #104909: AssertionError importing shapekeys~~ to FBX: Fix #84111: AssertionError importing shapekeys

2023-09-27 13:38:51 +02:00

Pratik Borhade requested review from Bastien Montagne 2023-10-05 11:48:37 +02:00

Bastien Montagne requested review from Thomas Barlow 2023-10-05 14:48:03 +02:00

Bastien Montagne commented

2023-10-05 14:51:43 +02:00

From my limited understanding of the situation patch looks good, thanks!

I'd rather let @Mysteryem check on it though, since they've been working extensively in FBX refactors lately, and afaik are already aware of that issue (see #104698)?

Also, please fill in proper commit message and PR description (commit message is even empty, besides the title?).

From my limited understanding of the situation patch looks good, thanks! I'd rather let @Mysteryem check on it though, since they've been working extensively in FBX refactors lately, and afaik are already aware of that issue (see #104698)? Also, please fill in proper commit message and PR description (commit message is even empty, besides the title?).

Thomas Barlow commented

2023-10-05 15:46:10 +02:00

There are two separate changes in this patch, one that prevents importing duplicate shape keys from crappy Unreal exported FBX files with duplicate connections and one that attempts to avoid the AssertionError when importing FBX with in-between shapes. I think these changes should be separate PRs. Notably, the PR description is about one issue and the PR title is about the other.

Preventing import of duplicate shape keys

I think this is something we could do, but I think it should be noted in a comment that duplicate connections causing the same shape keys to be imported many times is not something we expect from FBX files. Perhaps linking back to the #104909 issue that has a .fbx from Unreal that has this issue.

Avoiding AssertionError when FBX has in-between shapes

I don't think it's a good idea to ignore the extra shapes assigned to each imported shape key without at least some warning of data loss. I would rather see the issue of handling in-between shapes be discussed at #104698 and either a temporary or full solution be decided on there, especially because the issue is more complicated due to it also affecting how Blender imports/exports shape key vertex groups.

There are two separate changes in this patch, one that prevents importing duplicate shape keys from crappy Unreal exported FBX files with duplicate connections and one that attempts to avoid the AssertionError when importing FBX with in-between shapes. I think these changes should be separate PRs. Notably, the PR description is about one issue and the PR title is about the other. ### Preventing import of duplicate shape keys I think this is something we could do, but I think it should be noted in a comment that duplicate connections causing the same shape keys to be imported many times is not something we expect from FBX files. Perhaps linking back to the #104909 issue that has a .fbx from Unreal that has this issue. ### Avoiding AssertionError when FBX has in-between shapes I don't think it's a good idea to ignore the extra shapes assigned to each imported shape key without at least some warning of data loss. I would rather see the issue of handling in-between shapes be discussed at #104698 and either a temporary or full solution be decided on there, especially because the issue is more complicated due to it also affecting how Blender imports/exports shape key vertex groups.

Mikhail Matrosov force-pushed main from 3a6f098668 to 39b507610d

2023-10-07 22:41:53 +02:00

Compare

Mikhail Matrosov referenced this pull request

2023-10-08 01:16:22 +02:00

WIP: FBX: Preventing import of duplicate shape keys #104939

Thomas Barlow commented

2023-10-09 04:22:18 +02:00

I think that to properly fix this issue we'll need to:

First fix importing duplicate connections within the chain of connections from Shape to Mesh so that the correct number of Shapes per BlendShapeChannel are imported (#104939 (comment)).
Count the number of Shapes assigned to each BlendShapeChannel and pass that to blen_read_shapes.
Print a warning to the System Console if any BlendShapeChannel has more than one Shape because multiple Shapes per Shape Key is not supported. The best we can realistically do for now is either to import each Shape as a separate Shape Key (what I'm intending) or only import the last Shape (the Shape with the largest value in FullWeights).
When parsing the FullWeights array, if the number of Shapes assigned to the BlendShapeChannel is more than 1 or the number of FullWeights does not equal the number of Indices, completely skip trying to parse FullWeights as Vertex Group weights and only check that there is at least as many FullWeights as there are Shapes assigned to the BlendShapeChannel.

All of these steps combined should result in any 'in-between shapes'/'progressive blend' BlendShapeChannels importing all their Shapes as separate Shape Keys. I think it might be a good idea to also include the name of the BlendShapeChannel in the name of these Shape Keys to make it clear they belong together, ending up as something like f"{channel_name}|{shape_name}".

I can work on this if you want.

I think that to properly fix this issue we'll need to: 1) First fix importing duplicate connections within the chain of connections from Shape to Mesh so that the correct number of Shapes per BlendShapeChannel are imported (https://projects.blender.org/blender/blender-addons/pulls/104939#issuecomment-1040277). 2) Count the number of Shapes assigned to each BlendShapeChannel and pass that to `blen_read_shapes`. 3) Print a warning to the System Console if any BlendShapeChannel has more than one Shape because multiple Shapes per Shape Key is not supported. The best we can realistically do for now is either to import each Shape as a separate Shape Key (what I'm intending) or only import the last Shape (the Shape with the largest value in FullWeights). 4) When parsing the FullWeights array, if the number of Shapes assigned to the BlendShapeChannel is more than 1 or the number of FullWeights does not equal the number of Indices, completely skip trying to parse FullWeights as Vertex Group weights and only check that there is at least as many FullWeights as there are Shapes assigned to the BlendShapeChannel. All of these steps combined should result in any 'in-between shapes'/'progressive blend' BlendShapeChannels importing all their Shapes as separate Shape Keys. I think it might be a good idea to also include the name of the BlendShapeChannel in the name of these Shape Keys to make it clear they belong together, ending up as something like `f"{channel_name}|{shape_name}"`. I can work on this if you want.

Mikhail Matrosov commented

2023-10-09 11:24:26 +02:00

First-time contributor

To me it's not clear if untangling this is worth the time, since such files seem to be produced only by Unreal, and the result is going to be exactly the same (channels refer to same shapekeys), but @Mysteryem if you can write the patch that would parse this with correct assumptions, that would be terrific. Same is #104939.

I'll mark both PRs as WIP, and when you post your PRs, I'll retract mine. Ping @mont29

To me it's not clear if untangling this is worth the time, since such files seem to be produced only by Unreal, and the result is going to be exactly the same (channels refer to same shapekeys), but @Mysteryem if you can write the patch that would parse this with correct assumptions, that would be terrific. Same is #104939. I'll mark both PRs as WIP, and when you post your PRs, I'll retract mine. Ping @mont29

Mikhail Matrosov changed title from ~~FBX: Fix #84111: AssertionError importing shapekeys~~ to WIP: FBX: Fix #84111: AssertionError importing shapekeys

2023-10-09 11:34:56 +02:00

Thomas Barlow commented

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

I've merged !104956 into main which fixes this specific case of Unreal exported FBX resulting in the same error as #84111. Thank you for your report and patience.

I've merged !104956 into `main` which fixes this specific case of Unreal exported FBX resulting in the same error as #84111. Thank you for your report and patience.

Thomas Barlow closed this pull request

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

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: FBX: Fix #84111: AssertionError importing shapekeys #104910

Preventing import of duplicate shape keys

Avoiding AssertionError when FBX has in-between shapes

Pull request closed