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

Remove unused NOTIFICATION_NODE_RECACHE_REQUESTED notification #84419

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Nov 3, 2023

This is a follow-up to #82471. The notification was never used by the engine since the moment it was introduced in Godot 4 during its closing days of the development (#57606). It was sent, but never received by anything. While #82471 attempted to finally utilize it, the consensus was that the notification was a mistake and we should resolve the issue another way. Which we did.

So now the notification can be removed. This breaks compatibility, but there is no reason to assume users would rely on it in practice.

It also conflicts with some other notifications (see #75839), making it a problem for some GDExtensions.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Looks good from me. I was part of the discussions about the re-caching bugs in the past.

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

This change doesn't break binary compatibility in C#, only source compatibility, because it removes a constant field.

@YuriSizov YuriSizov force-pushed the core-remove-orphan-notification branch from 5481cca to 623b905 Compare November 6, 2023 11:16
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good.

@YuriSizov YuriSizov merged commit b7449a3 into godotengine:master Nov 6, 2023
15 checks passed
@YuriSizov
Copy link
Contributor Author

Thanks for reviews!

@YuriSizov YuriSizov deleted the core-remove-orphan-notification branch November 6, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants