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

Reorganize Region Rect Editor #61342

Merged
merged 1 commit into from
May 24, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented May 23, 2022

Problem:

  • Region Rect was pretty much a mythical editor. Because it was annoying for it to pop up automatically, it was hidden by default.
  • Because it was hidden by default, most users have no idea it even exists.
  • But because it is a transient editor, it would steal focus of other editors and annoy users unexpectedly.
  • This editor only edits a sole property of objects (region_rect) and there is a general disconnect to it, so users have it very difficult to make the mental link of what is being edited exactly.

In general, Region Rect editor is a very weird editor, because it only edits the single region_rect property of objects that read a texture from a region, no other editor works quite like it.

Solution:

Because of the strange nature of this editor, a dedicated solution was implemented:

  • As its only useful to edit a very specific property in objects, the way to access it is now from that property via a button.
  • Editor has been moved to a window.
  • Regions that can be edited now display a "Edit Region" button (below the region property). Pressing it brings up the Region Rect editor.

This required a slight change in EditorInspectorPlugin to allow custom editors to be below others.

How it looks in action:

Regions now have a button you can press to edit them:
image
This opens the region editor in context, only when you need it:
image

What it fixes:

Fixes #28341
Fixes #47514
Fixes #29873

And probably many others.

@KoBeWi KoBeWi added this to the 4.0 milestone May 23, 2022
@KoBeWi
Copy link
Member

KoBeWi commented May 24, 2022

The editor should automatically center the texture whenever opened. Right now it opens in top-left corner for the first time.

The window is super big, you don't have live preview when editing the region (unless you have a second screen). It should be probably smaller.

Also it might be helpful to display some faint borders to denote texture's size.

@reduz
Copy link
Member Author

reduz commented May 24, 2022

@KoBeWi makes sense. I did not modify the editor itself though, just put it in a window. I think improvements to the editor could happen on a separate PR.

Problem:

* Region rect was pretty much a hidden editor. Because it was annoying for it to pop up automatically, it did not.
* Because it did not, most users have no idea it even exists.
* But because it is a transient editor, it would steal focus of other editor and annoy users.

Solution:

* Editor has been moved to a window.
* Regions that can be edited add a button below the region which can be pressed to open the editor.

This required a slight change in EditorInspectorPlugin to allow custom editors to be below others.
@reduz reduz force-pushed the reorganize-region-rect-editor branch from 2478f5b to 4044cc7 Compare May 24, 2022 07:42
@reduz reduz requested a review from a team as a code owner May 24, 2022 07:42
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Looks good overall. The add_to_end argument added to add_property_editor is welcome.

@reduz reduz merged commit 4dd6f56 into godotengine:master May 24, 2022
@Sslaxx
Copy link

Sslaxx commented May 24, 2022

Instead of the small button on the left, make the entire "Edit Region" thing a button? And perhaps rename it "Region Editor" to be consistent?

@groud
Copy link
Member

groud commented May 24, 2022

Instead of the small button on the left, make the entire "Edit Region" thing a button? And perhaps rename it "Region Editor" to be consistent?

I think it is already the case. The icon is just ragged left.
For the name I prefer "Edit region", as it clearer it's an action (opening the editor) IMO.

@KoBeWi
Copy link
Member

KoBeWi commented May 24, 2022

I did not modify the editor itself though, just put it in a window.

That's not good, because any shortcoming of that switch is now a usability regression. The editor moved from a small panel to a huge window, so you get this:
image
Your texture is now crammed in a corner and the first thing you see after opening the editor is huge empty space, which covers your whole editor, so you can't even preview the region changes on the scene. It's not that bad, because you can just adjust the window, but you also removed editor state for some reason, so it will be reset to obtrusive window every time you restart Godot.

region_editor->snap_mode_button->select(state["snap_mode"]);
bool EditorInspectorPluginTextureRegion::parse_property(Object *p_object, const Variant::Type p_type, const String &p_path, const PropertyHint p_hint, const String &p_hint_text, const uint32_t p_usage, const bool p_wide) {
if ((p_type == Variant::RECT2 || p_type == Variant::RECT2I)) {
if (((Object::cast_to<Sprite2D>(p_object) || Object::cast_to<Sprite3D>(p_object) || Object::cast_to<NinePatchRect>(p_object) || Object::cast_to<StyleBoxTexture>(p_object)) && p_path == "region_rect") || (Object::cast_to<AtlasTexture>(p_object) && p_path == "region")) {
Copy link
Member

Choose a reason for hiding this comment

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

Checking property of StyleBoxTexture is not necessary, as region_rect is the only Rect2 property in there.

@groud
Copy link
Member

groud commented May 24, 2022

That's not good, because any shortcoming of that switch is now a usability regression. The editor moved from a small panel to a huge window, so you get this:

This is a little bit exaggerated. On your screen the new window is maybe twice the height of what would be the bottom panel, so it does not go from small to huge, but from already too big to huge. So I would not call that a regression.

I think a simple fix would be to popup with a smaller size for now (like 50% of the editor window instead of the default value, or even automatically computed according to the texture size), but I agree the texture should be centered if possible too.

@KoBeWi
Copy link
Member

KoBeWi commented May 24, 2022

This is a little bit exaggerated. On your screen the new window is maybe twice the height of what would be the bottom panel, so it does not go from small to huge, but from already too big to huge. So I would not call that a regression.

Yeah but the window appears over the 2D editor and covers the whole viewport. Panel didn't have this problem.

I think a simple fix would be to popup with a smaller size for now

That's what I suggested in my first comment, but then this got merged :v

@groud
Copy link
Member

groud commented May 24, 2022

That's what I suggested in my first comment, but then this got merged :v

Oh my bad, I though the commit pushed by reduz after your comment addressed your feedback. :/
I should have checked before approving.

@reduz
Copy link
Member Author

reduz commented May 24, 2022

@KoBeWi I can do some fixes later if you wish (or you can PR them to use a smaller window ratio or further improvements). But I needed to merge this to work on the shader editor. But in any case, I think changing how the window pops up should be enough.

@neikeq
Copy link
Contributor

neikeq commented May 24, 2022

A button in the inspector to open a floating window is exactly how I thought it should be. The only problem I see is that the button looks just like another label in the inspector, not like a button. This could hinder discoverability.

@reduz
Copy link
Member Author

reduz commented May 24, 2022

@neikeq I agree, but we are using all buttons like this in the inspector for everything so I did not want to make it different. We really need to discuss how to improve the readability.

@KoBeWi
Copy link
Member

KoBeWi commented May 24, 2022

Is something like this better?
image
The problem is that a non-flat button doesn't have borders, but just slightly lighter background. Maybe we could add some theme override here?

@reduz
Copy link
Member Author

reduz commented May 24, 2022

@KoBeWi I suppose its better, but you should check other buttons, like Add Metadata and the ones for array management so we use something unified.

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