godot
godot copied to clipboard
godotengine
Index override surface name starting from 1 to match surface name
I hate to be making a compatibility breaking PR, but this has been a concern of mine for awhile and I unfortunately didn't realize it would be compatibility breaking until I made the PR.
The issue is that Mesh surfaces index starting from 1 while MeshInstance3D surfaces start indexing from 0. Leading to this:
Surface override 1 corresponds to surface 2. This isn't such a big deal for simple meshes like this as you can see all the surfaces in one screen. It becomes a big deal when using larger meshes. For example, the Sponza scene I use has over 40 surfaces, it becomes really annoying to set a material on a specific surface and it is easy to forget to account for the off-by-one error
You can plainly see the issue, in Mesh we add 1 to each index: https://github.com/godotengine/godot/blob/30800d20f452015c054dc7d1d7cfd38362fe28ff/scene/resources/mesh.cpp#L1521-L1528
In MeshInstance3D we dont: https://github.com/godotengine/godot/blob/30800d20f452015c054dc7d1d7cfd38362fe28ff/scene/3d/mesh_instance_3d.cpp#L101-L103
The real trouble lies in the fact that these are not just labels assigned to the meshes. The surfaces are serialized based on these names. So if we change the numbering for either one, we will break the surfaces that have already been saved. For example, with this PR, if I had filled all three surface override slots, then I would lose the material in override_surface 0, the material that was in override_surface 1 would still be called override_surface 1, but it would match up with surface 1 instead of surface 2 etc. Accordingly, this is clearly a breaking change and will impact any users who use override surfaces.
It may be too late for this change as we are late into the beta cycle. But It may be worth discussing.
With this change the above screenshot becomes:
Did you write a proposal for this?
This is clearly a breaking change and will impact any users who use override surfaces.
Elsewhere in the system we expect adding surfaces to meshes to count from 0.
It would be preferable to use the name of the material but the material doesn't commonly doesn't exist.
Can we find the original reason for this change?
https://docs.godotengine.org/en/latest/classes/class_arraymesh.html#class-arraymesh-method-add-surface-from-arrays
Edited:
TL;DR make both start from 0.
@fire this change just impacts how the surface is displayed in editor and its name in the tscn file for serialization purposes. It doesn't change how you access the surface material programmatically
I think we may be able to add some compatibility code via _set / _get.
We could detect if a MeshInstance3D is using surface_material_override/0, and assume that it's from before this change, and move all its saved surface_material_override/* properties by one index.
I think we may be able to add some compatibility code via
_set/_get. We could detect if a MeshInstance3D is usingsurface_material_override/0, and assume that it's from before this change, and move all its savedsurface_material_override/*properties by one index.
Yes, that's a good idea I'll give it a try!
@akien-mga updated the code to preserve forward compatibility as you suggested. So now, scenes saved with the old indexing can be opened just fine with the new indexing. They always save with the new indexing, so if the scene is saved with the new indexing, it can no longer be used in older versions of the engine with the old indexing.
This is a much smaller compatibility breakage than before. But would still warrant some pretty clear warnings in the blog post.
yeah this broke my code
mat = $%Laser/Line3D["surface_material_override/0"]
easy enough fix but I agree with @fire and don't understand why the names weren't just changed to match the indexes.
should my code above be getting the material differently?
should my code above be getting the material differently?
Yes, you should be using get_surface_override_material()
Ok thanks will do. thinking about that answer though,
we have so many properties and sometimes they are hard to actually find especially when deeply nested. right clicking the property and selecting "Copy property path" returns 'surface_material_override/1' which isn't going to help scripting because the function takes the index, in this case 0?
not to mention the naming inconsistency between surface_material_override surface_override_material
should I make a ticket for that?
the ticket for the naming issue is https://github.com/godotengine/godot/issues/70233