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

Improved Inspector Sub-Resource Editing #45907

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

reduz
Copy link
Member

@reduz reduz commented Feb 11, 2021

Based on ideas from @LucyLavend and @RPicster, posted in godotengine/godot-proposals#2230

  • Better margins
  • Colors to delimit subresources better.

How it looks:

image

Bugsquad edit: This closes godotengine/godot-proposals#2230.

@RPicster
Copy link
Contributor

Can the user somehow manipulate the colors? Otherwise I feel they are a bit too strong / saturated.

@JulienVanelian
Copy link

Can the user somehow manipulate the colors? Otherwise I feel they are a bit too strong / saturated.

In that case an editor setting should give the ability to toggle or tweak the saturation of the colors, maybe?

@barbaros83
Copy link

looks great, easy to distinguish between sub resources

@EricEzaM
Copy link
Contributor

EricEzaM commented Feb 11, 2021

Rather than rotate the hue through the colours (16 of them it seems), why not just add editor theme settings for each level which can be modified by users? Then they can retain complete control over the theme of the editor. It doesn't have to be many - only 5 perhaps. Consider how many times you are going into sub resources which are 5+ levels deep?

I think having 5 or so and then looping back to the first colour if you have more than 5 would be fine: 1 > 2 > 3 > 4 > 5 > 1 > 2... etc. You would still be able to easily distinguish each section since the repeated colours would be deeply nested and would not get confused with one another.

Overall it is a good usability change though!

@LucyLavend
Copy link

Seeing the feedback on Twitter and Discord, there seem to be quite a lot of people that prefer to have the resources highlighted with blue only. Because of this it might be good to have an option to turn off the automatic coloring. Another way would be to have a modifiable array of colors like @EricEzaM mentioned, so that the user can set them all to blue if preferred.

@reduz
Copy link
Member Author

reduz commented Feb 11, 2021

I added this small setting in the inspector dock sections (not the theme) for those with a dark heart who can't stand colors. Put it to 0 and the color goes away. I set it to 0.75 by default which imo looks best (less saturated that in the screenshot above, but still very readable).
image

@reduz
Copy link
Member Author

reduz commented Feb 11, 2021

@EricEzaM Let's see how many find that to be a problem eventually and then we discuss again. I prefer automatic because i can ensure the colors are contrasting and the usability is good. It also works great because it's based on the accent color so, if you change it (or change from dark to light theme) it always finds contrasting colors.

On the other side, having to tweak these every time you tweak your theme to make it contrasting is a hassle that most users will not be happy about. The new theme system is designed to be hassle free (few things change lots of settings) and this has been a win so far, so better not return to the "lots of settings" theme.

@reduz reduz force-pushed the improved-inspector-subresources branch 2 times, most recently from dcec69d to 47afe53 Compare February 11, 2021 23:16
@RandomShaper
Copy link
Member

RandomShaper commented Feb 11, 2021

That will help a lot.

@hoontee
Copy link
Contributor

hoontee commented Feb 12, 2021

Very nice change, thank you!

@alexfreyre
Copy link

I dont know if is too late, but I have this ideas based on what is presented in this PR

solution_1
solution_2

@reduz
Copy link
Member Author

reduz commented Feb 12, 2021

@alexfreyre TBH we explored a lot of options in the past too, and the colors are by far what works better. In your images it's not entirely clear what's going on in comparison.

I am a pragmatical person, so if something objectively works better at its given task, and then it's the best solution. The "consistency" arguments are subjective and thus irrelevant, you will just get used to it like users did to a lot of changes I am not going to mention that were controversial at the time but nowadays no one cares any longer.

@alexfreyre
Copy link

alexfreyre commented Feb 12, 2021

@reduz totally agree, my intention with the previous images was to show another solution. Any simple task can be time consuming if we have no alternatives. I am not trying to change the idea of this PR, it is very good, but I want to contribute some more.

With colors the problem is solved, without doubts.

What seems to me that three different colors separate three elements that are integrated.

what I wanted to do with the tones was to integrate the elements into a tonal group, not three families of different colors, although it can also be a group of analogous colors within the same family or same color with different values.

for example,

Group Environment = blue family

  • subgroup1 = blue
  • subgroup2 = red + blue
  • subgroup2 = green + blue

then they are integrated, I think that this small detail would make any place have a visual hierarchy with unified groups and subgroups.

@API-Beast
Copy link

API-Beast commented Feb 12, 2021

grafik

This was a suggestion I made on Twitter. Basically same separation by hue, however with the hue steps much smaller. I would suggest hand-picking three colors and linearly interpolating between them based on the level of nesting. (1. -> 2. second color for the most common nesting depths, 2. -> 3. color for deeply nested)

@akien-mga
Copy link
Member

I like @API-Beast's "After" suggestion, but would suggest that contrast is increased on hover. This way you have something not too visually jarring by default, but when you do edit those options, you get a better contrast to make nesting much clearer (while keeping similar hues, like a typical hover effect would).

@RPicster
Copy link
Contributor

grafik

This was a suggestion I made on Twitter. Basically same separation by hue, however with the hue steps much smaller. I would suggest hand-picking three colors and linearly interpolating between them based on the level of nesting. (1. -> 2. second color for the most common nesting depths, 2. -> 3. color for deeply nested)

While I agree that it looks more stylish (apart from the colors getting basically all purplish, but that's taste), I think the contrast is much less resulting in a harder to differentiate subresources.
The idea of jumping between hues is to give a good contrast.
Also think about accesibility here, just moving along a hue give a hard time for colorblind people (and there are lot of colorblind people! Don't underestimate that)

You can upload your example image here:
https://www.color-blindness.com/coblis-color-blindness-simulator/

And you will quickly notice that the contrast is muss smaller and sometimes almost non existant.

@API-Beast
Copy link

grafik

If this chart is accurate then a transition between blue and yellow should be discernible for all types of color blindness.

@RPicster
Copy link
Contributor

RPicster commented Feb 12, 2021

If this chart is accurate then a transition between blue and yellow should be discernible for all types of color blindness.

The chart is correct. But compared to the palette that is used in the "After" image, the result is a different one.
The gradient you used in the example image goes from blue - through reddish hues - to yellow. So it includes hues that will be indistinguishable for many forms of colorblindness.
Also the hue differences are much too small.

I compiled a comparison of the first three sublevels of resources comparing the "before" to the "after" colorschemes when "emulated" with different forms of colorblindness:
contrast

As you can see, the colors on the right are not so easy to differentiate as on the left(which this color selection should be about).

It could be a solution to not move along the gradient but jump between end and start for each level to give maximum contrast between the subresource levels.

hue_contrast

Like this maybe... (Warning, super quick mockup 😉 )

I'm not doing a crusade here, I most importantly think, the user should have some kind of influence about the colors (Starting hue, saturation or controlling the colors directly)

@reduz
Copy link
Member Author

reduz commented Feb 12, 2021

Ok, I did some change and I will leave it at this, with a softer gradient but enough to be able to tell between sections.
Keep in mind this is a very extreme case of nesting, you won't get into so much normally (just one, at much two levels), so imo this is good enough.

image

@reduz reduz force-pushed the improved-inspector-subresources branch 2 times, most recently from 9350c47 to 3c9a9ec Compare February 12, 2021 12:22
@reduz
Copy link
Member Author

reduz commented Feb 12, 2021

Also since usability has considerably improved now, I made it possible again to edit materials inline.

for (int i = 0; i < 16; i++) {
Color si_base_color = accent_color;

float hue_roate = (i * 2 % 16) / 16.0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
float hue_roate = (i * 2 % 16) / 16.0;
float hue_rotate = (i * 2 % 16) / 16.0;

Comment on lines 734 to 735
Ref<StyleBoxFlat>
sub_inspector_bg = make_flat_stylebox(dark_color_1.lerp(si_base_color, 0.08), 2, 0, 2, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ref<StyleBoxFlat>
sub_inspector_bg = make_flat_stylebox(dark_color_1.lerp(si_base_color, 0.08), 2, 0, 2, 2);
Ref<StyleBoxFlat> sub_inspector_bg = make_flat_stylebox(dark_color_1.lerp(si_base_color, 0.08), 2, 0, 2, 2);

Copy link
Member Author

Choose a reason for hiding this comment

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

clang format is doing this for some reason, I can't fix it. Guess I will rewrite the code so it does not.

sub_inspector_bg->set_draw_center(true);

theme->set_stylebox("sub_inspector_bg", "Editor", sub_inspector_bg);
for (int i = 0; i < 16; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we even need to go to 16? If the impact on editor start performance is negligible it's fine, but otherwise I think 8 might be plenty enough, especially if it rotates.

Copy link
Member Author

Choose a reason for hiding this comment

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

performance is not a problem, and it just ensures the rotation is slow and smooth in case it needs to be.

-Better margins
-Colors to delimit subresources better.
@reduz reduz force-pushed the improved-inspector-subresources branch from 3c9a9ec to b9b68b7 Compare February 12, 2021 12:34
@RPicster
Copy link
Contributor

One question, will this change als be made available for the Godot 3 branch?

@akien-mga akien-mga merged commit bc55238 into godotengine:master Feb 12, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

One question, will this change als be made available for the Godot 3 branch?

This could possibly be backported to the 3.2 branch if a contributor is motivated. I doubt @reduz would do it himself as he prefers (and should) focus on getting 4.0 feature complete instead.

That being said, this kind of change is usually iterative and we might want to wait for things to be fine-tuned in the master branch before considering a backport.

@Xrayez
Copy link
Contributor

Xrayez commented Feb 12, 2021

I use subresources workflow quite often, if these changes are not too different from the 3.2 branch, I think I'd be interested in backporting this to 3.2 myself, unless there's someone else who'd be willing to pick this up/have more experience in developing the editor UI, I'd be interested to test out the PR at least.

@Xrayez
Copy link
Contributor

Xrayez commented Feb 13, 2021

That being said, this kind of change is usually iterative and we might want to wait for things to be fine-tuned in the master branch before considering a backport.

See #45940 for 3.2.

I'd say that most people actually use 3.2 currently, this also applies to a lot of engine contributors, and there are people who cannot even properly use/launch master branch on their machines, so it makes sense to me to merge #45940 now to get more feedback than waiting for months to get master stabilized.

@YuriSizov
Copy link
Contributor

I think the point of waiting is to make sure we don't run into a change later on that would be breaking in 3.2 and thus impossible to fix without releasing at least 3.3, which is not on the radar yet. This won't be merged into 3.2.4 anyway, since it's in RCs now, so there is no need to rush with a backport.

@Xrayez
Copy link
Contributor

Xrayez commented Feb 13, 2021

According to what I've seen, usability PRs usually get merged regardless of release status. See #45902 for example which was merged just yesterday.

Unlike #45902, #45940 is mostly cosmetic and does not introduce any compatibility breakage (it's internal change). Especially that I've tested it myself (and urge anyone to do the same testing, because this feature is great). 🙂

That said, I'm fine if this is postponed to 3.2.5...

@eon-s
Copy link
Contributor

eon-s commented Feb 17, 2021

One question, will this change als be made available for the Godot 3 branch?

This could possibly be backported to the 3.2 branch if a contributor is motivated. I doubt @reduz would do it himself as he prefers (and should) focus on getting 4.0 feature complete instead.

That being said, this kind of change is usually iterative and we might want to wait for things to be fine-tuned in the master branch before considering a backport.

It may be good to use 3.x as testing ground of that harmless feature, considering how long it will take for 4 to be released (and "feature preview" approaches seem to give good results for early tests on other popular tools).

@yosoyfreeman
Copy link

There is any way to disable this?

@YuriSizov
Copy link
Contributor

There is any way to disable this?

How do you imagine it should look, when disabled?

@Xrayez
Copy link
Contributor

Xrayez commented Feb 26, 2021

@yosoyfreeman you can try to set docks/property_editor/subresource_hue_tint editor settings to 0.0.

@yosoyfreeman
Copy link

@yosoyfreeman you can try to set docks/property_editor/subresource_hue_tint editor settings to 0.0.

Thanks you! This is a nice update anyway

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.

Sub-resource editing RFP (Request for Proposals).