Make names of the methods for mesh materials more consistent

[lukostello]

Describe the project you are working on

I'm trying to follow along with the 1000 fish demo for the multimesh tutorial

Describe the problem or limitation you are having in your project

I couldn't apply the shader I used for a single mesh for the multimesh

Describe the feature / enhancement and how it helps to overcome the problem or limitation

the mesh instance provides a get_surface_material method that I used when making the shader for the single mesh and if the multimesh also had the same method I wouldn't need to create 2 codes one for the single mesh instance and another for the multimesh instance. Instead I had to do multimesh.mesh.surface_get_material(0) which you'll notice references the mesh resource which has yet another convention.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I think mesh instance, multi-mesh instance and the mesh resource should all use the same convention. Not just with the get_surface_material but also set_surface_material and probably other methods as well.

If this enhancement will not be used often, can it be worked around with a few lines of script?

You'd have to create and maintain 2 seperate scripts which would be tedious and unnecessary.

Is there a reason why this should be core and not an add-on in the asset library?

its just some name changes.

P.S. would be handy to have a downloadable finished version of the 1000 fish demo. I'm still having trouble figuring out how to apply a vertex shader to something that already has a fragment shader and retain the fragment shader properties.

[DJ-Dav]

About the fragment/vertex shader problem, the two shader types can be added as different functions in one shader script.

And yes, I know this doesn't fix your main problem but I hope it helps...

[lukostello]

About the fragment/vertex shader problem, the two shader types can be added as different functions in one shader script.

And yes, I know this doesn't fix your main problem but I hope it helps...

That seems like a problem if you needed to use the same vertex shader for several objects with different fragment shaders.

[clayjohn]

Here is a relevant discussion

To summarize here, get_surface_material() in the MeshInstance API does something very different from surface_get_material() in the Mesh API. MeshInstance stores its own list of override materials which override the materials in the mesh. They are not two functions for accessing the same property.

In your described use-case, you should always be using mesh.surface_get_material() which is consistent between MeshInstance and MultiMeshInstance

The confusion here arises because the API does not make it clear that each Mesh stores a list of materials for its surfaces and each MeshInstance stores a different list of materials that override each surface's material.

Accordingly, there are two possible changes that may help the confusion you are having:

1. Rename get_surface_material() to get_surface_override_material()
2. Add an array of override materials to MultiMeshInstance similar to MeshInstance

[lukostello]

Yea I didn't know that. Both those suggestions sound reasonable. It could also be simplified to get_override_material() just be sure that whatever it is named that there is a mirror to that in the multimesh class. That is unless is some reason that only meshes need over ride materials but multimeshes don't.

[clayjohn]

The name needs to specify that it overrides a particular surface or else it will get confused with get_material_override()

[lukostello]

one thing I don't understand is why there needs to be surface material overrides in the mesh instance in the first place. Seeing as how you can already make the original mesh materials local to scene and over ride them using the code associated with the resource.

[clayjohn]

This allows you to share a Mesh between MeshInstances and still have unique Materials on a per-MeshInstance basis.

For example, I might have a forest of trees, for performance reasons I want each tree to use the same Mesh (instead of duplicating the same Mesh for each tree using "local to scene"). But I may want to apply a different Material to a tree I have selected. Instead of duplicating the Mesh resource and then applying a new Material, I can just set the material override of the tree temporarily. Then once I remove the material override the tree goes back to normal.

This is not only much better for performance, but it also really simplifies the organization of your scene. In other words, for medium to large scenes it will be way too complex to have a separate Mesh resource for every MeshInstance that needs a different Material. The sheer number of Mesh resources would quickly become unmanageable.

I hope renaming the property and the associated setter will help clear up the confusion you are experiencing for other users.

[lukostello]

Maybe I should start a new feature proposal for this, but it is relevant here. Perhaps rather than override it should be overlay. Such that you could apply a translucent material over the material from the mesh resource. In your example you could just apply a layer of blue when selected with an alpha of .5. You could still have a boolean with the option to over ride but I think having the option to overlay would be extremely powerful. Because you aren't getting rid of the previous information it would even help with the 1000 fish project I'm trying to do. I could use the material overlay function inherited from the geometry class to overlay a vertex shader, that way I can still use all the textures from the materials from the mesh resource

[clayjohn]

@lukostello That is an interesting idea and yeah, it would certainly require another proposal.

Off the cuff, a few comments:

1. You can already do an overlay using the next_pass property of Materials. This draws the object a second time with a different material. This works really well for your blue tint example.
2. Godot doesn't have a concept of vertex shader or fragment shader, it uses materials and shaders. So there is no way to just "overlay" a vertex shader on top of another complete shader

[DJ-Dav]

About the fragment/vertex shader problem, the two shader types can be added as different functions in one shader script.
And yes, I know this doesn't fix your main problem but I hope it helps...

That seems like a problem if you needed to use the same vertex shader for several objects with different fragment shaders.

Ctrl C Ctrl V

[lukostello]

@clayjohn
One nitpick I just had recently is how the mesh instance overrides have "material" slots rather than surface slots like the mesh reference has. To me this implies that if there is a mesh with 8 surfaces and the odd surfaces use material1 and even surfaces use material2, then because it is "material" slots and not surface slots then I would fully expect there to only be 2 slots. (unless this is what you guys are actually going for). My suggestion is in the mesh resource have the drop down named "default surfaces" and for the mesh instance have the drop down named "override surfaces" or alternatively "default surface materials" and override surface materials" to highlight the relationship between the 2. Thus the methods should be named get_default_material(surface_name) and get_override_material(surface_name) respectively. I personally don't see the issue with having the same name as geometry instance's get_override_material() its just what would be returned if there isn't a surface name specified (although I'm not sure what happens if you don't specify an argument for get_default_material(surface_name))

Also I think the mesh resource surfaces don't need a row just for the name. It could look the same way it looks in the mesh instance "materials" then to change the name you just double click on the number on the left hand side and write it there. In fact you should be able to do this in the mesh resource default surface drop down or the mesh instance override surface drop down and the names would be shared between them.

[clayjohn]

I'm not sure I follow what you are suggesting. A surface and a material are two different things. A surface is a collection of vertex attributes (arrays of position, normal, uv etc.) combined with a material. The MeshInstance has no way of overriding specific surfaces. It overrides the material associated with a particular surface.

In 3.4 the "Material" dropdown menu in the MeshInstance is now named "surface material override" to make it clear it is a material override for each surface.

[lukostello]

@clayjohn "surface material override" is very similar to my suggestion of "override surface material" doing that will satisfy the main nitpick I had. and I think you already changed the methods to match that. One thing I added is calling the surface materials from the mesh resource "default surface materials" if you want to distinguish the difference on both sides.

Other than that I noticed the row used in the mesh resource "surfaces" for name isn't necessary. The names could be on the left and the materials could be on the right similar to how it is with the mesh instance "materials" then to give it a new name you just double click on the name (which by default would be a number) and type to change it. These names would be shared between both the mesh resource default surface materials and mesh instance override surface materials and could be used in methods like get_surface_material(surface_name) because I think you can only access them by number right now and I'm not entirely sure what the names are for atm.

[lukostello]

or perhaps better yet "default_surface_material" and "instance_surface_material" that way if we ever switch to the overlay method, the name stays the same

[lukostello]

poop, I just realized I do infact need instance surface materials for a multimesh instance in my game.

edit: nvm was able to do what I wanted using other means. I was under the impression that if you needed to feed parameters to a shader the shader had to be on the override material. But you can just make it unique with the mesh resource material. In fact I'm realizing that it would be very odd to need a material over ride for multimesh instance. Because the over ride on the mesh instance is for when there are many meshes with the same material except for a few. If you've got many multimeshes with the same material except for a few; I'd wager you could be using those multimeshes more effectively such for each shader being used by a multimesh you'd only need one multimesh implements them