-
-
Notifications
You must be signed in to change notification settings - Fork 22.8k
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
base: master
Are you sure you want to change the base?
Conversation
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:
It seems like I just need to update |
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 |
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. |
There was a problem hiding this comment.
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 callClassDB::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 thesurface_get_format()
method from the parentMesh
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:
godot/core/extension/gdextension_interface.h
Lines 2867 to 2879 in 30456ba
/** | |
* @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.
I think we can use #80813 as a reference |
Ah, thanks for explaining the method hash behavior. Fortunately it looks like I created a small GDExtension project just to test these methods, and built it against godotengine/godot-cpp@4.4 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.
Rebased to resolve conflicts with master. |
Let me know if there is anything else I can do to help kick this over the finish line. |
This makes the
Mesh::surface_get_primitive_type()
,surface_get_format()
, andsurface_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()
andsurface_get_format()
methods were exposed to gdscript in theArrayMesh
subclass, but not in the baseMesh
class itself where they are defined in C++. This meant that gdscript code could not access these methods on other mesh types likePrimitiveMesh
subclasses.The fact that these methods were not previously exposed seems like an oversight to me:
_surface_get_primitive_type()
, and_surface_get_format()
, and_surface_get_lods()
methods were already exposed for gdscript to override in customMesh
subclasses.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.