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

Only store _edit_use_anchors_ metadata if value is not the default #57863

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Feb 9, 2022

master version of #58254.

The default value is assumed to be false, so this metadata only needs to be stored if the value is true.

This closes #57862.

Bugsquad edit (keywords for easier searching): edit_use_anchors

@Calinou Calinou added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:editor labels Feb 9, 2022
@Calinou Calinou added this to the 4.0 milestone Feb 9, 2022
@rakkarage
Copy link
Contributor

The pull request build seems to work for me. New scenes no longer have that setting written into them it seems.

But during testing I ran into another issue. You cannot undo a toggle of the anchor button? Maybe I will open a new issue?

Thanks.

@Calinou
Copy link
Member Author

Calinou commented Feb 9, 2022

But during testing I ran into another issue. You cannot undo a toggle of the anchor button? Maybe I will open a new issue?

Does this happen without this PR?

@rakkarage
Copy link
Contributor

rakkarage commented Feb 9, 2022

Yes, in 3 and 4.

  • Make new Control scene and drag/move control then toggle use anchors
  • Scene->undo un-moves the control instead of the toggle

@Calinou
Copy link
Member Author

Calinou commented Feb 10, 2022

Yes, in 3 and 4.

* Make new Control scene and drag/move control then toggle use anchors

* Scene->undo un-moves the control instead of the toggle

Use Anchors is an editor-only action, so I don't think it makes sense to be tracked in undo/redo. It would be like tracking selection of 2D/3D nodes in the editor as an undoable action. There is no project data being changed here, only data being read. Most other applications don't track such operations in their undo/redo state.

@rakkarage
Copy link
Contributor

rakkarage commented Feb 10, 2022

Ya sorry, I agree, I guess I still thought it was doing the the _edit_use_anchors_ stuff :)
Thanks.

@aaronfranke
Copy link
Member

Really this should be moved out of the scene file. We don't need to store local editor specifics in version-controlled scene files, this kind of data would have a better home in the .godot/editor/ folder.

@YuriSizov
Copy link
Contributor

Really this should be moved out of the scene file. We don't need to store local editor specifics in version-controlled scene files, this kind of data would have a better home in the .godot/editor/ folder.

Do we have examples of other per-node properties stored in there? This is similar to _edit_lock_, which is also stored with nodes at the moment. There is also a question of how and if we should allow editing this programmatically, so that, say, a plugin could create a node and mark it to use anchors by default.

I agree in principle, but there is also an argument to be made that this needs to be synced and needs to be attached to the node.

@YuriSizov
Copy link
Contributor

This PR needs a rebase/remake since the relevant code is now in control_editor_plugin.cpp. It should mostly be the same, IIRC, but I'd still appreciate an update before reviewing it.

The default value is assumed to be `false`, so this metadata
only needs to be stored if the value is `true`.
@Calinou Calinou force-pushed the editor-use-anchors-metadata-no-redundant branch from 701c8fb to 7ca843b Compare February 14, 2022 00:27
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

The change seems to be straightforward.

@rakkarage has a point though. In one case we save this editor-only flag as a part of UndoRedo, and in another we don't. But that's existing behavior, so I think it's a non-blocker for this PR.

@akien-mga akien-mga merged commit 1d910b1 into godotengine:master Feb 15, 2022
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the editor-use-anchors-metadata-no-redundant branch February 15, 2022 18:05
@akien-mga
Copy link
Member

Could use a dedicated backport for 3.x as this code has changed somewhat.

@Calinou Calinou removed needs testing cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 17, 2022
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.

Unnecessary _edit_use_anchors_ metadata in scene files
5 participants