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

Add NOTIFICATION_PREDELETE_CLEANUP notification to fix C# Dispose() #83670

Merged

Conversation

raulsntos
Copy link
Member

New notification sent after NOTIFICATION_PREDELETE to let Objects cleanup at the very end, it should be the last notification sent.

NOTIFICATION_PREDELETE_CLEANUP is used by C# scripts to cleanup (calls Dispose()), instead of NOTIFICATION_PREDELETE. This ensures that it's the last thing we do, after disposing we can't do anything else with the C# instances.

New notification sent after `NOTIFICATION_PREDELETE` to let Objects cleanup at the very end, it should be the last notification sent.
@raulsntos raulsntos added this to the 4.2 milestone Oct 20, 2023
@raulsntos raulsntos requested review from a team as code owners October 20, 2023 11:45
@@ -801,6 +801,8 @@ class Object {
NOTIFICATION_POSTINITIALIZE = 0,
NOTIFICATION_PREDELETE = 1,
NOTIFICATION_EXTENSION_RELOADED = 2,
// Internal notification to send after NOTIFICATION_PREDELETE, not bound to scripting.
NOTIFICATION_PREDELETE_CLEANUP = 3,
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be clearly reserved so future notifications don't use it, especially if we use a system based on registered values to track overlap

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems we use different magnitudes1 to avoid overlap (e.g.: the Node notifications are 1X, CanvasItem notifications are 3X, Control/Node3D notifications are 4X, Window notifications are 10XX). But I agree it can be error prone.

Footnotes

  1. Not sure if magnitude is the right word.

@reduz
Copy link
Member

reduz commented Nov 8, 2023

I think this is a good solution and I am all for solving this problem and move forward. I think the following should be considered.

  • These are hacks that keep piling up because Mono is not using extension API, otherwise this could be solved entirely within extension API, either with something existing or new.
  • As this is basically a temporary hack until we have the time and resources to port Mono to extension API, I suggest not exposing this notification to script (remove the bind constant) and use it internally from Mono, then we remove it when we deprecate the non extension mono sometime in the far future.

EDIT: I apologize, just realized you are not actually binding this, so its fine as is.

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.

Discussed with @reduz, seems good as a stopgap measure for the C# predelete, until we can have a more robust system.

Then we would remove that new notification - so PR reviewers should be aware of not letting it be exposed/used more internally, unless there's a very clear use case validated by the core team.

@akien-mga akien-mga changed the title Add NOTIFICATION_PREDELETE_CLEANUP notification Add NOTIFICATION_PREDELETE_CLEANUP notification to fix C# Dispose() Nov 9, 2023
@akien-mga akien-mga merged commit ce53362 into godotengine:master Nov 9, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the notification-predelete-cleanup branch November 9, 2023 12:48
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.

C#: Script methods bound to tree_exited / _ExitTree() can't be found.
4 participants