Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename get_surface_material to get_surface_override_material #47878

Merged
merged 1 commit into from Apr 15, 2021

Conversation

clayjohn
Copy link
Member

The surface material property of MeshInstance3Ds overrides the Material for a given surface. It behaves the same as override_material just on a per-surface basis. This has been poorly communicated to users who often mix up MeshInstance3D.get_surface_material() with Mesh.surface_get_material(). This becomes especially problematic when users set a material on the mesh and then try to get it from the MeshInstance3D as in #31558

Renaming the property to reflect that it is an override material is consistent with the override_material property of GeometryInstance3D and makes it clear that it is not the same things as a mesh material

Partially implements: godotengine/godot-proposals#2589
Implements: #31558 (comment)

@clayjohn clayjohn added this to the 4.0 milestone Apr 14, 2021
@clayjohn clayjohn requested review from a team as code owners April 14, 2021 04:07
@clayjohn clayjohn force-pushed the rename-get_surface_material branch from e122b58 to d16adc2 Compare April 14, 2021 06:02
@lawnjelly
Copy link
Member

Sounds sensible to me. 👍

@clayjohn clayjohn force-pushed the rename-get_surface_material branch from d16adc2 to 92731d2 Compare April 15, 2021 03:24
@akien-mga akien-mga merged commit c7a4e21 into godotengine:master Apr 15, 2021
@akien-mga
Copy link
Member

Thanks!

@clayjohn clayjohn deleted the rename-get_surface_material branch April 15, 2021 06:12
@lukostello
Copy link
Contributor

lukostello commented May 26, 2021

while this is a great improvement. I hope you have considered these options:

  1. naming the mesh resource surface materials "default_surface_materials" if you want it to be disambiguated from both sides.
  2. instead of "surface_override_material" it could be "instance_surface_material" this communicates effectively that it is unique to each instance and future proofs it if we ever switch from an override to an overlay method.
  3. The mesh resources surface_get_material seems wierd to me. Seems like it should be get_surface_material (or get_default_surface_material if you use suggestion 1. or even get_default_material(surface_name)

if there is a reason why it should be surface_get_material() then it seems the format of get_surface_material_override should also be surface_get_material_override()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants