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

CompositorEffect should use GDVIRTUAL_CALL() so it works with GDExtension #99981

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Dec 3, 2024

Virtual methods defined with GDVIRTUAL*() should always use GDVIRTUAL_CALL() to call them, so that they work via both GDExtension and GDScript.

Presently, CompositorEffect is using a trick with Callable() that only works with GDScript, and this PR switches to using GDVIRTUAL_CALL().

This is marked as DRAFT because I haven't actually tested it yet. I've never used compositor effects, so I need to get a test project going, but then I'll mark it as ready for review.

Fixes #99933

@dsnopek dsnopek added this to the 4.x milestone Dec 3, 2024
@dsnopek dsnopek requested review from BastiaanOlij and a team December 3, 2024 19:02
@dsnopek dsnopek requested review from a team as code owners December 3, 2024 19:02
@dsnopek dsnopek marked this pull request as draft December 3, 2024 19:05
@dsnopek dsnopek changed the title [DRAFT] CompositorEffect should use GDVIRTUAL_CALL() so it works with GDExtension CompositorEffect should use GDVIRTUAL_CALL() so it works with GDExtension Dec 6, 2024
@dsnopek dsnopek marked this pull request as ready for review December 6, 2024 22:46
@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 6, 2024

I tested this PR with the demo project here: https://github.com/godotengine/godot-demo-projects/tree/master/compute/post_shader

Everything seems to be working, so taking this out of draft!

@dsnopek dsnopek requested a review from a team December 10, 2024 16:08
@dsnopek dsnopek added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Dec 10, 2024
Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Haven't re-tested with Rust, but the callable_mp approach seems to be used in lots of places, so looks good to me. Thanks a lot!

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Dec 11, 2024
@Repiteo Repiteo merged commit 1c4d12d into godotengine:master Dec 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 11, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:gdextension topic:rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Virtual functions invoked through Callable aren't correctly dispatched to GDExtension
3 participants