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

Allow multiple contextual editors opened at once #45085

Closed
wants to merge 1 commit into from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jan 11, 2021

Godot has a known limitation that allows only one editor active at a time, which means you can't e.g. edit TextureRegion and ItemList items at the same time or TileMap and Shader etc. Main reason is that when you select a new node/resource, all previously opened editors are closed, because Godot focuses solely on that single item.

The biggest problem in solving this is ensuring that unused editors are closed. The approach I came up with is besides storing the list of currently used editors, store their owners. So e.g. you edit a Shader then the shader editor is opened, with the property as owner. If the property gets folded or removed from the inspector, shader editor's owner is no longer "valid" and it gets closed.
Otherwise it remains opened. This way you can have opened any combination of editors at the same time and they properly get closed if not needed anymore.

(That's the theory anyways. Submitting as draft, because closing editors doesn't fully work yet and I even managed to cause crash in Object::cast_to(). Hopefully I'll be able to finish this properly)

Fixes #28211
Fixes #32002
Fixes #28341
Fixes #47514
Fixes #29873
Bugsquad edit: Fixes #43635
And other similar issues if there are any.

EDIT:
Also screenshot for hype
image

@groud
Copy link
Member

groud commented Jan 12, 2021

The idea seems good, though I fear that it may cause issues is some cases.

Right now, as I am working on the TileSet and TileMap editors, I chose to create a single plugin able to edit both resources. It's not working great as, when I edit the TileSet resource, it's like I was not editing the TileMap anymore. Maybe this can solve the problem, but it does not seem that it allows a single plugin to edit two objects at the same time (maybe I am wrong though). So even if it does not solve the issue, I hope that it wont create more.

I am no expert of the EditorNode part has it's quite a huge mess. But globally I'd say this is an improvement, and can be merged, but we will have to watch closely for potential regressions in the future.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 12, 2021

Maybe this can solve the problem, but it does not seem that it allows a single plugin to edit two objects at the same time (maybe I am wrong though).

This haven't changed. Plugin will just call edit() on current object and if you want to edit two of them, you need to handle it yourself somehow.

@KoBeWi KoBeWi force-pushed the a_pile_of_editors branch from 257c52d to 196596b Compare January 12, 2021 13:06
@KoBeWi KoBeWi marked this pull request as ready for review January 12, 2021 13:09
@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 12, 2021

Ok, marked as ready for review, but probably has bugs and weird stuff (seems to work after brief tests though).

Summary of changes:

  • when new Resource is edited, ActiveEditor struct is added to the list of opened editors (previously the list held only plugins)
  • the struct contains the editor reference and its "owner
    • when clicking node in scene, the "owner" is this node
    • when unfolding resource inspector, "owner" is that EditorPropertyResource
  • I changed hide_top_editors to hide_unused_editors
  • on each selection/folding/unfolding, list of the editors is verified based on their owners
    • if owner is Node, Godot will check if it's the main selected node in scene tree (I added a property last_pushed_item to make this work)
    • if owner is EditorPropertyResource, Godot will check whether the property is unfolded and visible (i.e. parent properties are unfolded too)
    • based on that, unused editors are removed
  • I changed behavior of _fold_other_editors. Now it only folds properties that share the same editor

@KoBeWi KoBeWi force-pushed the a_pile_of_editors branch from 9dbbca7 to d627bd7 Compare March 18, 2021 22:27
@KoBeWi KoBeWi requested a review from a team as a code owner March 18, 2021 22:27
@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 19, 2021

Say in the new Theme Editor we would still rely on the Inspector dock for editing sub-resources. So in the ThemeEditor we have a panel with a list of styleboxes.

Example 1

Clicking on one would open it as an edited resource in the Inspector, which means that the Theme resource itself is no longer open there. Which means that the Theme Editor is also closed now.

Example 2

While I do want to make a new editor for StyleBoxes, I don't really want to reimplement an editor for all texture types. So this would be annoying... Clicking back in the Inspector brings the user back to the Theme resource and the editor is restored where we left it, but still.

Does this PR allow this to not be the case somehow? Or can we make it so, in this or a future PR that improves upon this one?

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 19, 2021

This sounds like #28341, which is fixed by this PR.

@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 19, 2021

In #28341 the problem is that the TextureRegion has its own bottom panel editor, is it not? So the issue presents itself while both the main resource and the sub-resource are open in the Inspector dock. In my example there is no editor associated with a sub-resource. It's just that the sub-resource takes the Inspector dock for itself, therefore the main resource is no longer open in it.

In other words, it's the same as when you disable this option:

image

2021-03-19_17-18-46.mp4

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 19, 2021

Not sure then. In worst case it can be solved by setting some custom owner for the Theme editor, so it's possible at least.

@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 19, 2021

Just for future reference: TileSet Editor works around this by using a special TilesetEditorContext object, that it can also edit, and storing a reference to the parent TileSet in the TilesetEditorContext. 🤔

I've been told that this is no longer the case in the remake by groud.

@Anutrix
Copy link
Contributor

Anutrix commented May 31, 2022

This seems awesome.
What's the status on this?
And how will it be affected by Reduz's #61459.

Also, just want to add that 4(#28211, #28341, #47514, #29873) of the 6 mentioned tickets in first comment seems to be closed already.

@YuriSizov
Copy link
Contributor

Status is that those reduz's PRs take priority for the moment, and will likely make this one obsolete.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 16, 2023

Closing this. The PR is not completely obsolete (2 of the linked issues are still open), but it's difficult to rebase and the implementation could be improved. I have some idea for a bit different implementation and I'd also want to solve #13849 at the same time. The new PR would supersede this one.

@KoBeWi KoBeWi closed this Jan 16, 2023
@KoBeWi KoBeWi deleted the a_pile_of_editors branch January 18, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment