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

Add a warning for when the scene root node is transformed #81892

Merged

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Sep 19, 2023

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.

Screenshot 2023-09-18 at 5 31 02 PM

@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from a9dd0ac to 532fa0a Compare September 20, 2023 02:19
@Calinou Calinou added this to the 4.x milestone Sep 20, 2023
Copy link

@Scoppio Scoppio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, running fine.

I always miss the presence of GUID or some other unique identification on which is the object that generated the warning, but in this case it seens to be pretty much on topic here.

@Calinou
Copy link
Member

Calinou commented Sep 25, 2023

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 godotengine/godot-proposals#3004.

@ryevdokimov
Copy link
Contributor

ryevdokimov commented Jan 23, 2024

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.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 2, 2024

The warning isn't entirely correct. Most often only position is overriden, e.g. when dropping the scene into viewport.

@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from 13533ad to 12bdf93 Compare March 27, 2024 01:53
@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from 12bdf93 to 9a123b0 Compare May 24, 2024 10:13
@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from 9a123b0 to 4c4c08c Compare June 26, 2024 02:10
@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from 4c4c08c to da2ba4f Compare August 4, 2024 05:00
@aaronfranke
Copy link
Member Author

aaronfranke commented Aug 4, 2024

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.

@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from da2ba4f to d7fe2cf Compare August 30, 2024 10:29
@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from d7fe2cf to 38b4f82 Compare October 2, 2024 23:51
@KoBeWi
Copy link
Member

KoBeWi commented Oct 3, 2024

Isn't this (at least partially) superseded by #80561?

@aaronfranke
Copy link
Member Author

@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.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 19, 2024

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?

@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from 38b4f82 to 4a4d2ff Compare October 30, 2024 07:17
@aaronfranke
Copy link
Member Author

@KoBeWi Yes, I noticed that too. It's a minor problem, I think it's fine, unless someone knows how to fix it.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 30, 2024

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 aaronfranke force-pushed the scene-editor-warn-root-transform branch from 4a4d2ff to dacadcf Compare October 30, 2024 11:17
@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from dacadcf to cf2fb47 Compare December 15, 2024 07:48
@aaronfranke aaronfranke force-pushed the scene-editor-warn-root-transform branch from cf2fb47 to e1429cd Compare December 18, 2024 07:49
@Repiteo Repiteo modified the milestones: 4.x, 4.4 Dec 20, 2024
@Repiteo Repiteo merged commit d48ada1 into godotengine:master Dec 20, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 20, 2024

Thanks!

@aaronfranke aaronfranke deleted the scene-editor-warn-root-transform branch December 20, 2024 08:32
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.

7 participants