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

Object does not dispatch NOTIFICATION_POSTINITIALIZE #1269

Closed
Naros opened this issue Oct 13, 2023 · 7 comments · Fixed by #1321
Closed

Object does not dispatch NOTIFICATION_POSTINITIALIZE #1269

Naros opened this issue Oct 13, 2023 · 7 comments · Fixed by #1321
Labels
bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation
Milestone

Comments

@Naros
Copy link
Contributor

Naros commented Oct 13, 2023

Godot version

4.2

godot-cpp version

4.2

System information

Windows 11

Issue description

When a custom resource is defined, such as:

class MyResource : public Resource {
  GDCLASS(MyResource, Resource);
};

The _notification method is never dispatched with NOTIFICATION_POSTINITIALIZE like it is in Godot / GDScript.

Steps to reproduce

Create MyResource as outlined above using:

Ref<MyResource> r;
r.instantiate();

or using memnew

MyResource *r = memnew(MyResource);

In neither case is the NOTIFICATION_POSTINITIALIZE fired.

Minimal reproduction project

N/A

@Calinou Calinou added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation labels Oct 13, 2023
@dsnopek
Copy link
Collaborator

dsnopek commented Oct 14, 2023

Thanks!

I'm not 100% sure if this should be handled on the godot-cpp side or on the Godot side, so this will need some investigation and discussion.

@Naros
Copy link
Contributor Author

Naros commented Oct 14, 2023

That's reasonable, hopefully, it's something we can get into 4.2 :)

@Naros
Copy link
Contributor Author

Naros commented Oct 26, 2023

Hi @dsnopek, I'm curious if you have had a chance to see what the impact/effort is for this?

@dsnopek
Copy link
Collaborator

dsnopek commented Oct 26, 2023

Well, it's really simple to fix on the godot-cpp side - I think it'd just be adding one line of code. :-)

But what I need to look into is if it's possible/desirable to fix this on the Godot side instead. If we can fix it on the Godot side, then all bindings will be fixed, rather than each having to do their own fix for this issue.

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 16, 2023

Discussed at the GDExtension meeting, and we favored having the GDExtension bindings dispatching NOTIFICATION_POSTINITIALIZE (rather than having the engine attempt to do it automatically), since there isn't a good place to do it that wouldn't have potential issues. For example, we discussed having Object::set_instance_binding() do it, but developers wouldn't necessarily expect that function to have side effects, and in some language bindings, they might not be finished initializing the object at that point. This does mean that we would need to document that all language bindings need to do this.

It would be helpful to have more feedback from other language binding maintainers, /cc @Bromeon

@Bromeon
Copy link
Contributor

Bromeon commented Nov 19, 2023

I guess that makes sense, and I'm fine with the extension dispatching NOTIFICATION_POSTINITIALIZE.

But how do people learn about this? Where would be a good place to document it?
Without seeing this issue, I'd likely never have known this is something we should do 😉

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 19, 2023

We probably need a "how to create GDExtension bindings" document, but that'll be a tricky thing to start given that there's so much to write and we probably can't get it all in one pass. Maybe it could be a "tips for creating GDExtension bindings" first?

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 topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants