godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.05k stars 21.18k forks source link

Export MeshLibrary - Apply MeshInstance Transform does not work for GLTF-based Node3D #96357

Open dmlary opened 2 months ago

dmlary commented 2 months ago

Tested versions

System information

Godot v4.3.stable - macOS 14.6.1 - Vulkan (Forward+) - integrated Apple M3 Max - Apple M3 Max (16 Threads)

Issue description

When using Scene -> Export As -> MeshLibrary with Apply MeshInstance Transforms enabled, the transform to Node3Ds that are imported from a GLTF are not added to the MeshLibrary.

Due to the explicit phrasing of "MeshInstance Transforms" I believe this is expected behaviour, but it's unexpected behavior from a user perspective. Tilesets frequently come in GLTF format, and it is very confusing to enable "Apply [...] Transform", but not have it applied to the exported meshes.

I can understand if this is an immediate close bug, but I was unable to find any documentation of this limitation. Developers should be aware that if they PoC with MeshInstances, switching to GLTF assets later will not work if any transform is needed.

The only workaround I've found is to manually update the MeshLibrary in the editor for the transform.

Steps to reproduce

Demo:

(Added grass tile from kaykit midevil hexagon pack, and translated it up 1.0)

Screenshot 2024-08-30 at 6 06 10 PM

(Export with "Apply MeshInstance Transforms" enabled)

Screenshot 2024-08-30 at 6 07 01 PM

(Editing the Mesh in the MeshLibrary, the Mesh Transform has a y offset = 0; it should be 1.0)

Screenshot 2024-08-30 at 6 07 36 PM

Minimal reproduction project (MRP)

mesh-library-export-apply-transform.zip

dmlary commented 2 months ago

Update

Everything below is wrong; I forgot I went up one frame before printing the Node3D transform.

The problem here is the heirarchy is:

- Node3D with transform
  |- MeshInstance3D without transform

MeshLibraryEditor::_import_scene_parse_node() only exports MeshInstance3Ds and doesn't take into account any parent transform.

Lies below this line

What's interesting here is that in MeshLibraryEditor::_import_scene_parse_node() if you cast p_node to Node3D, get_transform() returns the correct transform. But in the code the node is first cast to MeshInstance3D, and get_transform() returns the wrong value for the transform (the default, no transform).

Is this just because Node3D and MeshInstance3D are storing their transform in different places?

https://github.com/godotengine/godot/blob/61598c5c88d95b96811d386cb20d714c35f4c6d7/editor/plugins/mesh_library_editor_plugin.cpp#L159-L162

frame #0: 0x0000000101ccc740 godot.macos.editor.arm64`MeshLibraryEditor::_import_scene_parse_node(p_library=Ref<MeshLibrary> @ 0x000000016fdfbbc0, p_mesh_instances=0x000000016fdfbd50, p_node=<unavailable>, p_merge=<unavailable>, p_apply_xforms=true) at mesh_library_editor_plugin.cpp:161:45 [opt]
   158
   159      Transform3D item_mesh_transform;
   160      if (p_apply_xforms) {
-> 161          item_mesh_transform = mesh_instance_node->get_transform();
   162      }
   163      p_library->set_item_mesh_transform(item_id, item_mesh_transform);
   164
   (lldb) p ((Node3D*)p_node)->get_transform()
(Transform3D) {
  basis = {
    rows = {
      [0] = {
         = {
           = (x = 1, y = 0, z = 0)
          coord = ([0] = 1, [1] = 0, [2] = 0)
        }
      }
      [1] = {
         = {
           = (x = 0, y = 1, z = 0)
          coord = ([0] = 0, [1] = 1, [2] = 0)
        }
      }
      [2] = {
         = {
           = (x = 0, y = 0, z = 1)
          coord = ([0] = 0, [1] = 0, [2] = 1)
        }
      }
    }
  }
  origin = {
     = {
       = (x = 0, y = 1, z = 0)
      coord = ([0] = 0, [1] = 1, [2] = 0) // <--- Correct origin: (0, 1, 0)
    }
  }
}
(lldb) n
Process 88630 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x0000000101ccc760 godot.macos.editor.arm64`MeshLibraryEditor::_import_scene_parse_node(p_library=Ref<MeshLibrary> @ 0x000000016fdfbbc0, p_mesh_instances=0x000000016fdfbd50, p_node=<unavailable>, p_merge=<unavailable>, p_apply_xforms=true) at mesh_library_editor_plugin.cpp:163:2 [opt]
   160      if (p_apply_xforms) {
   161          item_mesh_transform = mesh_instance_node->get_transform();
   162      }
-> 163      p_library->set_item_mesh_transform(item_id, item_mesh_transform);
   164
   165      Vector<MeshLibrary::ShapeData> collisions;
   166      for (int i = 0; i < mesh_instance_node->get_child_count(); i++) {
Target 0: (godot.macos.editor.arm64) stopped.
(lldb) frame v item_mesh_transform
(Transform3D) item_mesh_transform = {
  basis = {
    rows = {
      [0] = {
         = {
           = (x = 1, y = 0, z = 0)
          coord = ([0] = 1, [1] = 0, [2] = 0)
        }
      }
      [1] = {
         = {
           = (x = 0, y = 1, z = 0)
          coord = ([0] = 0, [1] = 1, [2] = 0)
        }
      }
      [2] = {
         = {
           = (x = 0, y = 0, z = 1)
          coord = ([0] = 0, [1] = 0, [2] = 1)
        }
      }
    }
  }
  origin = {
     = {
       = (x = 0, y = 0, z = 0)
      coord = ([0] = 0, [1] = 0, [2] = 0) // <--- Wrong origin (0, 0, 0)
    }
  }
}
BlackShift commented 2 months ago
- Node3D with transform
  |- MeshInstance3D without transform

@dmlary Would it make more sense to apply a global transform? I can see other issues arising from that, but it would fix this use case. I also agree that this isn't really a bug, just counter-intuitive when using inherited scenes from a GLTF.

dmlary commented 2 months ago

Would it make more sense to apply a global transform?

@BlackShift I think so. Worst case, make a mutually exclusive “Apply MeshInstance Global Transforms” checkbox to prevent breaking any existing behavior. I’d also change the existing checkbox to say “Apply MeshInstance Local Transform”. Help people understand what is being done internally.

BlackShift commented 2 months ago

@dmlary Well it is pretty easy to fix and change it, I have a branch with some changes that make a global transform possible. I won't open a PR for it because its not a bug and it would need an improvement proposal.

image

image

https://github.com/BlackShift/godot/commit/17339ff23cd549116ea9bf1f1b81acd187cace02

I don't think this would be the right way to implement it. I also don't like the UI for it, but it seems to work.

dmlary commented 2 months ago

I agree, it's not the most pleasant UI. If godot feels comfortable saying that the transition from local to global transforms is unlikely to break anyone; I'd say do it without the independent option.

My feeling thinking about a workflow where that transition would cause a problem is that it is unlikely to impact anyone. Most examples of a tileset scene have all the meshes overlapping at the origin. The user would have to have created intermediate nodes for each mesh, then translate that intermediate. That feels like a lot of extra work to spread out the tiles in a rarely used view.

Regardless, I opened the proposal https://github.com/godotengine/godot-proposals/issues/10622