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

[3.x] Deferred NOTIFICATION_MOVED_IN_PARENT and register interest #74672

Closed

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Mar 9, 2023

Adds the ability to defer sending NOTIFICATION_MOVED_IN_PARENT to the next flush, instead of sending notifications immediately. This system allows the prevention of duplicate notifications on the same frame, which can result in large numbers of notifications and slowdown.

For efficiency, stores the range of children moved on the parent node, rather than storing each child individually.

Additionally 2D nodes register an interest in observing NOTIFICATION_MOVED_IN_PARENT, this is tested before sending the final notification.

Use in conjunction with #65581
Helps address #61929 (specifically the non-deletion cases)

Flushes

As this PR is only concerned with MOVED_IN_PARENT I've experimentally tried only flushing the notifications once prior to rendering, rather than every physics tick as in #65581 . The notification is used in three areas, and I'm not convinced yet it requires flushing except prior to rendering (and no testing so far suggests this).

Given that it is easy enough to add the flushes in during physics tick (one line), it is probably worth trying this out for a beta with a single flush. The reasoning is that we do flush each tick, if items are moved several times during multiple physics ticks before a frame, their canvas_item order ID will be sent multiple times (needlessly) to VisualServer.

Notes

  • As currently the only notification responsible for "notification explosion" problems, this is a more targetted alternative to the approach in [3.x] Add deferred notifications #65581
  • I first mentioned this approach in Deleting / detaching / moving large numbers of child nodes can be excessively slow #61929 (comment) .
  • This has the advantage that a range is stored on the parent node, instead of individually checking each child node whether it already has a deferred notification pending. This is more efficient for this particular notification, but in return it doesn't work in the general case, hence the difference between the two PRs (deferred notifications is more general if any future notifications required the same).
  • This also includes the check for interest before sending the notification that I first mentioned in [3.x] Add deferred notifications #65581 (comment) , which @Zylann later made a PR for in Make NOTIFICATION_MOVED_IN_PARENT opt-in. #70265 . On reflection, it does seem worth using in conjunction with the other methods, because the notification() call is rather heavyweight, calling functions on each class in the inheritance hierarchy as well as potentially script.
  • I did have some concerns that checking for interest may not be backward compatible, however it does seem unlikely that many users will have created 2D nodes outside of inheriting canvas_item. Even in this case in a heavily modded version of Godot, then a call to _set_observe_notification_moved_in_parent() is enough to re-establish the old behaviour, so on balance it seems worth doing, given the benefits.

Performance

As with #65581 this drastically reduces notifications, and even more so because of the range optimization specific to NOTIFICATION_MOVED_IN_PARENT.

Which alternative

Both versions address the problem pretty well - #65581 is more generic and future proof, whereas this PR is more efficient but only designed for this single problematic notification.
Personally I would probably be tempted to go for this version, as we are unlikely to need more deferred notification types in 3.x, and it should be more efficient.

@lawnjelly lawnjelly requested review from a team as code owners March 9, 2023 17:08
@lawnjelly lawnjelly added this to the 3.6 milestone Mar 9, 2023
@lawnjelly lawnjelly force-pushed the deferred_moved_in_parent_range branch 2 times, most recently from 7597c0e to ac5f91a Compare March 11, 2023 07:58
@lawnjelly lawnjelly changed the title Deferred NOTIFICATION_MOVED_IN_PARENT and register interest [3.x] Deferred NOTIFICATION_MOVED_IN_PARENT and register interest Mar 11, 2023
Adds the ability to defer sending NOTIFICATION_MOVED_IN_PARENT to the next flush, instead of sending notifications immediately. This system allows the prevention of duplicate notifications on the same frame / tick, which can result in large numbers of notifications and slowdown.

For efficiency, stores the range of children moved on the parent node, rather than storing each child individually.

Additionally 2D nodes register an interest in observing NOTIFICATION_MOVED_IN_PARENT, this is tested before sending the final notification.
@lawnjelly
Copy link
Member Author

Closing in favour of #82248 . If we decide that is too compat breaking, I can reopen this.

@lawnjelly lawnjelly closed this Oct 3, 2023
@AThousandShips AThousandShips removed this from the 3.6 milestone Nov 1, 2023
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