Improved synchronization of Transforms during live debug sessions

[MajorMcDoom]

What problems was this meant to address?

• Some editor transform operations (such as dragging 3D nodes) are committed to the undo/redo system with their global transforms, which are in turn synced to the running game. This means that all the corresponding child nodes in all instances of the scene in the running game are moved to the same location in world space, which is not intuitive or useful.
• Furthermore, when you move the root node of a scene (by any means), this operation is synced to the running game. This means that all instances of the edited scene in the running game are moved to the same location, which is also not intuitive or useful.

In the GIF below, you can see the current, undesirable outcome:

What was changed?

• Transform operations are now committed to undo/redo in local instead of global space.
• Transform edits to the root node of the current edited scene are no longer synced to the running game.

In the GIF below, you can see the new, expected behaviour:

Are there any caveats or tradeoffs?

• Committing operations in local space will not introduce any new issues, since the undo/redo chronology is maintained, so there is parity with the old behaviour when strictly considering the in-editor workflow. In the niche case of parents being moved around by tool scripts, there will not be parity with the old behaviour. However, it is more intuitive, consistent, and predictable for the operations to be undone/redone in local space in this use case anyway, for the same reasons as in the running game.
• You will no longer be able to move a scene in the running game by moving its scene root in the editor. However, this is arguably something that should never have been possible, as this should be done instead by editing the scene which contains the scene you want to move. This is more consistent with the usual editor workflow for nested scenes anyway, where editing the root transform of a scene does not affect instances of that scene in other scenes.
• Root scale is no longer propagated to the running game. Unlike position and rotation, it is a possibility that the user would expect the root scale to be consistent and propagated across all instances of the scene. However, there is also a possibility that all instances already had individual scales applied (e.g., lots of trees with slight variations in scale). This is a tradeoff, and I think being unable to preview a scale change is less destructive than wiping out existing scales on all instances. Another reason is that many of the synchronized operations are all-inclusive, and teasing out the scale is adding more complexity and overhead.

[AThousandShips]

Unsure if this breaks compatibility or not, as some possibly relevant behaviour changes, needs to be discussed

[MajorMcDoom]

Might be worth mentioning some context here:

As a new Godot user a few months ago, I saw the strange behaviour of live debug transforms and assumed it was a network bug. It had me feeling very lost as to what it was supposed to achieve, but it definitely didn't help me in any way.

With the new local-to-scene synchronizations, the full workflow can now be described as de facto realtime prefab editing, which not only covers a significant portion of the realtime editing use cases raised by many Unity migrants, but actually goes far beyond, since changes affect all instances, plus you don't have to remember and reapply the runtime changes after you stop debugging.

Editing all other node/script properties and resources in a scene at runtime has been seamless, and I think correctly syncing child transforms is the missing piece to that workflow.

[BastiaanOlij]

I agree with @AThousandShips that this needs to be discussed with a few developers that are better informed about how this was originally designed to work.

That said, I just reproduced the issue, it's not clearly enough described by @MajorMcDoom, but this is a bug and his fix may be the right fix.

From what I understand of the inner workings of the remote debug feature, as scenes are instantiated and added to the scene tree within the game this creates one large scene tree. It is this scene tree that you see in the remote debug tab and that you manipulate. As we're working with one big tree, where we are manipulating a single node in the tree, updating the global or local transform should have the same result.

But from the description of @MajorMcDoom we seem to have added the ability to open the subscenes and edit them and sync them. As mentioned above I just tried this, I wasn't aware we had this feature, it is SUPER COOL, but it's also SUPER broken :)

When editing a subscene you can't manipulate the global transform, yes we've shot ourselves in the foot that when you right click on a node and save the node as a subscene, that we save that scene with it's transforms intact which can give the illusion things are correct. But anything more complex, say having a subscene in a subscene, or having multiple instances of a subscene, and you immediately see the problem.

I just did a very simple experiment where I created a subscene with some meshes, instanced it multiple times in a parent scene and placed them around the scene, and then tried to edit the subscene to see what happened. Any change to the subscene ruined the running scene by placing all instances in the same spot. Not what I would call expected behaviour.

This also means that the 2nd and 3rd caveats are basically non-issues. When we're dealing with a subscene the root transform isn't change-able in a meaningful way, especially if the scene is instanced multiple times, as the real transform applied is in the parent scene. While if you've just saved a branch as a subscene, you can manipulate the root transform of the subscene and it will be "applied" to the parent scene, this is the only exception to the rule (I've often believed Godot shouldn't do this, and reset the transform in the subscene).

I need to test if the fix works as advertised, but based on the testing I just did my gut feeling says this is indeed a bug and this fix should fix it.

[BastiaanOlij]

Tested this fix using my MRP from #86752

Works like a charm:
https://youtu.be/MPnB2HELDWQ

[MajorMcDoom]

Tested this fix using my MRP from #86752

Works like a charm: https://youtu.be/MPnB2HELDWQ

That's good to hear! Just to be clear, there are multiple ways to affect changes to the transforms, like Align Rotation with Camera, and Snap to Floor, sliding the actual spinners in the inspector, dragging gizmos, etc. These all go through different properties and methods, and so my fix had to account for them all, to make sure they all sync correctly, but also maintain parity with old behaviour.

I have done that testing myself, but just noting that for any future testing that might be needed.

Another thing I want to mention is that my solution, for the sake of simplicity, simply filters out certain methods and properties under certain conditions from being sent over to the running game. It does mean that if we ever change/add features that modify transforms through other properties/methods, then those will have to be accounted for as well. I considered making a catch-all solution for transforms (either on the sending side or the receiving side), but the current way the feature is implemented made it difficult, so I opted for the current solution. If anything does come up in the future, they will be tiny edge cases that can be easily addressed.

Also, @BastiaanOlij @AThousandShips considering this is now considered a bug fix, is there any reason it can't be moved to 4.3 milestone?

[BastiaanOlij]

Also, @BastiaanOlij @AThousandShips considering this is now considered a bug fix, is there any reason it can't be moved to 4.3 milestone?

I think some others who are a little more versed in the editor should verify my findings, but if the group concurs with me that this is a bug, and if no further requirements are identified, I think this should indeed be a milestone for 4.3 with a possible cherry-pick for 4.2

[AThousandShips]

Needs to be verified as:

• A proper fix
• not undesired side effects

Even if we change it to 4.3 now that's no guarantee it won't be delayed due to confirmation needed, nor is it being on 4.x going to make it go slower 🙂

[KoBeWi]

Something is broken with undo:

godot.windows.editor.dev.x86_64_iI9ir0xqiR.mp4

I have yet to look at the code, but functionally it looks fine. The change to local transforms seems to have already existed in 2D, so this makes 2D and 3D consistent in the more useful way.
Not synchronizing root transforms also makes sense; the only time you could possibly want otherwise is when you have a single instance of the scene, or when the instance is the scene you are running, but not sure if it can be reasonably determined.
I'm not sure about the scale thing, but the reasoning seems fine.

I think if users complain about any of these changes, we could make the behavior customizable. But for now it's not necessary.

[MajorMcDoom]

@KoBeWi

Something is broken with undo:

Can you clarify how to repro the undo issue? You can see from the GIFs in my original post that undo does work, so you may have found an edge case that I need to address.

The change to local transforms seems to have already existed in 2D

This isn't quite true. All the operations involving transforms in 2D (as with 3D) use different properties and methods to achieve, and some already worked in local space, but not all. I don't think there was ever an intentional "change" over to local space - it just depended on what properties were used. I did have to change a couple things in 2D as well to address global space and root syncs.

[MajorMcDoom]

@KoBeWi I have found a repro for the undo bug you encountered. Fixing now.

[MajorMcDoom]

@KoBeWi Undo bug has been fixed.

[MajorMcDoom]

UPDATE:
I've changed the way the root transform sync exception works by implementing it on the receiving end, instead of the sending end. The advantages:

• There no longer has to be any special detection of "transform-affecting functions and properties" on the sending end, it's just a catch-all solution, which is much easier to maintain.
• On the receiving end it also now detects if the attempted edit is to the root of the current running game. If so, it does allow the root transform change, since the edit and the debug instance are running in the same context, and there can be no other interpretation of user intent.

[KoBeWi]

Functionally looks fine.

I'm wondering about the debugger code though. The PR adds a dependency on Node3D and CanvasItem. It can be avoided by using is_class() and call() instead (which I think might be preferred, given the dynamic nature of the debugger).

Another thing is that the transform is preserved by remembering old transform and reverting it. I think the previous property-based approach could've been better.

But that's more for the debugger team to asses.

[MajorMcDoom]

@KoBeWi

As per your suggestion, I've updated it to work with variants, is_class, and call instead, and removed the two #includes.

As for the record-restore approach, I think it makes more sense as a catch-all. If we use the property/method check approach, if anyone in the future changes how "Align Rotation with Camera" works, or introduces a new feature that moves a 2D sprite in the editor, it could break this, and someone would have to go in and reconcile the new property/function list.

Plus it is not something that executes frequently, so performance isn't an issue.

That said, I agree, debugger team feedback on this would be great.

[BastiaanOlij]

Not synchronizing root transforms also makes sense; the only time you could possibly want otherwise is when you have a single instance of the scene, or when the instance is the scene you are running, but not sure if it can be reasonably determined. I'm not sure about the scale thing, but the reasoning seems fine.

I think if users complain about any of these changes, we could make the behaviour customizable. But for now it's not necessary.

@KoBeWi I think we need to be really careful with this because it could lead to the user making and saving changes, that once re-running are lost because the parent scene overrides the transform. The expected behavior here would only work if the user has just saved a branch as scene. Any changes the user does from here, even if the subscene is used only once, runs the risk of the parent transform becoming useless.

So my gut feeling says we should exclude this at all times, and preferably even disable the transform field in debug mode (if we can). Then have a good explanation in a comment as to why we've made this limitation.

Other than that, this is looking really good @MajorMcDoom :)

[MajorMcDoom]

Not synchronizing root transforms also makes sense; the only time you could possibly want otherwise is when you have a single instance of the scene, or when the instance is the scene you are running, but not sure if it can be reasonably determined. I'm not sure about the scale thing, but the reasoning seems fine.
I think if users complain about any of these changes, we could make the behaviour customizable. But for now it's not necessary.

@KoBeWi I think we need to be really careful with this because it could lead to the user making and saving changes, that once re-running are lost because the parent scene overrides the transform. The expected behavior here would only work if the user has just saved a branch as scene. Any changes the user does from here, even if the subscene is used only once, runs the risk of the parent transform becoming useless.

So my gut feeling says we should exclude this at all times, and preferably even disable the transform field in debug mode (if we can). Then have a good explanation in a comment as to why we've made this limitation.

Other than that, this is looking really good @MajorMcDoom :)

Thank you!

As for your responses to @KoBeWi concerning the exceptional cases to allow, I think we should distinguish between different use cases mentioned, because they do deserve different treatment:

• Editing the root of a scene that is also the scene being run. i.e. there is no parent that can move. This case I have already made an exception for in my latest changes, and I think should be allowed. The scene would, structurally and visually, form the entire scope of operation for the user in both windows. It's the most logical interpretation of the user's intent that if they move the whole scene in one, it affects the other.
• Editing the root of a scene that is a single-instance child in the running game. In this case, the side effects are bad, as you said. Any change you make to the root in the editor would immediately mess up the runtime position state of the child in the running game.

I'm not sure about your idea of disabling the root Transform in the editor though. First, it's not really practical because it's not just a field - it's any operation that can affect the transform that is being blocked. Second, we don't really want to prevent edits to the project. We simply don't want them to propagate them to the running game in a way that doesn't make sense.

[BastiaanOlij]

@MajorMcDoom sorry I wasn't clear, I was purely talking about that second use case (e.g. Editing the root of a scene that is a single-instance child in the running game.), the first situation it makes sense it is editable.

Anyway I think the way you made it work is fine, disabling the transform field would be a nice to have and indeed as you say, it would likely remain incomplete.

As long as we clearly document the behaviour and why we did what we did, it's a great fix.

[BastiaanOlij]

ping @akien-mga

Imho this is an important bug fix that should not fall through the cracks. Right now this is marked as needing consensus with no further action seeming to happen nor clarity on who could provide that consensus. This sadly is an area of the editor that doesn't see heavy enough use within core contributors to gain their feedback. Users who do make use of this, won't test this until it's merged, possibly not even until it's in stable.
Waiting on @reduz for this doesn't seem productive, he is far too busy to look into something trivial like this.
So who else could weigh in on this to push it over the line?

@Faless, it's a little hidden in the "4.3 merge queue" thingy but you're tagged there to weigh in on the debugger changes here. If you have a moment that would be cool!

[akien-mga]

Couldn't get a final review, so let's get this merged. Sorry for the delay :)

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[MajorMcDoom]

Thanks! And congrats for your first merged Godot contribution 🎉

Awesome. Thanks y'all! :)