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

Make NOTIFICATION_MOVED_IN_PARENT opt-in. #70265

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zylann
Copy link
Contributor

@Zylann Zylann commented Dec 18, 2022

Makes it faster to remove non-2D nodes from the scene tree when they have lots of siblings.

This partly solves #61929 for non-2D nodes, with a very simple change. This is not exclusive to possible improvements mentionned in #61929 (comment) (although so far they are for Godot 3).
Only 2D nodes use this notification so I thought it made sense if that notification wasn't always sent to others in the first place. It was causing stalls in my 3D game.

@Zylann Zylann requested a review from a team as a code owner December 18, 2022 17:17
@lawnjelly
Copy link
Member

This is the bitflags approach I was referring to in #65581 (comment) . (Except using a single bool because only supporting one notification type).

To quote from my post there:

Another idea is for nodes to register their interest in a notification type (via a set of bitflags), and check this prior to sending the more heavyweight notification.

Although it's an improvement on the before case, there are a couple of problems:

  • It is compat breaking, as users have to register an interest (if they e.g. create new nodes in a module that want to respond to particular notifications etc)
  • It doesn't prevent the notification being sent multiple times to nodes that are interested.

That said, registering for interest is a worthy option that can be used in addition to other approaches e.g. #65581 and #62444.

Makes it faster to move/remove non-2D nodes from the scene tree.
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.

3 participants