godotengine
Add a warning for when the scene root node is transformed
This is a low priority PR, but I think it's a good idea.
This PR adds a configuration warning for when the root node has a transform. This is usually a mistake when saving a branch as a scene, or having the wrong node selected when doing transform operations. Instances of the scene will set their own transform, so it's best practice to not have the root be transformed.
The first commit in this PR is just code organization. It does not make much sense for the configuration warning string formatting to be in Node, since this is only needed in the editor.
Note that this may change if we do decide to print node configuration warnings in a running project (when debugging is enabled). This is something I've considered at some point, as it can help with difficult-to-diagnose runtime-only issues without requiring code duplication. See https://github.com/godotengine/godot-proposals/issues/3004.
I'm personally not a fan of this. I believe warnings should be reserved for when the configuration of a node results in behavior that in all circumstances is not correct (like a collision without a node that derives from it). I don't see anything objectively wrong with changing the transforms of a root node, as long as instantiations of it are handled correctly such as in the above-mentioned PR.
The warning isn't entirely correct. Most often only position is overriden, e.g. when dropping the scene into viewport.
The warning isn't entirely correct. Most often only position is overriden, e.g. when dropping the scene into viewport.
I would argue that if users want a scene to have inherent scale or rotation, they should instead scale or rotate the child nodes. This way this inherent scale or rotation will not be discarded if the user sets node.transform on the scene root.
If a user sets a scene's scale to be (2, 2, 2), it's expected that the scene should get bigger by a factor of 2. If the original scene had a root node scale of (10, 10, 10), this would result in the scene getting smaller by a factor of 5.
@KoBeWi They are separate things, but you are correct that there is a partial overlap. However, this PR covers the case of a user accidentally transforming their scene's root node.
So I'm not sure about the warning itself, but the implementation is fine. Except the warning does not update when you transform the node; you have to save the scene or update warnings in other way. Is that expected behavior?
@KoBeWi Yes, I noticed that too. It's a minor problem, I think it's fine, unless someone knows how to fix it.
It's confusing when you reset transform, but the warning is still there. At least mention in the warning that clearing it requires scene reload.
@aaronfranke As part of my configuration info refactor #90049 I need to somehow integrate this into the new system.
Was there any particular reason the checks and warnings were added to SceneTreeEditor, instead of adding them as normal configuration warnings on the Node2D and Node3D classes?
I assume it was because get_configuration_warnings is available outside of editor? If so, the new system is guarded by TOOLS_ENABLED so it would only run in editor. Which means I could move your code into both node classes.
@RedMser This was done because the warning is only valid for the scene root node, not just any Node2D/Node3D.
Also, in general, I would be interested in having less editor-specific stuff inside of non-editor classes, even if guarded by TOOLS_ENABLED, not more.
I can see where you're coming from, but I really don't like the thought of having the list of configuration info populated from multiple different sources. It's already tedious to follow the inheritance chain for config warnings, but having to now also check a separate function as an additional source of warnings doesn't seem great to me.
Sadly I don't have a proposal for a solution that makes both of us happy. Especially with how Resource would now also get config info, which means more editor-specifics in these classes...
tbh you can check whether the node is scene root from within the node, by using get_tree()->get_edited_scene_root().