[3.5] Mesh merging docs unclear; Doesn't properly restore materials

Godot version

3.5rc2

System information

Win 10/64 NVIDIA GeForce GTX 1060/PCIe/SSE2

Issue description

These two meshes have the same number of material slots and the exact same materials in the same order.

Issue 1 Merging the right into the left does not merge. It overwrites the left. What is shown here are the "combined" mesh on the left, and the original second mesh on the right. I specified global position, so the merged mesh should look identical to the above except as one meshinstance instead of two.

And the engine says they are mergable and that the merge was successful.

To run the script, click the root node and click start merge.

Issue 2 The original meshes have their materials on the surface slots of the mesh, while the material slots (on the material, not the mesh) are blank and available for special use.

On the new, combined item, the materials are not on the mesh. They are on the meshInstance. No good. They need to be put back where they were found, on the mesh surfaces, leaving the material overrides alone. Or better, duplicating exactly from the originals: Mesh surfaces -> Mesh surfaces, MeshInstance Materials -> MeshInstance Materials.

Reference https://github.com/godotengine/godot/pull/57661#issuecomment-1142317558 @lawnjelly

Steps to reproduce

See MRP.

Minimal reproduction project

test_merge.zip

Try this: tinmanjuggernaut_merging.zip

The issue seems to be a confusion, merge_meshes() should be used on a fresh MeshInstance, not one of the meshes being merged. I'll see if this can be made clearer in the docs / classref. I can also put a check in during merging that the destination MeshInstance isn't in the list of sources, and flag an error in this case.

Essentially I can see how it might be thought they would be able to merge into one of the source MeshInstances, but the destination data has to be separate from the source data. We could do some jiggery pokery to rename the old MeshInstance, create a new one with the same name, then merge the data into this, but this could be problematic if the user was holding a reference to the MeshInstance in question, and could lead to difficult to diagnose bugs.

So it seems like it might be better to just enforce this, make it clear in the docs and flag it as an error condition.

Issue 2

I'll have a look to see whether the material can be set on the alternative place. My aims originally were to just show the same visual result (not anticipating the user would want to change the merged MeshInstance further), but I can see that in some use cases it might be nice to be able to apply a material override later to a merged mesh (is this what you were intending? Maybe you can be more clear what you mean by "no good". In what respect is this problematic? This info can make it easier to decide how it should work.).

I'm not absolutely sure some of these possible use cases are a good idea to support as far as feature creep is concerned (at least during the early roll out), because it then implies some kind of repeatability - which would have to be supported in future and potentially make code more complex.

The issue seems to be a confusion, merge_meshes() should be used on a fresh MeshInstance, not one of the meshes being merged. I'll see if this can be made clearer in the docs / classref.

Yes that does work, thank you.

I can also put a check in during merging that the destination MeshInstance isn't in the list of sources, and flag an error in this case.

It already errors if the merging mesh is in the source list. eg mi1.merge_meshes([mi1, mi2, mi3]). I was confused because is_mergeable_with requires comparing two existing meshes, mi1.is_mergeable_with(mi2), so I followed the same format. You should write that the destination mesh should be a new, empty MI and provide an example in the docs.

I think the only thing you'll be able to check is if merging into mi1, and it already has data, push a warning that mi1 is being overwritten with the merge list. And specify that in the documentation so the user knows they need to specify all source meshes in the list, and not as the destination mesh, unlike is_mergeable_with, which requires data on both sides of the ..

Essentially I can see how it might be thought they would be able to merge into one of the source MeshInstances, but the destination data has to be separate from the source data. We could do some jiggery pokery to rename the old MeshInstance, create a new one with the same name, then merge the data into this,

It's sufficient to provide an example in the documentation.

Issue 2

I'll have a look to see whether the material can be set on the alternative place.

It's not the alternative place. Where I set the materials is the correct place. Meshes have surfaces for materials. MeshInstances also have materials. The slots are available for different purposes, and you should put things back the way you found them. Every MeshInstance has material slots, and has a Mesh, and every Mesh has surfaces for material slots. Yes you can place them back where they were.

You used MeshInstance.set_surface_material(surface: int, material: Material) instead of MeshInstance.Mesh.surface_set_material(surf_idx: int, material: Material).

But really you should be storing all MeshInstance surface materials and all MeshInstance.Mesh surface materials, then restoring them after merging.

My aims originally were to just show the same visual result (not anticipating the user would want to change the merged MeshInstance further)

The objects are not fully baked. As you said it's a very simple implementation, which means it's insufficient for real world use by itself. The objects will need to go through a lot more processing, whether automatic or manual before they will be useable. And since we have complex objects, heavy processing probably won't be practical at runtime, which means we need to write our own script to bake these out and store them. We're currently doing this to export into blender and back. The interest in this PR is to do it all in Godot.

, but I can see that in some use cases it might be nice to be able to apply a material override later to a merged mesh (is this what you were intending?

Material overrides are simply optional slots. In general I won't use them as materials should always default to the mesh material slots, not the MI slots where you put them. The MI override slots might used for special cases, for instance if a wooden object should be burned, it's base wood material can be overridden with a burned wood material.

In any case you should not be making the choice for us by moving my mesh materials to override slots and stripping out the mesh materials. As I wrote before, you should put things back the way you found them. Mesh surfaces to mesh surfaces, Mesh Instance materials to mesh instance materials.

Maybe you can be more clear what you mean by "no good". In what respect is this problematic? This info can make it easier to decide how it should work.).

The problem is that you're changing the behavior of my scene, mesh and material structure unnecessarily. All you're doing is merging meshes. The end result should be virtually identical to the source, except the vertices and faces are now part of the same object, Nothing else should be different. But it is. You've erased all of the mesh materials, and placed them in override slots.

I'm not absolutely sure some of these possible use cases are a good idea to support as far as feature creep is concerned (at least during the early roll out), because it then implies some kind of repeatability - which would have to be supported in future and potentially make code more complex.

I don't know what use cases you are referring to. The purpose of the PR was to merge meshes within the limited scope of matching meshes. That's fine and was accomplished well. However the PR does not do the work cleanly. It's like it came over to my house to clean, then rearranged the furniture without putting it back the way it was.

As noted in https://github.com/godotengine/godot-proposals/issues/3920 for Godot 4, which also applies here in Godot 3.

Materials should be returned where they were placed by gamedevs, and should not be moved anywhere else or reordered. Each material property has their own purpose. All materials at every level should be copied to the merged mesh:

Master mesh materials are imported here: Mesh.surface_set_material(surf_idx: int, material: Material) Per Instance, per surface overrides: MeshInstance.set_surface_material(surface: int, material: Material) All surface override: GeometryInstance.set_material_override(value: Material)

The mesh merging functionality is being considerably expanded in 3.6, see #61568 . As you observed, the intention of mesh merging is currently not to produce a material arrangement identical to the source meshes. Indeed this is not possible once you start merging non-duplicate meshes (consider if you have mesh A with wood in slot 0, and metal in slot 1, and mesh B with metal in slot 0 and wood in slot 1). Instead the object is to produce a similar visual result, with the simplest code possible.

To preserve material arrangements may have to be considered a special case for duplicates, and there would have to be a proven use case for this with broad support. We generally aim to keep code as simple as possible, and not add extra complexity unless there is a proven use case. So this isn't to say it can't be done, just that it would be good if you can elaborate on why it is necessary, before I make the effort to write it, because ultimately I will have to convince the other maintainers before it will be merged (not to mention not wasting dev time that could be spent on something else).

the intention of mesh merging is currently not to produce a material arrangement identical to the source meshes. Indeed this is not possible once you start merging non-duplicate meshes (consider if you have mesh A with wood in slot 0, and metal in slot 1, and mesh B with metal in slot 0 and wood in slot 1).

Perhaps we miscommunicated. Maintaining the order of material slots is not desired or requested. I understand those may get shifted around. What should remain the same is the materials stored on the same class member they were originally assigned to: Mesh surface_material, MeshInstance material override, or Geometry material override.

The current functionality inappropriately moves materials from Mesh.surface_material to MeshInstance.material.

Also there's no reason why multiple materials, some shared some unique can't be added as slots. A simple map would translate UVs from the old material slot to the new one. I know you understand the logic.

So:

Part A w/ Mesh.surface_material { 0:wood }, MeshInstance.material: NULL, Geometry material: NULL
Part B w/ Mesh.surface_material { 0:metal, 1:wood }, MeshInstance.material: NULL, Geometry material: NULL
Part C w/ Mesh.surface_material { 0:glass }, MeshInstance.material: NULL, Geometry material: NULL

Should turn into:

House w/ Mesh.surface_material { 0: wood, 1: metal, 2: glass }, MeshInstance.surface: NULL, Geometry material: NULL

Blender merges meshes with unique or shared materials quite easily. We've used it quite a bit to merge our houses constructed from Godot scenes containing walls, doors, windows, etc. Many of the meshes share wood, but some have extra materials like thatch, metal, glass. Blender has no issue combining hundreds of house components into one house mesh with 5-8 unique materials.

We generally aim to keep code as simple as possible, and not add extra complexity unless there is a proven use case. So this isn't to say it can't be done, just that it would be good if you can elaborate on why it is necessary, before I make the effort to write it, because ultimately I will have to convince the other maintainers before it will be merged (not to mention not wasting dev time that could be spent on something else).

It is quite common for a level designer to take house component meshes and build a house or other structure out of them. This isn't just a one time, unproven, special case. This is the normal workflow for level designers. There are countless videos on youtube showing this process, eg. https://www.youtube.com/watch?v=aIzMFWPQPjs

After the level design, the responsibility falls to the engine to render it optimally, or provide a facility to bake or merge meshes. Godot 3 does not batch well, or at all, so houses like these result in thousands of draw calls. We manually process a house through blender to reduce this down to 4-5 draw calls.

Ultimately I don't need this functionality in Godot 3.5. The current system in GD3 doesn't work well given the issues I've noted, so if we were staying on 3, we'd continue exporting to blender and reimporting to Godot. We are migrating to 4. If its mesh merging has similar issues, we will continue going through the extra steps of processing through blender. If the functionality in GD4 matches what can be done in blender, then we'll appreciate the time savings and use the GD4 function.

The issues I listed on this ticket are there and relevant to gamedevs using 3.5, but feel free to close this ticket if you don't have an intention of doing anything more in 3.5. As I said, we're moving to 4, I am just providing real world feedback to the engine functionality.

Hopefully 3.6 merging will do closer to what you are after. :+1:

3.5 is rather limited because it basically binds the functionality that was introduced in 3.4 simply for merging duplicates in rooms in the portal system.

I can't guarantee yet whether merging will be available in 4.x, there is some resistance to adding it based on the hope that the cheaper drawcalls / state changes in vulkan will reduce the need. Of course this benefit won't be present in the GLES3 renderer.

godotengine / godot