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

Custom classes can't pass themselves into calls to other Godot APIs in their constructor #1039

Open
dsnopek opened this issue Feb 14, 2023 · 1 comment · May be fixed by #1446
Open

Custom classes can't pass themselves into calls to other Godot APIs in their constructor #1039

dsnopek opened this issue Feb 14, 2023 · 1 comment · May be fixed by #1446
Labels
bug This has been identified as a bug

Comments

@dsnopek
Copy link
Contributor

dsnopek commented Feb 14, 2023

In SG Physics 2D, I'm using this pattern where the physics nodes pass this into function calls on the physics server in their constructors, for example:

SGCollisionObject2D::SGCollisionObject2D(RID p_rid) {
	SGPhysics2DServer *physics_server = SGPhysics2DServer::get_singleton();
	physics_server->collision_object_set_data(p_rid, this);
}

However, this causes lots of problems inside the physics server, when it tries to use the object with any Godot APIs, because the object itself hasn't had it's instance and instance binding set on the Godot engine side yet - that happens after the constructor is finished, in Wrapped::_postinitialize().

(One example of something that breaks: if you store the Object * in a Variant, and then take an Object * back out, it will be a new, different Object *! So, if the function you call in the constructor happens to take a Variant, it will do weird things, including possibly segfault.)

I'm not sure what the solution could be?

It would be great if the Wrapped constructor could set the instance and instance binding, because parent constructors are always run before their child classes constructors, but I'm not sure this wouldn't cause other problems?

It was PR #663 that originally moved this to Wrapped::_postinitialize(). Before that, it was done by generating a constructor in in the GDCLASS() macro, which we definitely don't want to go back to, as that blocked custom classes from making custom constructors altogether.

@Faless
Copy link
Contributor

Faless commented Feb 15, 2023

I think this might be a tricky one, because there are basically 2 scenarios:

  1. The object is instantiated from godot-cpp, so first Wrapped is created, then postinitialize sets the instance bindings
  2. The object is instantiated from godot, then passed to godot-cpp, so godot-cpp needs to instantiate the Wrapped object via the bindings callback (meaning, it should not then re-set the bindings).

Those 2 scenarios need to be handled differently to avoid re-creating bindings (#679).

It seems that if we set the bindings in the constructor, we can't differentiate between the 2 cases. See #798 .

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.

3 participants