Fixing api for how transforms are accessed through Node2D and CanvasItem

[jordo]

This PR fixes some API strangeness that falls under how transforms are addressed in CanvasItem and Node2D classes (with side considerations for the Control class).

The first function is get_relative_transform_to_parent() in Node2D. This function actually takes one input parameter (a target node), and traverses up the scene tree multiplying transforms until it hits the target node. This is clearly a relative to ancestor function and I renamed as so.

Likewise, similar to get_global_transform() (which is in CanvasItem), get_relative_transform_to_ancestor() should be in CanvasItem NOT Node2D. It only reads get_transform(), which is available in CanvasItem as pure-virtual function. This function should be accessable to all CanvasItems, not just a Node2D.

All three transform getters and setters are now available at the CanvasItem level, however the set_* functions are not registered at this level. This is because Control, (which extends CanvasItem), does not support setting a transform using these APIS. Control can not support this due to not storing an actual transform matrix, (include pivot as well as rts), so there is inrormation loss on a transform setter. Therefore these functions are only registered in Node2D.

This correctly sets up CanvasItem to correctly be inherited by new classes (other modules may have a class that extends CanvasItem).

Note** this is an API breaking change, as 'get_relative_transform_to_parent' has been renamed to 'get_relative_transform_to_ancestor'. The function however is still available in Node2D. Should be tracked in #16863

[jordo]

Sorry @reduz but I need a clarification something in this PR as well, and with your response I may need to fix a few more things.

Please can you clarify the intention of below in canvas_item.cpp:

Transform2D CanvasItem::get_global_transform() const {

This function traverses up the scene tree to find the global transform of a node. However, if it finds 'non' CanvasItem node it stops. Why? Why not continue on with the grandparent? That way sticking a generic 'Node' (or any other node that doesn't inherit from CanvasItem) in between CanvasItems nodes in the scene tree doesn't break the local to global transform and vice versa?

Or, if the intention is to stop at the traversal at a Viewport, why not explicity just check for that condition as well?

[jordo]

Please don't merge until after discussion with reduz. If this is not intentional design, I will amend this PR to fix these traversals.

[aaronfranke]

It doesn't make sense for this method to exist if it will just always fail.

As for whether we can implement this... yeah, it can be done. But is that desired? Control nodes usually are not meant to be used this way. If you want full control over a canvas node's transform, Node2D would make more sense.

[jordo]

It has to exist or the engine will not compile, as set_transform is now pure virtual in the base class. It's not exposed anywhere, and can hopefully be used if Control is re-written anytime to use a transform instead of 3 other positional properties (outside the scope of this PR).

The alternative is to make set_transform be non pure-virtual and implement a noop in CanvasItem, which effectively does the same thing, except if you ever use set_transform on a Control you will be un-aware of the issue, which I why i implemented it this way with a warning.

[aaronfranke]

The alternative would be to define set_transform on Node2D and not on CanvasItem :) (like is currently the case in the current master and in all previous Godot versions to my knowledge).

[jordo]

It's been like that for 7 years, since the first commit... That's why the suggestion to correct. There is no good reason why these aren't in CanvasItem, and if they are in the base class anyone can use recursion to traverse up the tree, etc. If my class inherits from CanvasItem, having these in Node2D don't help much do they

[jordo]

Currently the only two classes in-engine classes that inherit from CanvasItem are Control and Node2D, surely we can fix some of the quirks here.

[reduz]

I honestly dont even understand why this function exists, just request both global transforms and invert the ancestor:

ancestor.global_transform.affine_inverse() * node.global_transform

It will be much faster than any of this. Its probably added for ease of use, but it does not even warrant doing all the changes proposed in this PR.

[jordo]

@reduz can you comment on my second comment? Global transforms break whenever a non canvasitem node is in the traversal path.

Regarding this PR, this is about refactoring CanvasItem to be more entendable, not just that particular function, which I renamed and included because it was wrong. This is a based on a use-case I have right now.

If i can't get a refactor and clean up merged upstream, I'm forced to put the EXACT same code that is in Node2D in my class in my module that extends CanvasItem, that is going to suck. All a CanvasItem should know how to do, is get and set their transforms. If they implement that (pure-virtual), then the global_* recursive funcs that use that implementation for free, OOP for the win right?

If i can't move the setX to CanvasItem, I'm literally going to have just copy and paste the 'set_global_transform()' function from Node2D to my new node? The only thing i should have to implement is the set_transform(), I shouldn't have to re-implement set_global_transform with the exact same code copied from Node2D, for no other reason that we can't do a refactor properly. The PR really isn't doing much TBH

[akien-mga]

@jordo To clarify your use case: what is there in Node2D that you don't need in your own class? I.e. why do you want to extend CanvasItem instead of Node2D, if you want your node to behave like a Node2D?

[jordo]

@akien-mga @reduz My use case is I'm extending CanvasItem (My new class in module). I'm not extending Node2D because to be frank, it's confusing for users to access properties that I don't want them accessing because it breaks things in un-intuitive ways. (Scale in in-tree nodes, when related to syncing tree nodes with a physics engine). I don't want all the stuff that Node2D exposes (set_scale/get_scale). IF I do that, the first questions I will get about my module from users will be why things don't work when they zoom in and out of a physics node, or in a container node that holds physics nodes. I want to explicitly disallow users from shooting themselves in the foot. I don't want to have to rely on documentation to tell users don't do this, I just don't want to allow them to do things wrong in the first place.

Likewise the second question in this PR is about where actually is the global transform? The global transform stops at the first non CanvasItem node? Why? My current two modules I'm working on (Network module and Physics module) combined together need global transforms to work properly when combined together. Why do transforms (a visual rendering thing) break when encountering a non CanvasItem node in the traversal path? ( I can't put my controller or game logic nodes in these paths, and it's having ramifications for how I'm building my other module ). Lots of general purpose Nodes have to extend Node because we want them to work in 3D scenes and 2D scenes. But the second any of these Nodes are in a transform traversal path (2D or even 3D), it breaks the global transform, forcing users to build their scene hierarchy in annoying ways right now.

Can I provide a simple video if that would help to explain this? It would honestly be easier than here perhaps?

Bur here's something to consider, and I'm just pointing out that obviously at some point someone added 'get_relative_transform_to_parent()' because it's in the codebase, and it's used. Look at the cast in the traversal in the implementation of that function (it's a cast to Node2D NOT CanvasItem). That means this function works for Node2D traversals, but NOT canvasitem traversals (Controls). But the fact that I just caught that now, shows how awkward these traversals are already. If you remove get_relative_transform_to_parent() you probably are going to end up breaking something else in the engine right now that is relying on that functionality.

[jordo]

Look, there should be a base class in 2D that is consistent for doing in-tree global transformations recursively. (Clearly it's canvas item. It holds the get_transform() function that is used to implement the recursive global xform function). And canvas item is whats casted to when asking a node for their global 2d transform. Except, as I just noticed in 'get_relative_transform_to_parent()' it only works on a Node2D cast (which is most likely broken in subtle ways for users right now).

Look at the 3D path for this. It's clean. Everything is is Node3D. get_xform, set_xform, get_global, set_global. All clean and nicely accessible from the base class.

[jordo]

I will also add a change to get_global_transform()/set_global_transform() change in another PR, and correct the 3D path on that one as well since I think my first question in this PR is somewhat unrelated to the CanvasItem refactor.

[reduz]

Global transforms break whenever a non canvasitem node is in the traversal path.

@jordo that is completely intended, just dont do it

[jordo]

@reduz So just to confirm, it is then BY DESIGN that godot does this:
https://youtu.be/WwOaTPljKzw

over this:
https://youtu.be/ala39TYA3h0

And that is not open to change?

[reduz]

Maybe this is a documentation problem? Traversal paths are only for contiguous parent-children relationships. If something unrelated is in the middle, they break and start over.

[jordo]

Why though? I know that's the way it currently is, I'm struggling to figure out why. And also why the resistance to change when I believe it's clearly a better experience and more flexible.

[reduz]

@jordo Yes, this is by design and its not open to change. The reason is that there are many types of nodes where this relationship cant be continued anyway, like viewport, canvas, canvaslayer, window, or even controls with a toplevel flag, etc. Having to start adding exceptions on what can be continued and what cant is ugly, so as a rule you can only inherit transform on parent-child relationships.

[jordo]

@reduz all that I changed is a get_parent() canvas item with a get_ancestor() canvas item. Its halts at all the same places wrt canvas, viewports, and everything else. it 'just works'. But OK, if this is a design decision, but it's quite limiting for my use case and I believe confusing to new users.

[reduz]

In the event where you really want something like this, you can use the RemoteTransform 2D/3D nodes.

[jordo]

Unfortunately I can't :(

[reduz]

Sorry, this is not going to change. As I mentioned, current rule requires no exceptions so its preferred over the proposed one.
Your use case seems quite rare, so doing a workaround is not a problem. If there is some extension or feature that will make it easier thats another issue.

[jordo]

huh, OK. Well after spending a day in cavas_item it's really not an exception. The class already has a bunch of edge cases whether things are drawn in cavas_layer, viewport, or direct parent. Its not complicated to traverse for an ancestor canvasitem.

I think the more intuitive the workflow in the engine the better.

[reduz]

Keep in mind that, for users, keeping nodes of the same type chained up has never really been a problem and this rule is generally well understood, so finding that there are use cases where this breaks is rare.

[jordo]

I definitely think people who have been in the engine for many years just give this no thought because 'thats the way it works' and what they're used to.

I'm relative new user (3 months), and experienced game dev. I haven't encountered any other engine who's scene tree works the way godot's does in this regard.

[jordo]

I suppose you can close this PR, I'll probably maintain a separate branch with this functionality as I mentioned it's a blocker with how I'm building another module. And in general a little wonky to me. all good

[aaronfranke]

Don't close this just yet, get_relative_transform_to_parent is still badly named at the very least, could be moved to CanvasItem, and/or could be removed as @reduz suggested. We can update the PR so that it only changes get_relative_transform_to_parent.

[jordo]

If there is anymore interest in the specific use case for fixing this as well, some discussion here: briansemrau/godot_box2d#22

But for now I'll treat this as unfixable upstream and try and workaround or maintain a soft fork.

[briansemrau]

I gotta say, the more I think about this the more I'm not sure which side of the fence I'm on about whether Node (or other non-Node2D nodes) should break the transform chain... I was originally of the opinion that the current design in Godot is good, and that adding a Node to intentionally "reset" the global transform mid-tree was a good feature. However, there is a function that allows you to set a Node2D to not inherit the parent transform. Ignoring the (strong) argument about this breaking compat and changing a well known Godot behavior, shouldn't that setting be the intended way to break parent transform relationships, rather than adding a superfluous Node in the hierarchy?

I'm not firmly trying to argue that this should change, I just thought I'd share my opinion.

[jordo]

I mean. If you spend a few hours in canvas_item.cpp and really understand how the class works, you'll realize that to implement this functionality you really only have to change CanvasItem::get_parent_item() to something like CanvasItem::get_ancestor_item(). All the same checks for adding canvas_items to the internally maintained children_items list still work, and all the checks for whether the item is registered in the RenderingServer properly all still workout (canvas_layer, viewports, or canvasitem parent (or proposed ancestor) node).

It's also evident the class could use a refactor and little cleanup. Lots of places using get_parent_item() correctly, other places just get_parent() and casting.

But maybe some consideration to the front facing api would be help 'global_transforms' are definitely not the 'global' transform. In my example in the video, the Sprite2D under the Controller Node in the scene tree's 'global' transform is actually it's local one... Quite confusing.

At any rate, the decision is just stay with the status quo, which is fine. 👍

[aaronfranke]

@briansemrau I like the example of Viewport that reduz gave, so I'll elaborate of why Viewport must necessarily do this:

Viewport has to stop the transform chain otherwise things break. Here is a scene where each of the sprites has a position of (32, 32), the child of Viewport has icon.png, and the other has a ViewportTexture:

If we set the position of the root Node2D to (32, 0), this happens:

If Node2D's transform applied to the child of the Viewport, this would happen:

The logic is: If not all Nodes can keep the transform chain, then Node can't do it.

[jordo]

@aaronfranke viewports still stop the chain as before.

[jordo]

My second comment in this thread specifically asks that, because I think that is what should happen. And is still what happens in my proposed solution.
Or, if the intention is to stop at the traversal at a Viewport

Viewports are special for a whole bunch of reasons, and they are already treated as special cases in cavasitem itself. Your screenshots with viewport work correctly in my proposal.

The 'global_transform' is really the transformation to the nearest viewport. Which seems correct.

[jordo]

Anyways, I think we can leave it at that no? I've outlined my use-case, and the expected functionality in the PR.

If this won't get upstreamed because A), the functionality is not desired and B) the implementation would add significant complexity, then that's fine. I just don't happen to agree. With either. Life moves on :)