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

Attaching GDscript script to Rust class stops physics_process from running #111

Closed
lilizoey opened this issue Feb 4, 2023 · 9 comments
Closed
Labels
bug c: register Register classes, functions and other symbols to GDScript status: upstream Depending on upstream fix (typically Godot)

Comments

@lilizoey
Copy link
Member

lilizoey commented Feb 4, 2023

Given this code:

use godot::{engine::Engine, prelude::*};

#[derive(GodotClass)]
#[class(base=Node)]
pub struct Mini;

#[godot_api]
impl GodotExt for Mini {
    fn init(_: Base<Node>) -> Self {
        Self
    }

    fn physics_process(&mut self, _delta: f64) {
        if Engine::singleton().is_editor_hint() {
            return;
        }
        godot_print!("test");
    }
}

struct Testing;

#[gdextension]
unsafe impl ExtensionLibrary for Testing {}

When running a project containing a Mini node, it prints test repeatedly to the console as expected. However if you attach a script to the node, it will no longer print anything.

@Bromeon
Copy link
Member

Bromeon commented Feb 4, 2023

Do you know if this is something that Godot generally supports?
For example, does this currently work with the godot-cpp binding?

I found issue godotengine/godot#72034, but that talks about accessibility and not that the method themselves stop working.

@lilizoey
Copy link
Member Author

lilizoey commented Feb 4, 2023

it appears related to godotengine/godot-cpp#1022

@Bromeon Bromeon added bug status: upstream Depending on upstream fix (typically Godot) c: register Register classes, functions and other symbols to GDScript labels Feb 4, 2023
@Bromeon
Copy link
Member

Bromeon commented Feb 4, 2023

Thanks! There's probably not much that can be done on godot-rust side; I marked it as upstream for now.

@Bromeon Bromeon changed the title Attaching a gdscript script to a class defined in rust stops physics_process from running Attaching GDscript script to Rust class stops physics_process from running Feb 4, 2023
@HenryWConklin
Copy link
Contributor

HenryWConklin commented Oct 17, 2023

Ran into this as well. As mentioned in the linked issue, it seems like overriding on_notification instead and watching for Process/PhysicsProcess events is the best workaround. That seems to work reliably with or without a script attached, and it's what the native types do.

E.g. for a Node

fn init(base: Base<Node>) {
    // Ensure we get Process notifications, defaults to false.
    base.set_process(true);
}
fn on_notification(&mut self, notification: NodeNotification) {
    match notification {
        NodeNotification::Process => {
            let delta = self.base.get_process_delta_time(); 
            // ...
        }
        _ => {}
    }
}

@PgBiel
Copy link
Contributor

PgBiel commented Oct 20, 2023

This PR (which has just landed on 4.2 beta) has partially fixed this issue: godotengine/godot#83583

Now, the mere act of attaching a script to your gdext class will not suddenly make all overridden virtual methods (including physics_process) stop working.

However, if you explicitly override one of the virtual methods overridden by the extension, such as if you override _physics_process in the GDScript file, the original override of that particular method in the extension class won't run anymore, and calling super() from GDScript doesn't work (throws a parser error saying the function "isn't defined", as described in the PR's thread). So, that part isn't fully fixed yet.
Still, you can avoid most of that headache by just using on_notification, but worth mentioning that there's still a bit of work to do.

@Bromeon
Copy link
Member

Bromeon commented Oct 20, 2023

Thanks for following this up and testing!

However, if you explicitly override one of the virtual methods overridden by the extension, such as if you override _physics_process in the GDScript file, the original override of that particular method in the extension class won't run anymore, and calling super() from GDScript doesn't work (throws a parser error saying the function "isn't defined", as described in the PR's thread). So, that part isn't fully fixed yet.

I think that's a rather rare use case though. Also, the attached GDScript is technically not a subclass of the GDExtension class. So I'll close this, but we could document that limitation.

@Bromeon Bromeon closed this as completed Oct 20, 2023
@anrp
Copy link

anrp commented Oct 20, 2023

FYI I did dig a bit because I was curious, and I'm quite sure this is not supposed to work, however....

If you apply the following patch:

diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp
index 2439e5760c..bc303d9cc9 100644
--- a/modules/gdscript/gdscript_analyzer.cpp
+++ b/modules/gdscript/gdscript_analyzer.cpp
@@ -1743,7 +1743,7 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
                        }
 #ifdef DEBUG_ENABLED
                        if (native_base != StringName()) {
-                               parser->push_warning(p_function, GDScriptWarning::NATIVE_METHOD_OVERRIDE, function_name, native_base);
+                               // parser->push_warning(p_function, GDScriptWarning::NATIVE_METHOD_OVERRIDE, function_name, native_base);
                        }
 #endif
                }
@@ -3299,7 +3299,7 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
                // If the method is implemented in the class hierarchy, the virtual flag will not be set for that MethodInfo and the search stops there.
                // Virtual check only possible for super() calls because class hierarchy is known. Node/Objects may have scripts attached we don't know of at compile-time.
                if (p_call->is_super && method_flags.has_flag(METHOD_FLAG_VIRTUAL)) {
-                       push_error(vformat(R"*(Cannot call the parent class' virtual function "%s()" because it hasn't been defined.)*", p_call->function_name), p_call);
+                       // push_error(vformat(R"*(Cannot call the parent class' virtual function "%s()" because it hasn't been defined.)*", p_call->function_name), p_call);
                }

                // If the function requires typed arrays we must make literals be typed.

(which just prevents it from stopping processing on errors - a serious hack)

and you register _process again...

godot::ClassDB::bind_method(godot::D_METHOD("_process"), &GDExample::_process);

which causes the engine to complain

ERROR: Method 'GDExample::_process()' already registered as non-virtual.
   at: bind_virtual_method (external/godot-cpp/src/core/class_db.cpp:334)

and then you override in GDScript...

# Called every frame. 'delta' is the elapsed time since the previous frame.
func _process(delta):
  print(\"godot process\")
  super(delta)

then it works as you have expected: engine calls script which calls the native class function.

(edit: sorry, I forget this is not a C++ repo - however it likely would work similarly.)

@Bromeon
Copy link
Member

Bromeon commented Oct 20, 2023

@anrp Thanks a lot for looking deeper into this!

Could you post this in godotengine/godot-cpp#1022?
Here, none of the Godot maintainers will see it.

@PgBiel
Copy link
Contributor

PgBiel commented Oct 20, 2023

I think that's a rather rare use case though.

Well, I'd expect it to be very rare when developing GDExtension for one's own games, that's correct. But it might not be that rare when writing a GDExtension as a library for others to use. I guess it'd still be better to use on_notification in that case though. But further discussion should definitely be in a separate issue, if it does arise.

Also, the attached GDScript is technically not a subclass of the GDExtension class.

Hmm, maybe? I don't know much of the inner Godot architecture here, so I'll take your word for it. 🙂

Still, I guess it could be interesting to have super() work either way (at least, I wouldn't expect it to "bypass" the GDExtension class here). But this is something to be discussed in the Godot repo and not here. 🙂

So I'll close this, but we could document that limitation.

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: register Register classes, functions and other symbols to GDScript status: upstream Depending on upstream fix (typically Godot)
Projects
None yet
Development

No branches or pull requests

5 participants