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

Set position to zero when saving a positioned branch as scene #80561

Merged

Conversation

HolonProduction
Copy link
Member

@HolonProduction HolonProduction commented Aug 12, 2023

Fixes #80560

The usability problem results from Godot keeping the state of an instanced scene in sync. This is an important feature and can't be fixed. However by changing the position in the new scene to zero the situation where it becomes a problem for the user is avoided.

@HolonProduction HolonProduction requested a review from a team as a code owner August 12, 2023 20:18
@KoBeWi KoBeWi added this to the 4.x milestone Aug 12, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Aug 12, 2023

Looks alright on the technical side, but you need to change your code. Assignments inside expressions are generally not allowed and here you even made 2 at once.

Also I wonder if this needs a proper proposal. While the new behavior is more useful, some users might rely on the origin not being reset, because it's useful for their workflow for whatever reason.
And what about other transform properties, i.e. rotation and scale? Should they get reset too?

@HolonProduction
Copy link
Member Author

Assignments inside expressions are generally not allowed and here you even made 2 at once.

I wasn't sure about that and had a look at the C++ usage guidelines. If that is not allowed here it should be listed there IMHO. But sure I can rewrite it.

Also I wonder if this needs a proper proposal. While the new behavior is more useful, some users might rely on the origin not being reset, because it's useful for their workflow for whatever reason.

I try to avoid proposals whenever possible, sorry for that, but it just feels as if they never get anywhere. But if you think it might help, sure.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 12, 2023

If that is not allowed here it should be listed there IMHO.

Right, it's probably an oversight. The topic was discussed a few times and there was a decision against this coding style, but was never formalized.

I try to avoid proposals whenever possible, sorry for that, but it just feels as if they never get anywhere. But if you think it might help, sure.

Proposals have a bit more visibility than a random PR closing a random issue that was opened very recently. I think this change is fine, but it's better to get some broader feedback.

EDIT:
Your proposal was closed as a duplicate. The fact that there is an independent, year-old proposal, justifies this change IMO.

@Bitlytic
Copy link
Contributor

I noticed this was an annoying issue for new people getting into Godot so I was looking at making my own PR/Proposal for it and stumbled upon this. Was this abandoned/denied or was there a reason this is set to draft mode?

@KoBeWi
Copy link
Member

KoBeWi commented Nov 16, 2023

Looks abandoned.

@HolonProduction
Copy link
Member Author

HolonProduction commented Nov 16, 2023

I made it a draft when I opened a proposal for it and forgot to mark it for ready when the proposal was closed. But I think I haven't fixed up KoBeWi's notion about assignments inside expressions. (Didn't wanted to work on it while the proposal was active)

@HolonProduction HolonProduction marked this pull request as ready for review November 16, 2023 20:32
@HolonProduction
Copy link
Member Author

I added a question to the older proposal which didn't receive any feedback so here it is again: do we need a checkbox for it? Because if so it needs to be reworked. So yeah kinda waiting for feedback rn.

@Bitlytic
Copy link
Contributor

Ah I see, I'll put my personal opinion over on the proposal thread based on things that I've seen from other people using the engine

@KoBeWi
Copy link
Member

KoBeWi commented Nov 17, 2023

Based on #39754, I think non-centered scenes are a use-case that still should be supported. So indeed some checkbox/option (enabled by default) should be added.

@Bitlytic
Copy link
Contributor

@HolonProduction Do you want to continue the work you did on this? I'm not currently working on it because you were before but if you give me the go ahead I'll immediately hop on it as I really care about this feature being completed

@HolonProduction
Copy link
Member Author

@Bitlytic go ahead! I'm currently more focused on auto completion and don't have much time anyway.

@Bitlytic
Copy link
Contributor

Based on #39754, I think non-centered scenes are a use-case that still should be supported. So indeed some checkbox/option (enabled by default) should be added.

I'm getting around to this now, you mentioned that it should be enabled by default. I just wanna make sure, you want the new version that saves the branch at (0, 0, 0) to be the version used by default or what's live in Godot 4 to be the default?

@KoBeWi
Copy link
Member

KoBeWi commented Nov 25, 2023

The new version should be default.

@Mickeon
Copy link
Contributor

Mickeon commented Sep 2, 2024

This is a particularly desired even a year later. @HolonProduction could you rebase, so that someone could hopefully take a look at it for be merged soon?

@HolonProduction
Copy link
Member Author

Thing is, that apparently we want a checkbox to control this behavior and I never implemented that. So if this is still a requirement, this will need a bit more than a rebase I guess.

@HolonProduction HolonProduction force-pushed the i-have-made-my-position-clear branch 2 times, most recently from c7b80bb to 2892156 Compare September 6, 2024 09:53
@HolonProduction
Copy link
Member Author

So I added check boxes to the file dialog:

Screenshot from 2024-09-06 11-42-54

It might make sense to alter the way the options are displayed, to use a HFlowContainer because it really does not look great.

Also there are no API's to hide the options, which means they would also be visible when saving a node that is neither Node2D nor Node3D

@tcoxon
Copy link
Contributor

tcoxon commented Sep 6, 2024

This is a feature my team has wanted for a long time now, so I'll offer my opinion here.

It seems rare that anyone (in a 3D project, at least?) would want to reset only the position, and not also the rotation and scale. I see that the option defaults (true, false, false) are hard-coded. Should these be made into editor settings like docks/scene_tree/center_node_on_reparent so users only need to set them once?

@Mickeon
Copy link
Contributor

Mickeon commented Sep 6, 2024

Also there are no API's to hide the options, which means they would also be visible when saving a node that is neither Node2D nor Node3D

I would have them disabled instead of hidden. But I assume you mean there's no way to infer what type is being saved, which is a pity.

Should these be made into editor settings like docks/scene_tree/center_node_on_reparent so users only need to set them once?

These kinds of settings are usually stored per-project separately. Although, I'm not sure how at the moment.

@HolonProduction
Copy link
Member Author

I would have them disabled instead of hidden. But I assume you mean there's no way to infer what type is being saved, which is a pity.

No, inferring that before the dialog is shown would be possible through the current selection (that's how it is done after a file path was chosen anyway). But EditorFileDialog has no methods to disable, hide or remove an option after it was added. We could probably have two separate file picker dialogues, but that feels a bit heavy (also still no way to have them disabled).

I see that the option defaults (true, false, false) are hard-coded. Should these be made into editor settings like docks/scene_tree/center_node_on_reparent so users only need to set them once?

That's an interesting idea, for now I'd rather keep this PR at its current state and leave this open for a possible follow up. I don't know yet how this would be done, and I hope we can get this in for 4.4, so I don't want to delay it further.

editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@HolonProduction HolonProduction force-pushed the i-have-made-my-position-clear branch from 2892156 to 03c3c5f Compare September 30, 2024 07:43
@Repiteo Repiteo modified the milestones: 4.x, 4.4 Sep 30, 2024
@akien-mga akien-mga merged commit 0628af4 into godotengine:master Oct 1, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants