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] Deprecate NOTIFICATION_MOVED_IN_PARENT #82248

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Sep 24, 2023

  • NOTIFICATION_MOVED_IN_PARENT makes node children management very inefficient.
  • Replaced by a NOTIFICATION_CHILD_ORDER_CHANGED (and children_changed signal).
  • Most of the previous tasks carried out by NOTIFICATION_MOVED_IN_PARENT are now done not more than a single time per frame.

This PR strictly speaking breaks compatibility, however this notification was very rarely used, even within the engine, and provides an alternate way to do the same. The need to fix this design issue was deemed more important than the likelihood of any breaking in master, and the same applies in 3.x.

Alternative to #74672
Backport of #75701

Example Project

Run before and after PR:
ChildOrderChangedBenchmark.zip
before 9069 milliseconds, after 894 milliseconds (approx 10x faster)

The example project stress tests moving sprite children, which causes large numbers of NOTIFICATION_MOVED_IN_PARENT. After the PR, these are reduced to once per frame, and the notification is replaced with a cheaper direct function call.

Explanation

The exponential notification problem remains one of the biggest inefficiencies / achilles heel in the engine in 3.x, so it makes sense to solve it properly, even if there may require some beta testing to iron out any regressions.

This is not a straight backport of #75701 (as 3.x works in a slightly different way) but is inspired to work in a similar(ish) way. There are some significant differences:

  • The storage of the dirty parent nodes in Viewport::GUI becomes a static rather than per viewport. This single list allows storing an ID within the list on the canvas_item, which gives O(1) behaviour rather than relying on a HashSet and slower lookup as in the 4.x version. It also allows easy flushing once per frame, rather than also flushing on each tick (which is unnecessary and potentially greatly increases the cost).
  • As with [3.x] Deferred NOTIFICATION_MOVED_IN_PARENT and register interest #74672, the dirty nodes store a minimum and maximum child to update. This makes the processing significantly more efficient when only a small subset of the children are moved. This optimization is not so readily available in master, due to the new way the children are stored.

Notes

  • Like the version in master, this is (strictly speaking) compatibility breaking, because it avoids sending the NOTIFICATION_MOVED_IN_PARENT completely. This notification is no longer required in core, but there is the possibility that third parties may rely on it, and need to switch to the new NOTIFICATION_CHILD_ORDER_CHANGED. This is however unlikely to be widely used.
  • We could still send the old notification if breaking compatibility was a major concern, however this would lose a lot of efficiency. This could for instance be switched on by means of a project setting. However this may be best to avoid unless there are regressions requiring it.
  • Inspired by Deprecate NOTIFICATION_MOVED_IN_PARENT for NOTIFICATION_CHILD_ORDER_CHANGED #75701 this uses simple casting during iteration to determine whether a child needs an update, rather than marking nodes with a flag (the approach used in [3.x] Deferred NOTIFICATION_MOVED_IN_PARENT and register interest #74672).
  • This potentially makes the CanvasLayer updates more efficient because it ensures only a single canvas layer order update per frame.

Follow up

After we decide whether an approach such as this or the alternative PR is desired, there is another significant optimization which can be made in a followup PR, and possibly also applied in master. Much of the remaining inefficiency is in multiple calls to VisualServer::canvas_item_set_draw_index(), where in some cases hundreds or thousands of calls may take place. It seems to make sense to replace this by a single call to an extended version of the function which can change the draw indices of all the items in a single batch.

Discussion

Personally overall I would be tempted to go with this approach over #74672 . This is now closer to master, and is more efficient because it calls update_draw_order() directly instead of going through the heavyweight notifications.
Although it technically breaks compatibility, this is likely to be a rare case, which can either be worked around with the new notification, or we can add an optional legacy mode to call the old notification.
Additionally it should be significantly faster than the version in master because it only updates the range of children changed, rather than all the children every time.

Update

Due to concerns over applications using custom MainLoop, I moved the storage of the dirty list from the SceneTree to statics within Viewport. This also makes more sense architecturally as the 2D is more localized with other GUI data, and SceneTree is not associated with any particular node type.

@lawnjelly lawnjelly added this to the 3.6 milestone Sep 24, 2023
@lawnjelly lawnjelly marked this pull request as ready for review September 27, 2023 16:05
@lawnjelly lawnjelly requested review from a team as code owners September 27, 2023 16:05

if (parent) {
parent->connect("child_order_changed", this, "_order_changed_in_parent");
}
Copy link
Member Author

@lawnjelly lawnjelly Sep 27, 2023

Choose a reason for hiding this comment

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

This could potentially directly connect the skeleton as in master with a ref counted connection, however doing it via the Bone2D as before the PR is less error prone (and besides this is more likely to occur in editor than in game so may not be a bottleneck).

Could possibly do a direct skeleton connection in follow up PR.

if (is_inside_tree()) {
tree = get_tree();
} else {
tree = SceneTree::get_singleton();
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 is questionable whether we need a full update when the node is not within the main scene tree, the capacity to send CHILD_ORDER_CHANGED outside main scene tree is included for safety for now.

@lawnjelly
Copy link
Member Author

Have assigned Juan or Pedro for review, am struggling here because there's not a lot of us who are familiar with this part of core (and less so in 3.x). But anyone else is welcome to review.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Documentation matches master. It would be nice not to have this PR be forgotten to time, as I feel like 3.6 users would benefit from this a lot, too.

doc/classes/Node.xml Outdated Show resolved Hide resolved
scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
scene/2d/canvas_item.cpp Outdated Show resolved Hide resolved
scene/main/scene_tree.h Outdated Show resolved Hide resolved
@lawnjelly lawnjelly force-pushed the child_order_changed branch from 6105088 to dba288d Compare January 27, 2024 19:11
@lawnjelly lawnjelly force-pushed the child_order_changed branch from dba288d to e00a26e Compare February 6, 2024 08:43
@lawnjelly lawnjelly marked this pull request as draft February 6, 2024 08:45
@lawnjelly lawnjelly force-pushed the child_order_changed branch from e00a26e to bee3243 Compare February 6, 2024 09:20
@lawnjelly lawnjelly marked this pull request as ready for review February 6, 2024 09:22
@akien-mga akien-mga changed the title [3.x] Deprecate NOTIFICATION_MOVED_IN_PARENT [3.x] Deprecate NOTIFICATION_MOVED_IN_PARENT Apr 19, 2024
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.

Let's give it a spin!

@lawnjelly lawnjelly force-pushed the child_order_changed branch 2 times, most recently from bbdb0cb to 7131be6 Compare April 20, 2024 04:25
@lawnjelly lawnjelly marked this pull request as draft April 20, 2024 04:45
@lawnjelly lawnjelly marked this pull request as ready for review April 20, 2024 05:14
* NOTIFICATION_MOVED_IN_PARENT makes node children management very inefficient.
* Replaced by a NOTIFICATION_CHILD_ORDER_CHANGED (and children_changed signal).
* Most of the previous tasks carried out by NOTIFICATION_MOVED_IN_PARENT are now done not more than a single time per frame.

This PR breaks compatibility (although this notification was very rarely used, even within the engine), but provides an alternate way to do the same.
@lawnjelly lawnjelly force-pushed the child_order_changed branch from 7131be6 to d56d1ff Compare April 20, 2024 06:54
@lawnjelly lawnjelly merged commit fae7079 into godotengine:3.x Apr 20, 2024
14 checks passed
@lawnjelly lawnjelly deleted the child_order_changed branch April 20, 2024 07:15
@lawnjelly
Copy link
Member Author

I did a final pass last night / this morning (as I had a rebase disaster and had to look over it minutely anyway 😁 ), and spent a while doing as much testing as possible. I spotted a couple of minor bugs which are now fixed.

I confirmed this gives a major speed up in some node operations, but it is a significant change so will be watching for regressions particularly in controls. Hopefully the next beta will give this some decent testing in the wild.

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.

4 participants