From 7254dbedb3aec0b41e3ce03690c44f863976cd62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristoffer=20S=C3=B8holm?= Date: Sat, 23 May 2026 17:15:02 +0200 Subject: [PATCH] Fixes for buttons in the tool panel I noticed that clicking some of the tool buttons didn't work, and after some (AI-assisted) triaging I realized that a bunch of the drag operations were firing when clicking and immediately going back to object mode, plus some insufficient interaction guards. After fixing this the object mode button no longer worked, which I fixed by always dismissing active modals. The whole system feels very brittle and it would be surprising if it's entirely correct now, even though I didn't find any issues when clicking around a bit. This part of the editor needs better abstractions so the individual operators can express their intent and not deal with the underlying implementation. --- src/brush_drag_ops.rs | 11 +++++++++ src/core_extension.rs | 16 +++++------- src/measure_tool.rs | 55 +++++++++++++++++++++++------------------- src/viewport.rs | 14 ++++++++++- src/viewport_select.rs | 46 +++++++++-------------------------- 5 files changed, 71 insertions(+), 71 deletions(-) diff --git a/src/brush_drag_ops.rs b/src/brush_drag_ops.rs index 7fa227a..96fb510 100644 --- a/src/brush_drag_ops.rs +++ b/src/brush_drag_ops.rs @@ -111,11 +111,18 @@ pub(crate) fn face_drag_invoke_trigger( keybind_focus: KeybindFocus, modal: Res, draw_state: Res, + vp: ViewportCursor, mut commands: Commands, ) { if !mouse.just_pressed(MouseButton::Left) || drag_state.active || drag_state.pending.is_some() { return; } + + // Only trigger when inside the viewport + if vp.viewport_entity().is_none() { + return; + } + let in_face_edit = matches!(*edit_mode, EditMode::BrushEdit(BrushEditMode::Face)); let shift = keyboard.any_pressed([KeyCode::ShiftLeft, KeyCode::ShiftRight]); let alt = keyboard.any_pressed([KeyCode::AltLeft, KeyCode::AltRight]); @@ -686,6 +693,7 @@ pub(crate) fn vertex_drag_invoke_trigger( edit_mode: Res, drag_state: Res, keybind_focus: KeybindFocus, + vp: ViewportCursor, mut commands: Commands, ) { if !mouse.just_pressed(MouseButton::Left) @@ -693,6 +701,7 @@ pub(crate) fn vertex_drag_invoke_trigger( || drag_state.active || drag_state.pending.is_some() || keybind_focus.is_typing() + || vp.viewport_entity().is_none() { return; } @@ -1068,6 +1077,7 @@ pub(crate) fn edge_drag_invoke_trigger( edit_mode: Res, drag_state: Res, keybind_focus: KeybindFocus, + vp: ViewportCursor, mut commands: Commands, ) { if !mouse.just_pressed(MouseButton::Left) @@ -1075,6 +1085,7 @@ pub(crate) fn edge_drag_invoke_trigger( || drag_state.active || drag_state.pending.is_some() || keybind_focus.is_typing() + || vp.viewport_entity().is_none() { return; } diff --git a/src/core_extension.rs b/src/core_extension.rs index 16a71a9..7ea9bdb 100644 --- a/src/core_extension.rs +++ b/src/core_extension.rs @@ -45,16 +45,12 @@ fn dispatch_button_operator_call( .map(|(k, v)| (k.to_string(), v.clone())) .collect(); commands.queue(move |world: &mut World| { - // If the target is a modal operator, cancel any in-flight - // modal first. Lets the user switch tools (Draw Brush, - // Measure Distance, brush-element drags, terrain sculpt, ...) - // by clicking another toolbar button without reaching for - // Escape, and keeps the second dispatch from failing with - // `ModalAlreadyActive`. Extensions that wire their own - // operators to buttons inherit this behavior for free. - if let Ok(true) = world.operator(id.clone()).is_modal() { - let _ = world.operator("modal.cancel").call(); - } + // Treat every toolbar button as a peer of any active tool: cancel + // whatever modal is running first, then dispatch. Without this, + // clicking Object Mode (or any other non-modal mode button) while + // Draw Brush / Measure / etc. owns the modal slot is silently + // blocked by their `is_available` checks. + let _ = world.operator("modal.cancel").call(); let mut call = world.operator(id.clone()).settings(CallOperatorSettings { execution_context: ExecutionContext::Invoke, diff --git a/src/measure_tool.rs b/src/measure_tool.rs index c4d6aed..7b08b3e 100644 --- a/src/measure_tool.rs +++ b/src/measure_tool.rs @@ -100,12 +100,30 @@ pub(crate) fn measure_distance( vp: crate::viewport::ViewportCursor, mut ray_cast: MeshRayCast, ) -> OperatorResult { + if !state.initialized { + // Viewport capture is deferred so the modal can start from a + // toolbar button click where the cursor is over the toolbar. + state.initialized = true; + state.active = true; + state.has_start = false; + return OperatorResult::Running; + } + + if !state.active { + // Confirm triggered finish, clean up and exit the modal. + state.initialized = false; + state.has_start = false; + state.camera = None; + state.viewport = None; + return OperatorResult::Finished; + } + // Capture the viewport the modal was started on; subsequent // frames stick to it even if the cursor strays elsewhere. let camera_entity = state.camera.or_else(|| vp.camera_entity()); let viewport_entity = state.viewport.or_else(|| vp.viewport_entity()); let (Some(camera_entity), Some(viewport_entity)) = (camera_entity, viewport_entity) else { - return OperatorResult::Cancelled; + return OperatorResult::Running; }; if state.camera.is_none() { state.camera = Some(camera_entity); @@ -113,7 +131,9 @@ pub(crate) fn measure_distance( if state.viewport.is_none() { state.viewport = Some(viewport_entity); } - let (camera, cam_tf) = vp.camera_for(camera_entity)?; + let Some((camera, cam_tf)) = vp.camera_for(camera_entity) else { + return OperatorResult::Running; + }; // Try to get a world-space point under the cursor. let current_point = vp.cursor().and_then(|cursor_pos| { @@ -126,26 +146,6 @@ pub(crate) fn measure_distance( ) }); - if !state.initialized { - // First invocation: enter modal mode. Nothing is drawn until the first - // confirm click sets the start point. - let fallback = cam_tf.translation() + cam_tf.forward().as_vec3() * 5.0; - state.initialized = true; - state.active = true; - state.has_start = false; - state.end_point = current_point.unwrap_or(fallback); - return OperatorResult::Running; - } - - if !state.active { - // Confirm triggered finish; clean up and exit modal. - state.initialized = false; - state.has_start = false; - state.camera = None; - state.viewport = None; - return OperatorResult::Finished; - } - // Track cursor while waiting for the first click or while measuring. if let Some(point) = current_point { state.end_point = point; @@ -162,15 +162,20 @@ fn cancel_measure_distance(mut state: ResMut) { state.viewport = None; } -fn measure_tool_active(state: Res) -> bool { - state.active +/// Without the viewport check the toolbar click that activates the +/// modal would also be picked up as the first confirm. +fn confirm_measure_available( + state: Res, + vp: crate::viewport::ViewportCursor, +) -> bool { + state.active && vp.viewport_entity().is_some() } #[operator( id = "tools.measure_distance.confirm", label = "Confirm Measurement", description = "First click sets the start point, second click finishes", - is_available = measure_tool_active, + is_available = confirm_measure_available, allows_undo = false, )] fn confirm_measure_distance( diff --git a/src/viewport.rs b/src/viewport.rs index ded872a..453a52d 100644 --- a/src/viewport.rs +++ b/src/viewport.rs @@ -234,7 +234,7 @@ impl UiCursorPos<'_, '_> { /// Read-only guard resources checked by many interaction systems before acting. /// If any guard is active, the system should bail early. #[derive(SystemParam)] -pub(crate) struct InteractionGuards<'w> { +pub(crate) struct InteractionGuards<'w, 's> { pub gizmo_drag: Res<'w, crate::gizmos::GizmoDragState>, pub gizmo_hover: Res<'w, crate::gizmos::GizmoHoverState>, pub modal: Res<'w, crate::modal_transform::ModalTransformState>, @@ -242,6 +242,18 @@ pub(crate) struct InteractionGuards<'w> { pub draw_state: Res<'w, crate::draw_brush::DrawBrushState>, pub edit_mode: Res<'w, crate::brush::EditMode>, pub terrain_edit_mode: Res<'w, crate::terrain::TerrainEditMode>, + pub active_modal: ActiveModalQuery<'w, 's>, +} + +impl InteractionGuards<'_, '_> { + pub fn is_any_interaction_active(&self) -> bool { + self.gizmo_drag.active + || self.modal.active.is_some() + || self.viewport_drag.active.is_some() + || self.draw_state.active.is_some() + || matches!(*self.edit_mode, crate::brush::EditMode::BrushEdit(_)) + || self.active_modal.is_modal_running() + } } /// Tracks whether a right-click fly session started inside the viewport. diff --git a/src/viewport_select.rs b/src/viewport_select.rs index 0159667..29fe2e3 100644 --- a/src/viewport_select.rs +++ b/src/viewport_select.rs @@ -155,22 +155,11 @@ pub(crate) fn handle_viewport_click( let just_finished_draw = *was_drawing && !drawing_now; *was_drawing = drawing_now; - // Don't select during gizmo drag, modal ops, viewport drag, - // brush edit mode, draw mode, or terrain sculpt mode. Physics - // mode IS allowed: the user needs to click-select entities to - // drag them in the physics tool. - // - // Plain LMB-down still fires here for the immediate click case; - // if the user starts dragging, `box_select_promote_pending` - // dispatches `BoxSelectOp` which then overrides whatever - // selection this handler set on press. + // Physics mode is intentionally not blocked: the user needs to + // click-select entities to drag them in the physics tool. if !mouse.just_pressed(MouseButton::Left) - || guards.gizmo_drag.active + || guards.is_any_interaction_active() || guards.gizmo_hover.hovered_axis.is_some() - || guards.modal.active.is_some() - || guards.viewport_drag.active.is_some() - || matches!(*guards.edit_mode, crate::brush::EditMode::BrushEdit(_)) - || drawing_now || just_finished_draw || matches!( *guards.terrain_edit_mode, @@ -329,20 +318,14 @@ fn box_select_pending_trigger( face_entities: Query<(Entity, &BrushFaceEntity, &GlobalTransform)>, brush_caches: Query<&BrushMeshCache>, ) { + // `gizmo_drag.active` doesn't flip until next frame because the + // gizmo invoke-trigger queues its dispatch — `gizmo_hover` covers + // the same-frame case. if box_state.active || box_state.pending.is_some() || !mouse.just_pressed(MouseButton::Left) - || guards.gizmo_drag.active - // The gizmo invoke-trigger queues the drag operator via - // `commands.queue`, so `gizmo_drag.active` doesn't flip until - // after this Update frame ends. Without checking the hover - // state here the press both arms a pending box-select and - // starts the gizmo drag, so the marquee draws while the user - // is dragging the gizmo. + || guards.is_any_interaction_active() || guards.gizmo_hover.hovered_axis.is_some() - || matches!(*guards.edit_mode, crate::brush::EditMode::BrushEdit(_)) - || guards.draw_state.active.is_some() - || guards.modal.active.is_some() { return; } @@ -401,17 +384,10 @@ fn box_select_promote_pending( box_state.pending = None; return; } - // A modal that started in the same frame as the press (gizmo - // drag, viewport drag, transform shortcut, draw brush) won't have - // shown up in the trigger system's guard check, but it has by - // now. Drop the pending press so we don't dispatch a competing - // box-select on top of the active gesture. - if guards.gizmo_drag.active - || guards.viewport_drag.active.is_some() - || guards.modal.active.is_some() - || guards.draw_state.active.is_some() - || matches!(*guards.edit_mode, crate::brush::EditMode::BrushEdit(_)) - { + // An interaction that started in the same frame as the press + // wouldn't have shown up in the trigger's guard check, but has by + // now. Drop the pending press so we don't fight it. + if guards.is_any_interaction_active() { box_state.pending = None; return; }