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

Fix transition expression base node with autoloads #66573

Conversation

GuilhermeGSousa
Copy link
Contributor

Fix for #62530 when using autoloaded nodes.

My fix isn't exactly the most elegant one here, I couldn't find a better of doing it. The bug comes from the fact that advance_expression_base_node_path is relative to the scene's root node. The best way I found of retrieving that node at runtime was to get the last child of get_tree()->get_root() (the game Window) since the first ones are the autoloaded ones.

@GuilhermeGSousa GuilhermeGSousa requested a review from a team as a code owner September 28, 2022 22:03
@kleonc
Copy link
Member

kleonc commented Sep 28, 2022

The bug comes from the fact that advance_expression_base_node_path is relative to the scene's root node. The best way I found of retrieving that node at runtime was to get the last child of get_tree()->get_root()

Is it relative to the root node of the scene the tree_base is in? Or to the so called "current scene"? What you're doing is for the latter case, for the former case you'd need to somehow use tree_base.owner instead.

You can test out which is the actual case by instancing the scene containing the AnimationTree as a subscene within another scene with some nested subnodes and run such scene. If it's relative to the root node of the scene the tree_base is in then your current solution from this PR would fail.

@GuilhermeGSousa
Copy link
Contributor Author

GuilhermeGSousa commented Sep 29, 2022

You're right @kleonc its relative to the root node of the scene tree_base is in. I think the correct way of fixing this would be to have the AnimationNodeStateMachineTransition's advance_expression_base_node_path follow the same behaviour as the one of the AnimationTree: have the path be relative to the tree_base itself. In a setup like this:

image

if the path is set to "Main Menu" on the transition it should be something like ".." instead of "." as it currently is.

I believe that behaviour is different between the two since one is a Resource and the other one a Node.

@GuilhermeGSousa
Copy link
Contributor Author

GuilhermeGSousa commented Sep 29, 2022

I think this bug come from a more generic problem that affects all Resources, exported NodePaths in resources are always relative to the root of whatever scene is currently being edited, and therefore hardly useable at runtime.

The way I see it advance_expression_base_node_path should maybe be removed from transitions altogether until there is a way of referencing Nodes in a scene from a Resource.

Another solution would be to have AnimationNodeStateMachineTransition hold a reference to its AnimationTree, much like classes that inherit AnimationNodes do (which despite its name is a Resource).

@Chaosus Chaosus added this to the 4.0 milestone Oct 1, 2022
@GuilhermeGSousa
Copy link
Contributor Author

If agreed I'd close this PR to open a new one to remove advance_expression_base_node_path from transitions

@GuilhermeGSousa
Copy link
Contributor Author

Closing this PR and opening a new one to remove base expression paths from transitions

@GuilhermeGSousa GuilhermeGSousa deleted the fix-transition-expressions-with-autoload branch October 7, 2022 07:10
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