godot icon indicating copy to clipboard operation
godot copied to clipboard

Deferred NOTIFICATION_MOVED_IN_PARENT and register interest

Open lawnjelly opened this issue 1 year ago • 0 comments

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.

Alternative to #65581 Helps address #61929

Notes

  • As currently the only notification responsible for "notification explosion" problems, this is a more targetted alternative to the approach in #65581
  • I first mentioned this approach in https://github.com/godotengine/godot/issues/61929#issuecomment-1354351264 .
  • 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 https://github.com/godotengine/godot/pull/65581#issue-1368085497 , which @Zylann later made a PR for 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.

lawnjelly avatar Mar 09 '23 17:03 lawnjelly