Account for physics interpolation and transform snapping when Y-sorting

[kleonc]

trafficstars

Makes the transforms used for Y-sorting canvas items match the transforms used for such items in RendererCanvasCull::_cull_canvas_item, aka it accounts for physics interpolation and transform snapping.

Fixes #92982.

This is done in the first commit as a simple change which results in calculating physics interpolation and transform snapping twice for each y-sorted item: when determining transforms used for y-sorting while gathering to-be-y-sorted items, and when actually handling them after they're y-sorted (also note that calculating transforms twice introduces a chance of using different transforms for sorting/rendering in case there's some mismatch in calculations).

---

The second commit makes the transform calculations (physics interpolation, transform snapping) happen once for each y-sorted item, before y-sorting (so it's sorted according to the position used later for rendering).

For reviewing the second commit I recommend looking at combined first+second commits diff, should be simpler.

In more detail:

RendererCanvasCull::Item::ysort_xform was previously storing a transform from the parent of the given item to the y-sorted subtree's root, now it's changed to store a transform from the given item to the y-sorted subtree's root instead. Meaning previously y-sorted item's final transform was ysorted_subtree_root.final_xform * item.ysort_xform * item.self_xform, now it's ysorted_subtree_root.final_xform * item.ysort_xform as item.self_xform is calculated before y-sorting and it's included into item.ysort_xform to not need to recalculate it later.

As a result RendererCanvasCull::Item::ysort_pos was removed as no longer needed, now item.ysort_xform.origin is the exact same position (the item's position in y-sorted subtree's root coordinate space) which can be used for sorting instead.

Renamed RendererCanvasCull::_cull_canvas_item's p_allow_y_sort parameter to p_is_already_y_sorted as it seemed more intuitive to me, given it's usage (and negated the passed value everywhere).

So now when p_is_already_y_sorted == true the item's final transform was already calculated during y-sorting, and it's passed as p_parent_xform to RendererCanvasCull::_cull_canvas_item (kinda disliking this naming/behavior discrepancy but oh well). Thus in such case self/local transform is not being recalculated, and thus it's not available (only the final transform is passed).

Also extracted the count-only part of RendererCanvasCull::_collect_ysort_children into separate RendererCanvasCull::_count_ysort_children as with the transform calculations added it seemed too "ify". And moved _mark_ysort_dirty into RendererCanvasCull as well.

I guess that's it, feel free to ask for any further explanation/clarification in case something is unclear.

Oh, and I'd understand if the second commit is considered too risky for cherry-picking into 4.3.stable etc. so of course I could split it into 2 PRs or whatever else is desired.

[lawnjelly]

I'll take a look.

I vaguely remember doing something to fix y-sorting at some point but maybe it's not working in 3.x too, I'll check the issue and see if I can reproduce in 3.x.

EDIT: I can't seem to reproduce so far in 3.x, but then the Y sort works quite differently there. (I'm not super familiar with how 4.x does y sorting.)

@rburing welcome to look as he is maintaining 4.x (I can't guarantee I can spend much time on this). Key things is probably making sure it isn't doing more work than necessary, ideally shouldn't be interpolating transforms twice.

[kleonc]

I can't seem to reproduce so far in 3.x, but then the Y sort works quite differently there. (I'm not super familiar with how 4.x does y sorting.)

IIRC in 3.x the canvas item with sort_y = true is treated as a grouping-only item, it's not drawn itself (the VisualServer implementation seems to be done specifically with YSort node in mind, assuming YSort doesn't draw itself - hence e.g. #49165). I'd guess that's something about that. But yeah, I haven't looked in detail into 3.x in quite a while (we complement each othere here :smile:).

Key things is probably making sure it isn't doing more work than necessary, ideally shouldn't be interpolating transforms twice.

Yes, currently it indeed does interpolate twice per y-sorted item, I'm well aware of that and I do dislike it. Was focusing on conceptually fixing the transform calculations here (in fact I haven't tested it much in action, only in the MRP from #92982; was fixing purely from the code standpoint; so testing further welcomed). But I think I already know how to avoid this double computation, later today I will probably push a second commit in here (such change seems more regression prone so I think leaving it as a separate commit would make sense).

[kleonc]

Added second commit (see updated description) and rebased against master (to include #92763).

BTW @markdibarry have you tested how/if y-sorting works with parallax? :thinking: I haven't and am just curious. :upside_down_face: In any case in this PR I just tried to ensure the parallax calculations remain the same as before.

[markdibarry]

@kleonc Oof! I never even imagined anyone would use a parallax effect with y-sorting... and I still can't think of a use case, but I'm sure someone can and will be very upset! I tried it on the master branch and it seems to remove all repeats due to _cull_canvas_item() using a completely different culling approach when y-sorting and (from the look of it) removing all parent/child relationships... which it seems the repeated canvas item's culling boundaries relies on. It seems as though we'll need to:

1. Think of a new way to cull the repeats
2. Think of a new way to cull while y-sorting
3. Say it's not supported for now and think of it later.

[kleonc]

@markdibarry Firstly let's clarify of what behavior of y-sorting + repeating we're talking.

y-sorted no repeat	(1) repeats y-sorted separately	(2) repeats y-sorted as one item together with the original one

Knowing a little how these work I intuitively was thinking about (2) but I guess someone could expect (1).

(1) seems not feasible performance-wise to me, as it would require each repeat to basically become its own item to be ordered separately (thus in case of items' different materials more batching breaking etc.; way costlier/slower).

(2) should be easy to achieve. Actually it already works in some cases. :upside_down_face:

Also given it's questionable whether it has a use case I'd

I tried it on the master branch and it seems to remove all repeats due to _cull_canvas_item() using a completely different culling approach when y-sorting and (from the look of it) removing all parent/child relationships...

I've looked into the repeating code and main issue is that for all y-sorted items _cull_canvas_item() is called with repeat_size and repeat_times of the y-sorted subtree's root item. While y-sorted repeat source items (repeat_source == true) will override these for themselves, their original children won't get such values. And that's pretty much all what's preventing (2) from just working.

Meaning it already works e.g. if Parallax2D is the first y-sorted node (the root of the y-sorted subtree) and there are no nested Parallax2Ds involved etc.

In v4.3.beta1.official [a4f2ea91a]:

Fixing it would be pretty much the same as what's done in this PR, aka it's a matter of propagating proper repeat_size and repeat_times when preparing y-sorting, and skip the current code determining these in case we're dealing with an already y-sorted item.

There are some other little issues with the repeating I can see in code, but it's offtopic here.

In general I dislike how now things need to be duplicated codewise to be done before y-sorting, so I'm thinking _cull_canvas_item should likely be splitted / refactored a little, but this I consider as for after 4.3. But again, out of scope of this PR, it was meant to be asimple fix. :upside_down_face:

[markdibarry]

@kleonc Thanks for the advice! Made an issue and PR here: #93182

[kleonc]

Rebased (#93182 was merged).

[akien-mga]

Needs a rebase, and then an approval would be great :)

[kleonc]

Rebased (pretty much no changes compared to the pre-rebase version).

[rburing]

~~Besides the above comment~~ the code looks fine to me and it fixes the issue.

[lawnjelly]

Am happy that @rburing has checked properly, I've had a little look through and it looked fine but I didn't follow it completely. :grin:

[Repiteo]

Thanks!

[KeyboardDanni]

Hmm, this change replaces the floor + 0.5 math with round, basically undoing #93740. Was this intended?

[kleonc]

Hmm, this change replaces the floor + 0.5 math with round, basically undoing #93740. Was this intended?

Absolutely not. It was like that originally and I must have overlooked this while rebasing, my bad. Thanks for spotting it out!

[kleonc]

Note: if/when cherry-picking, should be done together with #98195.