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

Fix collision shape debug color breaking GDExtension #100317

Conversation

TCROC
Copy link
Contributor

@TCROC TCROC commented Dec 12, 2024

This fixes #100313.

Issue description copied in below:

Tested versions

System information

N/A

Issue description

Discovered when using Rust gdext here: https://github.com/godot-rust/gdext

ERROR: [panic]
  Failed to load class method CollisionShape3D::set_debug_color (hash 2920490490).
  Make sure gdext and Godot are compatible: https://godot-rust.github.io/book/gdext/advanced/compatibility.html

I've found exactly what is causing the issue. Its because Godot only binds these methods in debug and editor builds:

https://github.com/godotengine/godot/blob/38775731e8494a28deddc3febda895679f49fd6e/scene/3d/physics/collision_shape_3d.cpp#L183C1-L195C24

#ifdef DEBUG_ENABLED
	ClassDB::bind_method(D_METHOD("set_debug_color", "color"), &CollisionShape3D::set_debug_color);
	ClassDB::bind_method(D_METHOD("get_debug_color"), &CollisionShape3D::get_debug_color);

	ClassDB::bind_method(D_METHOD("set_enable_debug_fill", "enable"), &CollisionShape3D::set_debug_fill_enabled);
	ClassDB::bind_method(D_METHOD("get_enable_debug_fill"), &CollisionShape3D::get_debug_fill_enabled);

	ADD_PROPERTY(PropertyInfo(Variant::COLOR, "debug_color"), "set_debug_color", "get_debug_color");
	// Default value depends on a project setting, override for doc generation purposes.
	ADD_PROPERTY_DEFAULT("debug_color", get_placeholder_default_color());

	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "debug_fill"), "set_enable_debug_fill", "get_enable_debug_fill");
#endif // DEBUG_ENABLED

And don't let the DEBUG_ENABLED fool you like it did me. That is not only indicating a debug build. That is indicating it is an editor or debug build as can be seen in the SConstruct here:

env.debug_features = env["target"] in ["editor", "template_debug"]
. So even release editor builds have this.

env.debug_features = env["target"] in ["editor", "template_debug"]

I'm thinking the proper solution is to support these debug colors even in release builds. Especially since we do for collision_shape_2d: https://github.com/godotengine/godot/blob/38775731e8494a28deddc3febda895679f49fd6e/scene/2d/physics/collision_shape_2d.cpp#L269C1-L288C2

void CollisionShape2D::_bind_methods() {
	ClassDB::bind_method(D_METHOD("set_shape", "shape"), &CollisionShape2D::set_shape);
	ClassDB::bind_method(D_METHOD("get_shape"), &CollisionShape2D::get_shape);
	ClassDB::bind_method(D_METHOD("set_disabled", "disabled"), &CollisionShape2D::set_disabled);
	ClassDB::bind_method(D_METHOD("is_disabled"), &CollisionShape2D::is_disabled);
	ClassDB::bind_method(D_METHOD("set_one_way_collision", "enabled"), &CollisionShape2D::set_one_way_collision);
	ClassDB::bind_method(D_METHOD("is_one_way_collision_enabled"), &CollisionShape2D::is_one_way_collision_enabled);
	ClassDB::bind_method(D_METHOD("set_one_way_collision_margin", "margin"), &CollisionShape2D::set_one_way_collision_margin);
	ClassDB::bind_method(D_METHOD("get_one_way_collision_margin"), &CollisionShape2D::get_one_way_collision_margin);
	ClassDB::bind_method(D_METHOD("set_debug_color", "color"), &CollisionShape2D::set_debug_color);
	ClassDB::bind_method(D_METHOD("get_debug_color"), &CollisionShape2D::get_debug_color);


	ADD_PROPERTY(PropertyInfo(Variant::OBJECT, "shape", PROPERTY_HINT_RESOURCE_TYPE, "Shape2D"), "set_shape", "get_shape");
	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "disabled"), "set_disabled", "is_disabled");
	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "one_way_collision"), "set_one_way_collision", "is_one_way_collision_enabled");
	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "one_way_collision_margin", PROPERTY_HINT_RANGE, "0,128,0.1,suffix:px"), "set_one_way_collision_margin", "get_one_way_collision_margin");
	ADD_PROPERTY(PropertyInfo(Variant::COLOR, "debug_color"), "set_debug_color", "get_debug_color");
	// Default value depends on a project setting, override for doc generation purposes.
	ADD_PROPERTY_DEFAULT("debug_color", Color());
}

Steps to reproduce

Invoke the godot editor with the --dump-extension-api arg. Then inspect the extension_api.json and see that set_debug_color, get_debug_color, and other related methods that are only available in debug and editor builds are dumped for CollisionShape3D.

Proposed Solution:

Allow these methods in all builds the same way we do for ColisionShape2D.

Minimal reproduction project (MRP)

N/A

@dsnopek
Copy link
Contributor

dsnopek commented Dec 12, 2024

Thanks!

I agree that we shouldn't be conditionally binding these methods - the methods should always exist.

However, do we always want them to actually do something? Is it possible to show debug shapes in release builds? If so, then, yes, then this PR is probably the way to go. But if debug shapes can only be shown in editor or debug builds, then we probably want to remove the functionality in release builds, and just keep the method registration.

@TCROC
Copy link
Contributor Author

TCROC commented Dec 12, 2024

I'm honestly not sure. I've personally never used debug colors. But I did reference how it is done on the 2d side, and it does look like these ones are functional in release builds and do not differentiate with DEBUG_ENABLED define symbols:

https://github.com/godotengine/godot/blob/19e003bc08ac2fb14fc2434bde9530387afd994c/scene/2d/physics/collision_shape_2d.h

https://github.com/godotengine/godot/blob/19e003bc08ac2fb14fc2434bde9530387afd994c/scene/2d/physics/collision_shape_2d.cpp

I'll let someone with additional expertise on this area of code comment though. @dsnopek any idea who we should cc?

@dalexeev
Copy link
Member

@TCROC
Copy link
Contributor Author

TCROC commented Dec 20, 2024

@dalexeev sounds good! I'll update the pr! :)

@TCROC TCROC force-pushed the fix-collision-shape-debug-color-breaks-gdextension branch from 147435b to fd232ed Compare December 23, 2024 18:25
@TCROC TCROC requested a review from a team as a code owner December 23, 2024 18:25
@TCROC
Copy link
Contributor Author

TCROC commented Dec 23, 2024

@dalexeev The PR has been updated. Let me know if you have any other suggestions :)

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@TCROC TCROC force-pushed the fix-collision-shape-debug-color-breaks-gdextension branch from fd232ed to f0c077d Compare December 23, 2024 20:03
@akien-mga akien-mga changed the title Fix collision shape debug color breaking gdextension Fix collision shape debug color breaking GDExtension Dec 28, 2024
@Repiteo Repiteo merged commit df2b117 into godotengine:master Dec 30, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 30, 2024

Thanks!

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.

CollisionShape3d Debug Color breaks gdextension compatibility
4 participants