godot icon indicating copy to clipboard operation
godot copied to clipboard

Mesh importer tangents

Open fire opened this issue 1 year ago • 11 comments

Do not merge for 4.3.

https://github.com/godotengine/godot-proposals/issues/8696

  • [x] Blend shapes have special processing

See https://github.com/godotengine/godot/pull/83648 for a test case.

fire avatar Feb 14 '24 19:02 fire

Looking for review.

  1. Not sure if I should be forcing dummy tangents.
  2. The conditions for generating tangents is duplicated and different between importers previously. now it's unified.

fire avatar Apr 19 '24 19:04 fire

The linked PR appears to be for removing SurfaceTool from the import process, but this seems to increase reliance on the SurfaceTool instead of removing it. Is there a reason for the change in plan?

clayjohn avatar Apr 22 '24 22:04 clayjohn

The idea is we first consolidate all the uses of Surface Tool in one place in the scene importer from GLTF, Obj, FBX and Collada. Then, we can optimize that entire chunk of code as a whole by in-lining it etc. Like split the problem.

I read the proposal for this carefully, and although it said there is a desire to remove surface tool, it also said that duplication of code is an issue among the importers so there's two problems.

Edited:

When I posted this to @lyuma, he wasn't able to review the code because the in-lined version of the "SurfaceTool::generate_tangent" was too lengthy and there's a lack of reviewers on this.

fire avatar Apr 24 '24 13:04 fire

Conversation between Bqq and Lyuma on Tangents

  1. Broken Tangents and Blend Shape Normals in DCC: Lyuma expressed concern about the perception that some Digital Content Creation (DCC) tools may produce broken tangents or blend shape normals+tangents.

  2. Godot's Import Settings Limitations: According to Lyuma, Godot's import settings are too limited. Users have to choose between 'Ensure Tangents' or not. However, if blend shapes exist, this can completely break.

  3. bqq's Approach for Tangents: bqq shared his personal approach for handling tangents, which is to discard the DCC tangents unconditionally and recompute new ones with mikktspace. He believes this should be an option, not the default.

  4. Godot's Lack of Option for Recomputing Tangents: Lyuma pointed out that Godot doesn't provide the option to recompute tangents. If DCC tangents exist, Godot will always use them, leaving no way to fix these.

Suggested Improvements

  • A dropdown option for handling tangents like "always generate", "generate if missing", "ignore if missing", "remove always(?)".
  • Improvement in Godot's import settings to handle tangents better.

fire avatar Apr 25 '24 13:04 fire

A dropdown option for handling tangents like "always generate", "generate if missing", "ignore if missing", "remove always(?)".

A Remove Always option makes sense to minimize mesh file size/memory usage if you don't intend on using normal or height maps on the mesh in question.

Calinou avatar Apr 26 '24 22:04 Calinou

Remove Always option makes sense to minimize mesh file size/memory usage

The problem is that godot did something in 4.2 that made mesh format more rigid and broke many models unless they have normals and tangents. I don't want to add this until we fix the bugs in the rendering server

I understand the rendering team has a lot bigger issues to contend with right now but it would be nice to have a discussion with them on this issue at some point and figure out if there is a way to make mesh formats less rigid so we can optimize models that don't need tangents or normals.

lyuma avatar Apr 26 '24 23:04 lyuma

Remove Always option makes sense to minimize mesh file size/memory usage

The problem is that godot did something in 4.2 that made mesh format more rigid and broke many models unless they have normals and tangents. I don't want to add this until we fix the bugs in the rendering server

I understand the rendering team has a lot bigger issues to contend with right now but it would be nice to have a discussion with them on this issue at some point and figure out if there is a way to make mesh formats less rigid so we can optimize models that don't need tangents or normals.

Can you be more specific what issues you are seeing? AFAIK we have fixed all the issues with tangents in the RenderingServer already. There aren't any more bug reports. If you are seeing issues arising from the new mesh compression stuff, I'd welcome a bug report. Fixing these tangent issues is a very high priority for me.

clayjohn avatar Apr 26 '24 23:04 clayjohn

Can you be more specific what issues you are seeing? AFAIK we have fixed all the issues with tangents in the RenderingServer already. There aren't any more bug reports. If you are seeing issues arising from the new mesh compression stuff, I'd welcome a bug report. Fixing these tangent issues is a very high priority for me.

@clayjohn I am talking about the regression in 4.2 on meshes with blend shapes without tangents. That regression is still present in my 1-week-old build. Ensure Tangents on: works vrm_ensure_tangents_on

Ensure Tangents off: mesh fails to render vrm_ensure_tangents_off

I'm reluctant to offer an option in the import settings which will break meshes (I know we have Ensure Tangents, but we need to keep it for compatibility and it defaults to on)

lyuma avatar Apr 27 '24 06:04 lyuma

Can you be more specific what issues you are seeing? AFAIK we have fixed all the issues with tangents in the RenderingServer already. There aren't any more bug reports. If you are seeing issues arising from the new mesh compression stuff, I'd welcome a bug report. Fixing these tangent issues is a very high priority for me.

@clayjohn I am talking about the regression in 4.2 on meshes with blend shapes without tangents. That regression is still present in my 1-week-old build. Ensure Tangents on: works vrm_ensure_tangents_on

Ensure Tangents off: mesh fails to render vrm_ensure_tangents_off

I'm reluctant to offer an option in the import settings which will break meshes (I know we have Ensure Tangents, but we need to keep it for compatibility and it defaults to on)

Very interesting. Is there a bug report I can take a look at? I can fix this if I have access to a bug report or MRP. I haven't seen this issue reported before though

clayjohn avatar Apr 27 '24 06:04 clayjohn

@clayjohn documented in #86384 mrp: https://github.com/godotengine/godot/files/13738417/MeshTangents.zip

and mrp from 85960: https://github.com/godotengine/godot/files/13623984/resource_loader_test.zip (run and press end key)

lyuma avatar Apr 27 '24 09:04 lyuma

@clayjohn documented in #86384 mrp: https://github.com/godotengine/godot/files/13738417/MeshTangents.zip

and mrp from 85960: https://github.com/godotengine/godot/files/13623984/resource_loader_test.zip (run and press end key)

Thanks! I added it to the list of release blockers. I'll take a closer look during the week

clayjohn avatar Apr 28 '24 01:04 clayjohn