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

GDScript disabling GDExtension node methods from running #1224

Closed
DarkyMago opened this issue Aug 28, 2023 · 5 comments · Fixed by godotengine/godot#83583
Closed

GDScript disabling GDExtension node methods from running #1224

DarkyMago opened this issue Aug 28, 2023 · 5 comments · Fixed by godotengine/godot#83583
Labels
bug This has been identified as a bug
Milestone

Comments

@DarkyMago
Copy link

DarkyMago commented Aug 28, 2023

Godot version

4.1 (bd6af8e0e)

godot-cpp version

4.1 (28494f0)

System information

Windows 10, amd R5 5600X

Issue description

Following signal example of gdextensions tutorial, there seems to be a conflict making extension c++ extension class not working when adding a script to the node.
More precisely, having a script on the node seems to block gdextensions functions to execute.

I don't know if it's problem of :

  • docs not mentioning a known cave-at
  • gdextension not binding correctly when there's a script and in this case
    • if it's in godot-cpp
    • or godot management of gdextensions in general

Putting the script (and signal target) on another node seems to circumvent the issue.

Steps to reproduce

  1. have a node with a gdextended class.
  2. put a script on it
  3. run

Minimal reproduction project

godot_tutos.zip

@DarkyMago
Copy link
Author

I have a feeling it's related to #1022 but am not sure

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 7, 2023

Thanks!

I think there might be a couple things going on here:

  1. For virtual methods like _process(), they can only be implemented by script or GDExtension, not both (and scripts always win if both implement the same virtual method). This is also more-or-less true of modules as well, where the solution is that module code in C++ implements _notification() (rather than _process()) and scripts use _process(). So, in a GDExtension, you should also use _notification()
  2. Due to the above, I think we should update the docs to use the module-like approach of using _notification() rather than the script-like approach of using _process(), because I've seen this come up a couple of times
  3. However, looking at the code in the MRP (I haven't actually tested it yet) it doesn't seem like that's what's happening in this case. The GDExtension does implement _process() but the script attached to the same node doesn't, it has only a single normal (non-virtual) method, so I would have thought that _process() from GDExtension would continue to run fine. This part needs more investigation!

@DarkyMago
Copy link
Author

DarkyMago commented Sep 7, 2023

The GDExtension does implement _process() but the script attached to the same node doesn't, it has only a single normal (non-virtual) method

that's it and that is why I found this really weird.

I've checked if the binding methods do get called just in case and they are.
I fear it is the GDscript presence nullifying standard methods from GDExtensions such as _ready and _process as if those where defines as either _process() : pass or the method from GDExtension parent (don't know if any native classes have a process / ready function that can be bypassed to check, I don't know godot well enough yet).

@anrp
Copy link

anrp commented Oct 18, 2023

The logic in (the template strings of) https://github.com/godotengine/godot/blob/master/core/object/make_virtuals.py#L9 almost certainly is this issue...that if there is a script it doesn't look at the extension at all (vs just if the script has the method, I think it'd be CallError::CALL_ERROR_INVALID_METHOD but I haven't looked, probably better to use script_instance->has_method)

@anrp
Copy link

anrp commented Oct 18, 2023

I haven't tested ^ that yet but it may fix this...

@akien-mga akien-mga added this to the 4.2 milestone Oct 20, 2023
@akien-mga akien-mga added the bug This has been identified as a bug label Oct 20, 2023
ProbablyWorks pushed a commit to ProbablyWorks/godot that referenced this issue Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants