godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.7k stars 528 forks source link

Fix NOTIFICATION_POSTINITIALIZE sent twice to native parent classes #1447

Closed dsnopek closed 4 months ago

dsnopek commented 5 months ago

As brought up on PR https://github.com/godotengine/godot/pull/91018, we're currently sending NOTIFICATION_POSTINITIALIZE to the native parent class twice.

This issue was introduced by PR https://github.com/godotengine/godot-cpp/issues/1269, which was attempting to get the NOTIFICATION_POSTINITIALIZE to arrive to the extension class by sending it in Wrapped::_postinitialize(), but the way it's doing it will send the notification through the native parent class again.

This PR makes it so that Wrapped::_postinitialize() will only send NOTIFICATION_POSTINITIALIZE through the class hierarchy that exists purely on the GDExtension side. Because NOTIFICATION_POSTINITIALIZE is run on the parent class first, this effectively just picks up where the previous notification that went through the native parent class heirarchy stopped.

PR https://github.com/godotengine/godot/pull/91018 and https://github.com/godotengine/godot/pull/91019 attempt to more comprehensively fix NOTIFICATION_POSTINITIALIZE, but those require changing the GDExtension interface (which is Godot 4.4 material at this point), and we need a fix to this issue that can be used with Godot 4.1, 4.2 and 4.3.

AThousandShips commented 5 months ago

We should be able to test for this with a flag and a notification check, just to validate it

dsnopek commented 5 months ago

We should be able to test for this with a flag and a notification check, just to validate it

There are already automated tests for ensuring that NOTIFICATION_POSTINITIALIZE goes to the extension class - they were added in PR https://github.com/godotengine/godot-cpp/issues/1269 and they still pass with this PR :-)

I don't think there's a way for us to test that the bug this PR is fixing is fixed, though, since that all happens on the Godot side. So, I don't think this PR needs to add any more tests.

AThousandShips commented 5 months ago

Meant like adding something in the existing check so:

    if (p_what == NOTIFICATION_POSTINITIALIZE) {
        if (post_initialized) {
            // Some error.
        }
        post_initialized = true;
    }

Since the issue was sending it twice yes?

Unless it's only sent to the parent twice

dsnopek commented 5 months ago

Unless it's only sent to the parent twice

It's only sent to the native parent class twice. On the godot-cpp side, we only see it once.

Naros commented 5 months ago

Nice catch @dsnopek !

dsnopek commented 4 months ago

Cherry-picked for 4.2 in PR https://github.com/godotengine/godot-cpp/pull/1465

dsnopek commented 4 months ago

Cherry-picked for 4.1 in PR https://github.com/godotengine/godot-cpp/pull/1466