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

Usability & Quality of life: Add shortcut widget to change brush size and opacity #470

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

GustJc
Copy link
Contributor

@GustJc GustJc commented Nov 25, 2024

A much needed quality of life feature.
This adds a simple widget to quickly change the size and opacity of the brush.
Press the hotkey and move the mouse the set the value.

heighmap_demo_1.mp4

Right now, the hotkeys are set to:

  • G for size
  • H for opacity

We can add an option to change the hotkeys later.

This PR also fixes a bug where changing the size/opacity in the brush settings dialogue would not change the bottom panel sliders

heighmap_fix_1.mp4

Based on TerraBrush's UI features

selector.brush_preview_color = widget_color

_overlay_selector = selector
_overlay_selector.position = active_viewport.get_global_mouse_position()
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if it's not already possible to infer viewport-relative mouse position from parameters of _forward_3d_gui_input. I'm a bit annoyed that Godot doesn't seem to expose enough things

@Zylann
Copy link
Owner

Zylann commented Nov 25, 2024

Some additional thoughts:

  • It's not easy to discover these shortcuts, and I'm also wondering if existing shortcuts might come in conflict with it. The probability is low, but who knows.
  • It's not easy to realize which hotkey changes what. Both have the same color. Maybe it should at least show the name of the brush property being changed.
  • I was wondering how much sense it makes for opacity to be shown with a circle that grows and shrinks. But I'm not sure what else.
  • I was also wondering if rather than a 2D widget it could have scaled the decal directly. But the same question as above arises regarding opacity.
  • I noticed the Escape key collapses editors. But then it doesn't get rid of the widget, which feels wrong.

@GustJc
Copy link
Contributor Author

GustJc commented Nov 25, 2024

  • It's not easy to discover these shortcuts, and I'm also wondering if existing shortcuts might come in conflict with it. The probability is low, but who knows.

I personally use "G" for 'translate transform' to move nodes. But it didn't give me any problems as the plugin consumes the input and it only affects it when the node is selected. But that's why the next step would be to allow changing the hotkeys. In another PR.

  • It's not easy to realize which hotkey changes what. Both have the same color. Maybe it should at least show the name of the brush property being changed.

Oh yea, I intended to change the opacity color border to red but ended up forgetting about it.
But also showing the name might be a good thing.

  • I was also wondering if rather than a 2D widget it could have scaled the decal directly. But the same question as above arises regarding opacity.

That's an option. In that case, should there even be an overlay widget?
I quite like the overlay UI as most programs use them and they feel natural and precise with the numbers display.
But setting a size in the overlay and having the decal be a different size is a bit strange sometimes. But usually its all right.

  • I noticed the Escape key collapses editors. But then it doesn't get rid of the widget, which feels wrong.

Oh, I didn't test that. I'll make it cancel the widget when Escape is pressed.

@GustJc
Copy link
Contributor Author

GustJc commented Nov 26, 2024

I've made the changes.
Now it also properly works with multiple viewports.

heighmap_demo_2.mp4

@@ -86,6 +88,10 @@ var _mouse_pressed := false
#var _pending_paint_action = null
var _pending_paint_commit := false

var _overlay_selector : HT_BrushEditorOverlay = null
var _widget_action_name : String = ""
var _editor_viewports : Array[Node] = []
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't it be Array[SubViewportContainer]?

Copy link
Contributor Author

@GustJc GustJc Nov 26, 2024

Choose a reason for hiding this comment

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

The find_children used to get the nodes returns an Array[Node] so Godot doesn't let me.
Edit: I tried manually casting it, but Godot still complains for some reason

@GustJc
Copy link
Contributor Author

GustJc commented Nov 27, 2024

Made the changes. I'm using an exponential range as you suggested. From a hidden slider inside the overlay to convert it.

I've also changed the overlay update from process to physics_process.
Not sure if it makes much difference, but it should save a few frames of useless calculations.

@GustJc
Copy link
Contributor Author

GustJc commented Nov 27, 2024

Removed some useless code while getting the _editorviewport
The editor_viewport used encompasses the whole godot window, so there is no need to check if a viewport window is active or not. (which is was not doing anyways after the first submitted code)

It can even display the overlay on top of other panels.
But I think that is okay.

_exponential_slider.min_value = MIN_UI_CIRCLE_SIZE*_dpi_scale
_exponential_slider.max_value = MAX_UI_CIRCLE_SIZE*_dpi_scale

var reverse = (initial_value - min_value) / (max_value-min_value)
Copy link
Owner

Choose a reason for hiding this comment

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

Untyped, is it float?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me just comment once here.
Sorry for all the missing statics. My head was kinda floating after bumping my head against the wall trying to figure out how to make that math works. Specially making the start position offset work was more difficult than I was hoping.

HT_Brush.MAX_SIZE_FOR_SLIDERS,
Color.LIGHT_GREEN, _terrain_painter.get_brush_size(),
"Brush Size",
func on_value_changed(value):
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not used to the new lambda feature in GDScript, but I found out giving a name is optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. The name is optional.
But I think having the name there is helpful to know at a glance what the callback does without checking the function declaration.

But I can remove it if you want to?

@GustJc
Copy link
Contributor Author

GustJc commented Nov 28, 2024

Changes made.

@Zylann Zylann merged commit dd14984 into Zylann:master Nov 28, 2024
3 checks passed
@Zylann
Copy link
Owner

Zylann commented Nov 28, 2024

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants