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

Improve editing of min/max particle properties #81260

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Sep 2, 2023

Adds this fancy widget for particles:

f4KJJapiWI.mp4

It can edit min and max as part of the one slider, including dragging them together and scaling. Old sliders are still provided for more precise values and values outside the default range. There is also a new slider mode where you can edit pivot and deviation instead of min and max.

This required some internal changes to ParticleProcessMaterial, namely the min/max properties have now third property, which is a vector of both min and max. This was needed to add inspector plugin. The linked properties are editor only, while the old properties are code-only and stored in scene. I added a convenience macro to make binding min/max properties easier.

Closes godotengine/godot-proposals#7583
Closes godotengine/godot-proposals#4315

Builds on top of #79527, so putting as draft until that PR is merged.

EDIT:
Here's new style:

Ea6ledp2yv.mp4

Production edit: closes godotengine/godot-roadmap#17

@Calinou
Copy link
Member

Calinou commented Sep 2, 2023

Looks great!

For those wondering, you can change deviation without unfolding the inspector by holding Shift before dragging from anywhere with the left mouse button (you don't have to aim for the min/max icon).

Shift is used to scale the widget. Needs to be more obvious

We can add a tooltip when hovering the slider at the very least, similar to EditorSpinSlider.

Copy link
Contributor

@MewPurPur MewPurPur left a comment

Choose a reason for hiding this comment

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

Some initial lookover. I believe we need to decouple this from particles and just call it MinMaxPropertyEditor. I'll post UX suggestions as a comment in a bit.

doc/classes/ParticleProcessMaterial.xml Outdated Show resolved Hide resolved
@MewPurPur
Copy link
Contributor

MewPurPur commented Sep 3, 2023

This is a downgrade for me personally, as I usually use the line edits to write my values.

I suggest we change this:

image

I think instead of numbers, we should have two EditorSpinSliders that can be edited while you're not using the widget.

This means that this could be simplified:

image

Instead, the arrow could popup a context menu with the two options - Range, Pivot - to change which values are shown. This would save A LOT of vertical space.

What if you want to see min/max easily while editing pivot mode, for example? IMO, this should be a tooltip while you're hovering the slider.

Speaking of hovering, the slider needs hovering and dragging feedback. In general, I suggest making the colors similar to scrollbars in the editor.

I think the grabbers to the side of the slider are way too big and could be confused for part of the range. In my opinion, shrink their width, and expand the click area only programmatically. Make them a different darker color, maybe.

Edit: The arrow doesn't even have to be an arrow if this is done, it can just be a toggle button that changes between the two methods, this would make it even easier to use.

@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch 4 times, most recently from 6535686 to de6f566 Compare October 10, 2023 21:48
@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch from de6f566 to 39df870 Compare November 18, 2023 20:55
@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 18, 2023

I'm back to working on this PR. Pushed a fix for EditorProperty.

@MewPurPur How do you imagine replacing the labels with EditorSpinSliders? There is enough space only to display min/max, with pivot/deviation it would get extremely cramped. Unless the sliders are displayed in 2 rows, which would mean that the editor would take 3 rows by default (currently it takes two). I wanted it to take as small vertical space as possible.

@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 18, 2023

We can also use icons, it would also help us not worry about localizations. If not, then I don't see much harm in doing µ and σ for pivot and deviation.

Ugly mock-up:

image

A button to toggle which mode to use could be snuggled on the right of these.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 18, 2023

Then I'd need icons for min, max, pivot and deviation. Not sure if µ and σ would be recognizable.
As for your mock-up, I don't think it's possible with EditorSpinSlider, unless I add a Label that would partially overlap it 🤔

@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 18, 2023

If you think this is a good direction to pursue, just ping me when you need the icons

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 18, 2023

image
I'll go with piv and dev for the other mode. The advantage of labels is that EditorSpinSlider has them built-in and they disappear while editing, so it should be fine.

@MewPurPur
Copy link
Contributor

Looks fantastic IMO! The next thing I think needs to be done in terms of GUI is to:

  • Make the handles only appear when you're hovering over the widget.
  • Add some kind of clear indication of where the pivot is.
  • Make the color more gray-ish generally, lighter gray when you're hovering, bluish while you're dragging. This also holds for the handles. When you're pressing Shift, highlight both handles (as they are what the operation would affect).

Ugly mock-up with the right handle being hovered:

image

@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch from 39df870 to 3859ffe Compare November 19, 2023 18:58
@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 19, 2023

Pushed the new layout (still need to apply other changes).

@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch 2 times, most recently from 716bca0 to 14d2983 Compare November 19, 2023 20:50
@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 19, 2023

Finished.

godot.windows.editor.dev.x86_64_XBCh54XPgb.mp4

EDIT:
I also came up with outline color change (not pushed):

godot.windows.editor.dev.x86_64_nsO8pGigdE.mp4

Does it make sense? The "active" color is the same as the middle bar, so not sure.

@KoBeWi KoBeWi marked this pull request as ready for review November 19, 2023 20:51
@KoBeWi KoBeWi requested review from a team as code owners November 19, 2023 20:51
@KoBeWi KoBeWi requested a review from QbieShay November 19, 2023 20:52
@Geometror
Copy link
Member

@QbieShay, @TheRensei and I discussed the naming of the parameters again and settled with value and the plus-minus sign (U"\u00B1"). What do you think?

grafik

Disclaimer: The following wasn't part of the discussion
To make it clearer you could call val base value and +/- maybe spread in the tooltips?

@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch from fafde43 to a424439 Compare December 19, 2023 16:43
@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 19, 2023

Maybe grabbing outside of the range could already cause the central pivot to move without changin the interval, without holding down control or shift?

Something like that?

godot.windows.editor.dev.x86_64_sAw10cDTAR.mp4

I removed the modifier. Now clicking anything outside arrows will move the center.

@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch from a424439 to dd74dc5 Compare December 19, 2023 16:51
@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch from dd74dc5 to c0e37ce Compare December 19, 2023 20:22
@QbieShay
Copy link
Contributor

I tested again and i had concerns about the backwards compatibility with animation player possibly animating min-max properties, but im happy to report it works now (it's also not a common usecase as far as i know).

Not necessary for this PR, but i think that the shift modifier for making smaller change would help also this feature 👍

@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch from c0e37ce to e7dc615 Compare December 19, 2023 23:09
@QbieShay
Copy link
Contributor

Going below limit with avg +- doesn't work, there's some quirks there. also the slider of the +- is being funny when moved 🤔

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 20, 2023

Going below limit with avg +- doesn't work

Not sure what do you mean by that.

also the slider of the +- is being funny when moved

That's because the max value adjusts to be within the range.

@QbieShay
Copy link
Contributor

Sorry - I shoudl articulate better.

In the avg mode (not minmax), i think the sliders are clamped to the min or max, even if the property specifies or_greater or or_lesser. it works in minmax mode.

I don't think it's a blocking issue anyway.

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 20, 2023

Ok I made lesser/greater allowed in the second mode. However the max value of ± can't be greater than max - min of the property. Also ± only allows "or greater" when both min and max can go outside range.

@KoBeWi KoBeWi force-pushed the min_maxing_particle_editing branch from e7dc615 to ce9fec9 Compare December 20, 2023 05:44
@QbieShay
Copy link
Contributor

just tested, NICE 👌

I can't approve again but i APPROVE

Comment on lines +357 to +361
background_color = dark_theme ? Color(0.3, 0.3, 0.3) : Color(0.7, 0.7, 0.7);
normal_color = dark_theme ? Color(0.5, 0.5, 0.5) : Color(0.8, 0.8, 0.8);
hovered_color = dark_theme ? Color(0.8, 0.8, 0.8) : Color(0.6, 0.6, 0.6);
drag_color = hovered_color.lerp(accent_color, 0.8);
midpoint_color = dark_theme ? Color(1, 1, 1) : Color(0, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to put stuff like this into the editor theme. But we don't have a good reference how to do this in an organized manner. A task for another day...

hb->add_child(toggle_mode_button);
toggle_mode_button->connect(SNAME("toggled"), callable_mp(this, &ParticleProcessMaterialMinMaxPropertyEditor::_toggle_mode));

set_bottom_editor(content_vb);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mark the range widget as focusable with add_focusable? Or can it not be controlled with the keyboard at all?

Copy link
Contributor

@YuriSizov YuriSizov Dec 20, 2023

Choose a reason for hiding this comment

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

Can be done in a follow-up, if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not controllable with keyboard. Focus would help with #81260 (comment) (last point), but it's a plain Control and has no focus style.

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.

LGTM

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Dec 20, 2023
@YuriSizov YuriSizov merged commit bc6be82 into godotengine:master Dec 20, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks! Amazing usability improvement for particles 🌟

@KoBeWi KoBeWi deleted the min_maxing_particle_editing branch December 20, 2023 14:17
@golddotasksquestions
Copy link

golddotasksquestions commented Dec 21, 2023

This is really fantastic to use!! What a great UI improvement! Thanks KoBeWi and thanks also to everyone commenting with improvement suggestions, especially MewPurPur!

I would really love to use this slider (in countless places really) in my projects. Both for exported values in the Inspector as well as Control node.

I tried to figure out if it is already available outside the GPUParticle node, but it seems to be it is not. Would it make sense to open a proposal?

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 21, 2023

I think this could be exposed using a special property hint. The only problem is that right now it's basically hard-coded into the particle material, so the new editor needs some internal rework. It's not very difficult to replicate in GDScript though, I did a prototype before opening the PR.

But yeah, opening a proposal wouldn't hurt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
8 participants