Skip to content

Expose a few missing Mesh surface methods to gdscript #107952

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simpkins
Copy link
Contributor

@simpkins simpkins commented Jun 24, 2025

This makes the Mesh::surface_get_primitive_type(), surface_get_format(), and surface_get_lods() methods available in gdscript.

These methods exist in C++, but were previously not exposed to gdscript. This omission seems like an oversight, rather than intentional. The surface_get_primitive_type() and surface_get_format() methods were exposed to gdscript in the ArrayMesh subclass, but not in the base Mesh class itself where they are defined in C++. This meant that gdscript code could not access these methods on other mesh types like PrimitiveMesh subclasses.

The fact that these methods were not previously exposed seems like an oversight to me:

  • The virtual _surface_get_primitive_type(), and _surface_get_format(), and _surface_get_lods() methods were already exposed for gdscript to override in custom Mesh subclasses.
  • The Mesh::surface_get_arrays() method is exposed to gdscript, but the index data it returns cannot be interpreted properly without knowing the primitive type and format flags.

This fixes godotengine/godot-proposals#6890.

@simpkins simpkins requested a review from a team as a code owner June 24, 2025 20:01
@simpkins
Copy link
Contributor Author

The check failure looks related to GDExtension compatibility. I'm not all that familiar with GDExtension, but I believe this change should be safe from a compatibility perspective:

  • For people compiling existing GDExtensions from source, the definitions for these methods have simply moved from the generated ArrayMesh class in gen/include/godot_cpp/classes/array_mesh.hpp into the parent Mesh class, so everything should still compile.
  • For people attempting to run GDExtensions built for an older version of Godot with a newer Godot engine that includes this commit, I believe the code should also still work: older methods like ArrayMesh.surface_get_format() will call ClassDB::get_method_with_compatibility("ArrayMesh", "surface_get_format") to get the method, and this code does look up the inheritance change so I believe it should correctly return the surface_get_format() method from the parent Mesh class.

It seems like I just need to update misc\extension_api_validation\4.4-stable.expected to suppress this warning, if I understand correctly.

@AThousandShips
Copy link
Member

I do believe this might break compatibility because of method pointers in some cases, needs testing so added the label and we can remove it if it doesn't end up breaking compatibility

Comment on lines +334 to +331
GH-107952
--------
Validate extension JSON: API was removed: classes/ArrayMesh/methods/surface_get_format
Validate extension JSON: API was removed: classes/ArrayMesh/methods/surface_get_primitive_type

Moved methods from ArrayMesh to its parent Mesh class. No adjustments should be necessary.
Copy link
Contributor

@Bromeon Bromeon Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: sorry I missed the previous message:

I believe the code should also still work: older methods like ArrayMesh.surface_get_format() will call ClassDB::get_method_with_compatibility("ArrayMesh", "surface_get_format") to get the method, and this code does look up the inheritance change so I believe it should correctly return the surface_get_format() method from the parent Mesh class.

I wasn't aware of this derived-to-base lookup. It seems to happen in type = type->inherits_ptr here:

godot/core/object/class_db.cpp

Lines 1076 to 1109 in 30456ba

MethodBind *ClassDB::get_method_with_compatibility(const StringName &p_class, const StringName &p_name, uint64_t p_hash, bool *r_method_exists, bool *r_is_deprecated) {
Locker::Lock lock(Locker::STATE_READ);
ClassInfo *type = classes.getptr(p_class);
while (type) {
MethodBind **method = type->method_map.getptr(p_name);
if (method && *method) {
if (r_method_exists) {
*r_method_exists = true;
}
if ((*method)->get_hash() == p_hash) {
return *method;
}
}
LocalVector<MethodBind *> *compat = type->method_map_compatibility.getptr(p_name);
if (compat) {
if (r_method_exists) {
*r_method_exists = true;
}
for (uint32_t i = 0; i < compat->size(); i++) {
if ((*compat)[i]->get_hash() == p_hash) {
if (r_is_deprecated) {
*r_is_deprecated = true;
}
return (*compat)[i];
}
}
}
type = type->inherits_ptr;
}
return nullptr;
}

In that case I agree with @AThousandShips that we should test this.


Old post

This breaks GDExtension because function pointers are fetched by class name and hash:

/**
* @name classdb_get_method_bind
* @since 4.1
*
* Gets a pointer to the MethodBind in ClassDB for the given class, method and hash.
*
* @param p_classname A pointer to a StringName with the class name.
* @param p_methodname A pointer to a StringName with the method name.
* @param p_hash A hash representing the function signature.
*
* @return A pointer to the MethodBind from ClassDB.
*/
typedef GDExtensionMethodBindPtr (*GDExtensionInterfaceClassdbGetMethodBind)(GDExtensionConstStringNamePtr p_classname, GDExtensionConstStringNamePtr p_methodname, GDExtensionInt p_hash);

This means the pointer can no longer be fetched under the old class name. Extensions compiled against earlier Godot APIs thus have no way of using these two methods when run under newer Godot versions (barring reflection). Calling through old pointers unchecked would result in undefined behavior.

You can address this by registering compatibility methods that just pass through to the new implementation, and afterwards remove all changes to this file.

@AThousandShips
Copy link
Member

I think we can use #80813 as a reference

simpkins added a commit to simpkins/gdextension_test that referenced this pull request Jun 25, 2025
@simpkins
Copy link
Contributor Author

Ah, thanks for explaining the method hash behavior. Fortunately it looks like MethodInfo::get_compatibility_hash() hash does not use the class name when computing the hash, and only uses the method signature. Therefore the hash for the 2 existing methods are both unchanged (3718287884 for surface_get_format() and 4141943888 for surface_get_primitive_type()) despite the fact that they are now in a different class.

I created a small GDExtension project just to test these methods, and built it against godotengine/godot-cpp@4.4
I confirmed it appears to work correctly when run both with godot.exe from 4.5-beta1 as well as with a build from this PR branch.
Here's my test code: https://github.com/simpkins/gdextension_test/blob/1a962861e684a84d7f8cbed35e3d34c309ff2afe/src/gdexample.cpp

Let me know if that covers the cases you are concerned about, or if I should be testing something else.

This makes the `Mesh::surface_get_primitive_type()`,
`surface_get_format()`, and `surface_get_lods()` methods available in
gdscript.

These methods exist in C++, but were previously not exposed to gdscript.
This omission seems like an oversight, rather than intentional.  The
`surface_get_primitive_type()` and `surface_get_format()` methods were
exposed to gdscript in the `ArrayMesh` subclass, but not in the base
`Mesh` class itself where they are defined in C++.  This meant that
gdscript code could not access these methods on other mesh types like
`PrimitiveMesh` subclasses.

The fact that these methods were not previously exposed seems like an
oversight to me:
* The virtual `_surface_get_primitive_type()`, and
  `_surface_get_format()`, and `_surface_get_lods()` methods were
  already exposed for gdscript to override in custom `Mesh` subclasses.
* The `Mesh::surface_get_arrays()` method is exposed to gdscript, but
  the index data it returns cannot be interpreted properly without
  knowing the primitive type and format flags.

This fixes godotengine/godot-proposals#6890.
@simpkins
Copy link
Contributor Author

Rebased to resolve conflicts with master.

@simpkins
Copy link
Contributor Author

simpkins commented Jul 1, 2025

Let me know if there is anything else I can do to help kick this over the finish line.

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.

Add missing surface_get_lods() method to Mesh/ArrayMesh
3 participants