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

Fix NOTIFICATION_POSTINITIALIZE sent twice to native parent classes #1447

Merged
merged 1 commit into from
May 17, 2024

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Apr 24, 2024

As brought up on PR godotengine/godot#91018, we're currently sending NOTIFICATION_POSTINITIALIZE to the native parent class twice.

This issue was introduced by PR #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 godotengine/godot#91018 and godotengine/godot#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.

@dsnopek dsnopek added bug This has been identified as a bug cherrypick:4.1 cherrypick:4.2 labels Apr 24, 2024
@dsnopek dsnopek added this to the 4.3 milestone Apr 24, 2024
@dsnopek dsnopek requested a review from a team as a code owner April 24, 2024 18:13
@dsnopek dsnopek modified the milestones: 4.3, 4.x Apr 24, 2024
@dsnopek dsnopek changed the title Fix NOTIFICATION_POSTINITIALIZE sent twice to native parent class Fix NOTIFICATION_POSTINITIALIZE sent twice to native parent classes Apr 24, 2024
@dsnopek dsnopek force-pushed the avoid-double-postinitialize branch from 030f3e6 to 06373ce Compare April 24, 2024 18:22
@AThousandShips
Copy link
Member

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

@dsnopek
Copy link
Collaborator Author

dsnopek commented Apr 24, 2024

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 #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
Copy link
Member

AThousandShips commented Apr 24, 2024

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
Copy link
Collaborator Author

dsnopek commented Apr 24, 2024

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
Copy link
Contributor

Naros commented Apr 25, 2024

Nice catch @dsnopek !

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved in the GDExtension meeting.

@dsnopek dsnopek merged commit b697ba8 into godotengine:master May 17, 2024
12 checks passed
@dsnopek
Copy link
Collaborator Author

dsnopek commented May 17, 2024

Cherry-picked for 4.2 in PR #1465

@dsnopek
Copy link
Collaborator Author

dsnopek commented May 17, 2024

Cherry-picked for 4.1 in PR #1466

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 this pull request may close these issues.

5 participants