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

ufbx: Dataset comparison against FBX2glTF #90986

Closed
Tracked by #91519
bqqbarbhg opened this issue Apr 21, 2024 · 13 comments
Closed
Tracked by #91519

ufbx: Dataset comparison against FBX2glTF #90986

bqqbarbhg opened this issue Apr 21, 2024 · 13 comments

Comments

@bqqbarbhg
Copy link
Contributor

bqqbarbhg commented Apr 21, 2024

Tested versions

4.3.dev (2a757e4)

System information

Godot v4.3.dev (2a757e4) - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3080 (NVIDIA; 31.0.15.4633) - AMD Ryzen 9 3900X 12-Core Processor (24 Threads)

Issue description

I imported my private dataset (12532 FBX files) using both ufbx and FBX2glTF to validate the ufbx loader. I hacked the editor to import the file, focus the editor camera, and take a screenshot of both imported scenes. Then I manually reviewed each image pair for differences.

The bulk of the comparisons were in the following categories:

  • 11699 equal: Almost every model (~10800) had material differences, FBX2glTF tends to be a bit shinier. Animations were also slightly different in many files (~800) due to trimming working differently.
  • 236 having incorrect lights in FBX2glTF, fixed by ufbx
  • 86 missing skinned mesh in FBX2glTF, fixed by ufbx
  • 43 files had missing triangles in FBX2glTF, fixed by ufbx
  • 83 files seemed to have bad materials in ufbx
  • 73 files had worse rigs in ufbx
  • 16 files had node visibility/existence differences

In addition to some other various differences, there were 165 files with unique problems, including:

  • Mirrored meshes behaving inconsistently
  • Mismatched transforms in ufbx and FBX2glTF
  • Smooth normals in FBX2glTF and hard faces in ufbx
  • Significant material/light color differences
  • Vertex color alpha not being imported by ufbx importer
  • Some animations appearing as T-pose in ufbx

I don't think the specific issues are very urgent for 4.3, as many of them seem to be isolated bugs affecting only a small amount of files. However, some errors are quite systematic and would be a breaking change to fix later. I'd say the largest three differences that we might want to consider to fix before 4.3 are:

  • Material differences: This seems to be mostly how specular settings are converted to roughness/metallic. It's not clear which importer handles it better here, so fixing this would only matter if we care about compatibility with FBX2glTF, otherwise I'd say we can leave it as-is.
  • Visibility: It seems that in many cases FBX2glTF either does not import or hides (to be tested) nodes that have their FBX Visibility attribute set to false. Changing this would be a breaking change and we would need to decide if we want to follow FBX2glTF here.
  • Skeletons: A vast majority of skeletons are imported in an identical way by FBX2glTF and ufbx. However, there are many cases where ufbx seems to be strictly worse, see below.

Skeletons

Most skeletons are exactly equal between FBX2glTF and ufbx. However, there is some category of skeletons that get imported in a strange way using ufbx. One example case from the dataset I can share is Astronaut_Idle.zip:

FBX2glTF
W_ufbx_dataset_local_unity_Astronaut_Idle_gltf

ufbx
W_ufbx_dataset_local_unity_Astronaut_Idle_ufbx

As can be seen, the ufbx import has some strange bones protruding in what looks like orthogonal directions from the mesh, while the FBX2glTF has a clean skeleton internally. The same kind of artifact recurs in multiple files, and I assume it might have something to do with how the rest pose is set up w.r.t. the FBX PreRotation property.

Steps to take

As mentioned above, many of the encountered issues are semi-rare bugs which I wouldn't consider urgent. At the very least I would like to look into the skeleton issue as that will be very difficult to fix later-on, without breaking other meshes.

I'd like to have some input on what is the general consensus on what to do with the material and visibility differences, as those are not clear issues in the ufbx-based importer, but rather inconsistencies between the two importers.

Also let me know if you want to open separate issues for some of the sub-issues discovered here.

Steps to reproduce

Unfortunately, due to the private nature of the dataset, I cannot share many of the files causing the issues. Below are a few of the example files exhibiting the issues that I've got permission to share:

I do plan on going through all the ufbx-related issues myself. On the other hand, I probably won't have time to look at the FBX2glTF missing triangles issue.

Minimal reproduction project (MRP)

N/A

@fire
Copy link
Member

fire commented Apr 21, 2024

I can link to the algorithm for specular to metallic / roughness but I need a qualitative judgement like is it better looking.

@bqqbarbhg
Copy link
Contributor Author

I can link to the algorithm for specular to metallic / roughness but I need a qualitative judgement like is it better looking.

Yeah it's hard to say, I'd say the ufbx mapping looks better for most cases, but in some cases the FBX2glTF one looked better, but I haven't compared them to any objective reference (if one even exists). The ufbx mapping is based on what Blender does internally iirc.

@lyuma
Copy link
Contributor

lyuma commented Apr 21, 2024

Thank you, bqq for the extensive testing. This is really helpful info.

In terms of compatibility, the good news is we don't need to be 1-to-1 with FBX2glTF because we migrate existing projects to keep using FBX2glTF for existing files

Let's look at some of the specific issues.
Physical Lights in FBX2glTF... it is because gltf uses lux units I think

trimming working differently... I wonder if we should compare trimming off fbx2gltf vs trimming on ufbx. The trimming setting doesn't talk to the fbx library in fbx2gltf so it has no idea about start time.

Vertex color alpha is pretty important because a lot of people use vertex colors for VFX, like painting trees to have natural swaying motion. This is one of those things that feels like it should be easy to fix so I might look into it.

Animations in t-pose. Is this maybe another bug related to "remove immutable tracks" option? Does it happen without using bone map and with remove immutable disabled?

as for material differences, spec gloss itself is not a supported workflow in godot. We can try to do something to convert to pbr but I think we can call it unsupported. if you mean metallic specular, it is something that I would like to support but godot's default shader currently only supports grayscale for some reason so it won't look exactly the same.

visibility: the ufbx behavior is correct. The reason for the FBX2glTF behavior is due to what I believe to be a design error in the glTF spec that forbids exporting hidden meshes. I think the new behavior is an improvement in this case.

thanks for sharing some of the testcases.

@bqqbarbhg
Copy link
Contributor Author

Thanks for the input!

Let's look at some of the specific issues.
Physical Lights in FBX2glTF... it is because gltf uses lux units I think

Most light issues are caused by directional lights being incorrectly rotated in FBX2glTF as far as I can tell. In FBX the lights point along the node's negative Y axis and I'm not sure if this is taken into account in FBX2glTF.

trimming working differently... I wonder if we should compare trimming off fbx2gltf vs trimming on ufbx. The trimming setting doesn't talk to the fbx library in fbx2gltf so it has no idea about start time.

Yeah I knew this was going to be an issue going into the testing, I just hoped it wouldn't cause too many issues. In the end it was fine for most cases, I'll probably re-run the tests on models I tagged as mismatching in animation without trimming.

Vertex color alpha is pretty important because a lot of people use vertex colors for VFX, like painting trees to have natural swaying motion. This is one of those things that feels like it should be easy to fix so I might look into it.

Yeah shouldn't be too bad, I actually think the vertex color alpha is being imported already, but the materials are not tagged as transparent if it exists. So it uses a solid shader with alpha defined in the vertex color.

Animations in t-pose. Is this maybe another bug related to "remove immutable tracks" option? Does it happen without using bone map and with remove immutable disabled?

I'll need to look into those later, there's also a solid chance that it's choosing the wrong animation, as I have a heuristic for selecting the most interesting animation in the file, and there may be cases where it would accidentally select a T-pose.

visibility: the ufbx behavior is correct. The reason for the FBX2glTF behavior is due to what I believe to be a design error in the glTF spec that forbids exporting hidden meshes. I think the new behavior is an improvement in this case.

Good to know, I'm not sure if the visibility bit was being imported as visibility currently, but it should be quite straightforward to add if wanted.

as for material differences, spec gloss itself is not a supported workflow in godot. We can try to do something to convert to pbr but I think we can call it unsupported. if you mean metallic specular, it is something that I would like to support but godot's default shader currently only supports grayscale for some reason so it won't look exactly the same.

What I meant with material differences is that 99% of FBX materials are defined using a specular workflow. Both FBX2glTF and ufbx in Godot then translate this information into a metallic/roughness workflow:

FBX2glTF does the following transform:

auto getRoughness = [&](float shininess) { return sqrtf(2.0f / (2.0f + shininess)); };

In the ufbx importer, we use the roughness provided by ufbx by default:

material->set_roughness(float(fbx_material->pbr.roughness.value_real));

Which is calculated as (abridged):

static void ufbxi_mat_transform_unknown_shininess(ufbx_vec4 *v) {
    v->x = (ufbx_real)(1.0f - ufbx_sqrt(v->x) * (ufbx_real)0.1);
}

If I remember correctly, I designed this to be the inverse transform of Blender's roughness to FBX shininess mapping.

But these two transformations are quite different, and it's impossible to say which one is objectively better I think. Like I said earlier I think in most cases I prefer the ufbx transform due to it being less shiny, but in some cases the transform used by FBX2glTF looks better. Here's an example of what the larger material differences look in practice (the astronaut model in the issue itself has a slight material difference that is very common across all models):

FBX2glTF
W_ufbx_dataset_sketchfab_cartoon-character-for-game-rigged_fiverrr_packed0_diffuse_gltf

ufbx
W_ufbx_dataset_sketchfab_cartoon-character-for-game-rigged_fiverrr_packed0_diffuse_ufbx

So in general I think we should choose to either keep the ufbx interpretation, or change fbx_document.cpp to use the FBX2glTF (via mapping ufbx_material.fbx.shininess) if the material in question does not use roughness (and as mentioned, almost none do).

@fire
Copy link
Member

fire commented Apr 22, 2024

I am ok with the ufbx material intepretation to be honest.

@bqqbarbhg
Copy link
Contributor Author

Updated findings after implementing #91036.

Re-tested all FBX files that import a Skeleton3D, resulting in 2264 files:

  • 2135 equal: Again some with non-important material or light differences
  • 80 with broken skinned meshes in FBX2glTF, these seem to be caused by blend shapes
  • 25 meshes with relatively significant differences in skeletons, but this time no obvious issues. Mostly the differences are in the roll of the bones, probably caused by the bind pose data in the file.
  • 6 files with various ufbx issues for me to look at later:

@fire
Copy link
Member

fire commented May 3, 2024

Are you ok with splitting this issue into the 6 mentioned earlier and closing?

@bqqbarbhg
Copy link
Contributor Author

Are you ok with splitting this issue into the 6 mentioned earlier and closing?

Sure, there's a whole bunch of other issues not listed above. I'll evaluate the cases quickly before making any issues though as some of them are already fixed in ufbx and some may be faster to fix than to make an issue for.

@bqqbarbhg
Copy link
Contributor Author

Turns out the issues were mostly fixed or easily fixable:

There were still a bunch of various differences and some material issues, but I can add issues for those as I encounter them. Feel free to close this issue if you consider this enough.

@fire
Copy link
Member

fire commented May 3, 2024

I'll close this when #91526, #91528 and #91528 are merged.

@fire
Copy link
Member

fire commented May 3, 2024

I'm curious if there's any pattern in:

25 meshes with relatively significant differences in skeletons, but this time no obvious issues. Mostly the differences are in the roll of the bones, probably caused by the bind pose data in the file.

@bqqbarbhg
Copy link
Contributor Author

Probably just normal from the difference in bind pose and the initial pose in the file, minor compatibility issue but shouldn't be a huge problem.

@fire
Copy link
Member

fire commented May 7, 2024

I'll close this issue now that the three prs are merged.

@fire fire closed this as completed May 8, 2024
@akien-mga akien-mga added this to the 4.3 milestone May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants