diff --git a/.cursor/plans/operator_invariants_completion_2026-07-12.plan.md b/.cursor/plans/operator_invariants_completion_2026-07-12.plan.md new file mode 100644 index 0000000..c131a0d --- /dev/null +++ b/.cursor/plans/operator_invariants_completion_2026-07-12.plan.md @@ -0,0 +1,33 @@ +# Operator Invariants Completion + +Working plan for Gitea +[`#33`](https://git.spacetrainclubhouse.com/Falling-Metal-Interactive/Blacksite/issues/33). + +## Status + +Implementation complete. Source and headless gates pass on the publication branch; the final exact +commit is recorded in the issue closure comment. Packaged-runtime testing remains deferred by the +project owner. + +## Scope + +1. Make registered commands distinguish immediate completion from modal Preview ownership. +2. Require stable operator IDs and terminal phases for commit, cancel, block, failure, no-op, and + automatic interruption paths. +3. Make Group Selection and lighting changes atomic history transactions with repeatable undo/redo. +4. Exercise production dispatch/finalizers for assets, material drops, brushes, terrain, physics + placement, and transform gizmos through reusable semantic projections. +5. Publish the invariant contract, acceptance evidence, and production-gate status without claiming + release-candidate acceptance. + +## Verification + +- `cargo test -p editor --lib --no-fail-fast` +- `cargo clippy -p editor --lib --tests -- -D warnings` +- `cargo fmt --all -- --check` +- `cargo test --workspace --all-targets --no-fail-fast` +- `cargo clippy --workspace --all-targets -- -D warnings` +- `cargo validate-levels --project .` + +Live editor checks cover command-palette modal ownership and the grouped selection/lighting history +surface. Packaged tests are deliberately excluded until re-enabled by the project owner. diff --git a/.cursor/plans/production_readiness_acceptance_2026-07-12.plan.md b/.cursor/plans/production_readiness_acceptance_2026-07-12.plan.md index 6b43d89..3d53ad8 100644 --- a/.cursor/plans/production_readiness_acceptance_2026-07-12.plan.md +++ b/.cursor/plans/production_readiness_acceptance_2026-07-12.plan.md @@ -15,12 +15,12 @@ owner requests it again. 1. Keep one evidence matrix under `docs/editor/evaluations/production-readiness/`; historical H1-H6 notes remain context only. -2. Close implementation blockers before nominating a candidate: the deformed Solari geometry - boundary in `#51`, terrain `#22`-`#24`, physics placement/diagnostics `#25`-`#26`, and their - regression fixtures. Collaborative safety `#49`, Material Library/targeted drops `#16`/`#18`, - and native-dialog responsiveness `#52` are accepted and integrated. -3. Complete the representative regression project, mutation-invariant coverage, performance budgets, - and first-hour workflow tracked by `#32`-`#36`. +2. Close implementation blockers before nominating a candidate. Renderer foundation `#51`, terrain + `#22`-`#24`, physics placement/diagnostics `#25`-`#26`, operator invariants `#33`, collaborative + safety `#49`, Material Library/targeted drops `#16`/`#18`, and native-dialog responsiveness `#52` + are accepted and integrated. +3. Complete the representative regression project, performance budgets, and first-hour workflow + tracked by `#32`, `#34`, `#35`, and `#36`. 4. Nominate one exact commit, validate it from a clean checkout, and record source/headless results. 5. When packaged testing is re-enabled, run the candidate's package/build and packaged-runtime matrix without substituting older artifacts. diff --git a/README.md b/README.md index 2b72765..85a4a5d 100644 --- a/README.md +++ b/README.md @@ -419,7 +419,7 @@ crates/ - [x] CI workflow for format, check, clippy, tests, and binary builds - [x] ADRs for roadmap architecture and Bevy migration policy - [x] Determinism harness: same inputs over same ticks produce the same state summary/hash -- [x] Reusable editor operator harness for commit/cancel/block, helper cleanup, dirty state, grouped undo, and undo/redo round trips +- [x] Production operator invariants across palette dispatch, assets/material drops, brush/terrain/physics modal tools, grouping/lighting, and the transform finalizer: stable terminal status, exact cancel/failure rollback, helper cleanup, grouped history, and repeated undo/redo projections ([testing contract](docs/editor/operator-regression-testing.md), [evaluation](docs/editor/evaluations/operator-invariants/), [Gitea #33](https://git.spacetrainclubhouse.com/Falling-Metal-Interactive/Blacksite/issues/33)) - [x] Verify: `cargo fmt --check` / `cargo check --workspace` / `cargo clippy --workspace` / strict foundation clippy / `cargo test -p sim` - [x] Stable asset registry with UUIDs + import settings in asset browser details - [x] Asset Browser expandable model subasset shelves, independent mesh/material/texture thumbnails, staged import/material details with shader-schema parameters, context actions, and trash-first file removal diff --git a/crates/editor/src/assets/operators.rs b/crates/editor/src/assets/operators.rs index 03e0320..acce012 100644 --- a/crates/editor/src/assets/operators.rs +++ b/crates/editor/src/assets/operators.rs @@ -469,6 +469,86 @@ mod tests { ); } + #[test] + fn multi_actor_material_assignment_is_one_undo_group() { + let mut world = World::new(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let material_path = std::env::temp_dir().join(format!( + "blacksite-material-operator-{}.material.ron", + uuid::Uuid::new_v4() + )); + let fixture = shared::MaterialAsset { + schema_version: shared::MATERIAL_ASSET_SCHEMA_VERSION, + label: "Concrete".into(), + shader: None, + shader_ref: None, + render_state: Default::default(), + material: MaterialDesc { + roughness: 0.85, + ..Default::default() + }, + }; + std::fs::write( + &material_path, + ron::ser::to_string_pretty(&fixture, ron::ser::PrettyConfig::default()).unwrap(), + ) + .unwrap(); + let material_path = material_path.to_string_lossy().into_owned(); + world.insert_resource(AssetRegistry { + records: vec![AssetRecord { + id: AssetId::new(), + path: material_path.clone(), + label: "Concrete".into(), + kind_tag: "Material".into(), + import_settings: Default::default(), + dependencies: Vec::new(), + }], + index_dirty: false, + }); + let first = world.spawn((LevelObject, MaterialDesc::default())).id(); + let second = world.spawn((LevelObject, MaterialDesc::default())).id(); + let mut selected = SelectedEntities::default(); + selected.select_replace(first); + selected.select_maybe_add(second, true); + let asset = EditorAsset { + label: "Concrete".to_string(), + path: Some(material_path.clone()), + folder_path: "assets/materials".to_string(), + kind: super::super::EditorAssetKind::Material, + }; + super::super::materials::material_desc_from_asset(&material_path) + .expect("committed material fixture should resolve"); + let harness = OperatorInvariantHarness::capture(&mut world); + let read_materials = move |world: &mut World| { + [first, second] + .into_iter() + .map(|entity| { + world + .get::(entity) + .and_then(|material| material.material_asset_path.clone()) + }) + .collect::>() + }; + + let applied = apply_material_operator(&mut world, asset, &selected); + assert!( + applied, + "material operator failed: {:?}", + world.resource::().status + ); + + harness.assert_committed(&mut world, 1, 0); + assert_undo_redo_round_trip( + &mut world, + vec![None, None], + vec![Some(material_path.clone()), Some(material_path.clone())], + read_materials, + ); + let _ = std::fs::remove_file(material_path); + } + #[test] fn missing_subasset_commit_cancels_without_side_effects() { let mut world = World::new(); @@ -533,4 +613,59 @@ mod tests { harness.assert_blocked(&mut world); } + + #[test] + fn material_assignment_without_authored_selection_is_blocked() { + let mut world = World::new(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let harness = OperatorInvariantHarness::capture(&mut world); + let asset = EditorAsset { + label: "Concrete".to_string(), + path: Some("assets/materials/concrete.ron".to_string()), + folder_path: "assets/materials".to_string(), + kind: super::super::EditorAssetKind::Material, + }; + + assert!(!apply_material_operator( + &mut world, + asset, + &SelectedEntities::default() + )); + + harness.assert_blocked(&mut world); + } + + #[test] + fn audio_and_animation_assignment_reject_incompatible_actors() { + let mut world = World::new(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let entity = world.spawn(LevelObject).id(); + let audio = EditorAsset { + label: "Impact".into(), + path: Some("assets/audio/impact.ogg".into()), + folder_path: "assets/audio".into(), + kind: super::super::EditorAssetKind::AudioClip, + }; + let audio_harness = OperatorInvariantHarness::capture(&mut world); + + assert!(!assign_audio_clip_operator(&mut world, audio, entity)); + audio_harness.assert_blocked(&mut world); + + let animation_harness = OperatorInvariantHarness::capture(&mut world); + let selection = AssetSelection::SubAsset { + parent_path: "assets/models/missing.glb".into(), + sub_asset_id: "animation:missing".into(), + label: "Missing".into(), + kind: super::super::AssetSubAssetKind::AnimationClip, + source_path: None, + }; + assert!(!assign_animation_clip_operator( + &mut world, selection, entity + )); + animation_harness.assert_blocked(&mut world); + } } diff --git a/crates/editor/src/ext/extensibility.rs b/crates/editor/src/ext/extensibility.rs index a2e6d98..dac3996 100644 --- a/crates/editor/src/ext/extensibility.rs +++ b/crates/editor/src/ext/extensibility.rs @@ -4,7 +4,7 @@ use bevy::prelude::*; use bevy_egui::{egui, EguiContexts, EguiPrimaryContextPass}; use crate::history::group_selection_with_history; -use crate::operators::{run_immediate_operator, OperatorAvailability}; +use crate::operators::{run_operator_action, OperatorAction, OperatorAvailability}; use crate::state::{EditorMode, PlayPossession}; use crate::ui::helpers::{ reset_scene_lighting_to_project_defaults, toggle_play_mode, toggle_play_paused, @@ -27,7 +27,7 @@ pub trait EditorCommand: Send + Sync { fn disabled_reason(&self, _world: &World) -> Option { None } - fn execute(&self, world: &mut World); + fn execute(&self, world: &mut World) -> Result; } #[derive(Resource, Default)] @@ -70,13 +70,11 @@ impl EditorCommandRegistry { .map(|cmd| (cmd.label().to_string(), cmd.disabled_reason(world))) } - pub fn run(&self, world: &mut World, name: &str) -> bool { - if let Some(command) = self.commands.iter().find(|cmd| cmd.name() == name) { - command.execute(world); - true - } else { - false - } + pub fn run(&self, world: &mut World, name: &str) -> Option> { + self.commands + .iter() + .find(|command| command.name() == name) + .map(|command| command.execute(world)) } } @@ -191,6 +189,10 @@ pub fn register_editor_plugin(app: &mut App, plugin: Box) { } fn register_builtin_commands(mut registry: ResMut) { + populate_builtin_commands(&mut registry); +} + +fn populate_builtin_commands(registry: &mut EditorCommandRegistry) { registry.register(Box::new(TogglePlayCommand)); registry.register(Box::new(TogglePlayPausedCommand)); registry.register(Box::new(TogglePossessionCommand)); @@ -235,7 +237,7 @@ fn dispatch_editor_command(world: &mut World, name: &str) { let name_for_commit = name.to_string(); let name_for_status = name.to_string(); let label_for_commit = label.clone(); - run_immediate_operator( + run_operator_action( world, &name_for_status, &label, @@ -244,14 +246,10 @@ fn dispatch_editor_command(world: &mut World, name: &str) { None => OperatorAvailability::Ready, }, move |world| { - let ran = world.resource_scope(|world, registry: Mut| { + let result = world.resource_scope(|world, registry: Mut| { registry.run(world, &name_for_commit) }); - if ran { - Ok(()) - } else { - Err(format!("Unknown editor command: {name_for_commit}")) - } + result.unwrap_or_else(|| Err(format!("Unknown editor command: {name_for_commit}"))) }, ); @@ -269,8 +267,9 @@ impl EditorCommand for TogglePlayCommand { "Toggle Play / Edit" } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { toggle_play_mode(world); + Ok(OperatorAction::Commit) } } @@ -290,8 +289,9 @@ impl EditorCommand for TogglePlayPausedCommand { .then(|| "Enter Play mode before pausing the simulation".to_string()) } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { toggle_play_paused(world); + Ok(OperatorAction::Commit) } } @@ -311,12 +311,13 @@ impl EditorCommand for TogglePossessionCommand { .then(|| "Enter Play mode before toggling possession".to_string()) } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { let next = match *world.resource::() { PlayPossession::Possessed => PlayPossession::Ejected, PlayPossession::Ejected => PlayPossession::Possessed, }; *world.resource_mut::() = next; + Ok(OperatorAction::Commit) } } @@ -331,8 +332,9 @@ impl EditorCommand for ResetLightingCommand { "Reset Scene Lighting" } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { reset_scene_lighting_to_project_defaults(world); + Ok(OperatorAction::Commit) } } @@ -347,13 +349,23 @@ impl EditorCommand for GroupSelectionCommand { "Group Selection" } - fn execute(&self, world: &mut World) { + fn disabled_reason(&self, world: &World) -> Option { + world + .resource::() + .selected_entities + .as_slice() + .is_empty() + .then(|| "Select at least one actor to group".to_string()) + } + + fn execute(&self, world: &mut World) -> Result { let selected: Vec = world .resource::() .selected_entities .iter() .collect(); group_selection_with_history(world, &selected); + Ok(OperatorAction::Commit) } } @@ -368,8 +380,9 @@ impl EditorCommand for FocusSelectionCommand { "Focus Selection" } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { focus_editor_camera_on_selection(world); + Ok(OperatorAction::Commit) } } @@ -384,8 +397,9 @@ impl EditorCommand for ResetSelectionTransformCommand { "Reset Selection Transform" } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { reset_selected_transforms(world); + Ok(OperatorAction::Commit) } } @@ -405,8 +419,9 @@ impl EditorCommand for DrawBrushCommand { .then(|| "Enter Edit mode before drawing brushes".to_string()) } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { start_draw_brush_tool(world); + Ok(OperatorAction::ContinuePreview) } } @@ -425,8 +440,9 @@ impl EditorCommand for IntersectBrushesCommand { (selected_brush_count(world) < 2).then(|| "Select at least two brushes".to_string()) } - fn execute(&self, world: &mut World) { - intersect_selected_brushes(world); + fn execute(&self, world: &mut World) -> Result { + intersect_selected_brushes(world)?; + Ok(OperatorAction::ContinuePreview) } } @@ -445,8 +461,9 @@ impl EditorCommand for MergeBrushesCommand { (selected_brush_count(world) < 2).then(|| "Select at least two brushes".to_string()) } - fn execute(&self, world: &mut World) { - merge_selected_brushes(world); + fn execute(&self, world: &mut World) -> Result { + merge_selected_brushes(world)?; + Ok(OperatorAction::ContinuePreview) } } @@ -465,8 +482,9 @@ impl EditorCommand for SubtractBrushesCommand { (selected_brush_count(world) < 2).then(|| "Select at least two brushes".to_string()) } - fn execute(&self, world: &mut World) { - subtract_selected_brushes(world); + fn execute(&self, world: &mut World) -> Result { + subtract_selected_brushes(world)?; + Ok(OperatorAction::ContinuePreview) } } @@ -481,8 +499,9 @@ impl EditorCommand for CreatePostProcessVolumeCommand { "Create Post-process Volume" } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { crate::rendering_diagnostics::spawn_post_process_volume_at_camera(world); + Ok(OperatorAction::Commit) } } @@ -497,9 +516,10 @@ impl EditorCommand for FocusActiveVolumesCommand { "Focus Active Post-process Volumes" } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { crate::rendering_diagnostics::select_volumes_at_camera(world); focus_editor_camera_on_selection(world); + Ok(OperatorAction::Commit) } } @@ -514,8 +534,9 @@ impl EditorCommand for SelectVolumesAtCameraCommand { "Select Volumes at Camera" } - fn execute(&self, world: &mut World) { + fn execute(&self, world: &mut World) -> Result { crate::rendering_diagnostics::select_volumes_at_camera(world); + Ok(OperatorAction::Commit) } } @@ -671,6 +692,53 @@ fn command_palette_row(entry: &EditorCommandEntry) -> egui::WidgetText { #[cfg(test)] mod tests { use super::*; + use crate::history::{apply_command_redo, apply_command_undo, EditorHistory}; + use crate::operators::test_harness::OperatorInvariantHarness; + use crate::operators::{ActiveOperator, OperatorPhase}; + use crate::scene_io::SceneIo; + use crate::selection::{SelectedEntity, ViewportClick}; + use crate::viewport::brush_tool::BrushToolState; + use shared::{ + ActorKind, AuthoringLightKind, BrushDesc, EditorVisibility, HierarchySiblingIndex, + LevelObject, LightDesc, + }; + + fn command_world() -> World { + let mut world = World::new(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.insert_resource(State::new(EditorMode::Editing)); + world.insert_resource(UiState::default_layout()); + let mut registry = EditorCommandRegistry::default(); + populate_builtin_commands(&mut registry); + world.insert_resource(registry); + world + } + + fn hierarchy_projection(world: &mut World) -> Vec<(String, Option, i32)> { + let mut query = world.query_filtered::<(Entity, &Name), With>(); + let mut result = query + .iter(world) + .map(|(entity, name)| { + let parent = world + .get::(entity) + .and_then(|child| world.get::(child.parent())) + .map(|name| name.as_str().to_string()); + let sibling_index = world + .get::(entity) + .map(|index| index.0) + .unwrap_or_default(); + (name.as_str().to_string(), parent, sibling_index) + }) + .collect::>(); + result.sort(); + result + } #[test] fn command_filter_matches_human_label_and_stable_id() { @@ -697,4 +765,208 @@ mod tests { assert_eq!(entries[0].name, "play.toggle"); assert_eq!(entries[1].name, "scene.reset_lighting"); } + + #[test] + fn grouping_dispatch_is_one_history_transaction_and_round_trips() { + let mut world = command_world(); + let first = world + .spawn(( + Name::new("First"), + LevelObject, + ActorKind::Empty, + Transform::from_xyz(1.0, 0.0, 0.0), + GlobalTransform::default(), + HierarchySiblingIndex(0), + EditorVisibility::default(), + )) + .id(); + let second = world + .spawn(( + Name::new("Second"), + LevelObject, + ActorKind::Empty, + Transform::from_xyz(2.0, 0.0, 0.0), + GlobalTransform::default(), + HierarchySiblingIndex(1), + EditorVisibility::default(), + )) + .id(); + let unselected = world + .spawn(( + Name::new("Unselected"), + LevelObject, + ActorKind::Empty, + Transform::from_xyz(3.0, 0.0, 0.0), + GlobalTransform::default(), + HierarchySiblingIndex(2), + EditorVisibility::default(), + )) + .id(); + world + .resource_mut::() + .selected_entities + .select_replace(first); + world + .resource_mut::() + .selected_entities + .select_maybe_add(second, true); + let initial = hierarchy_projection(&mut world); + let harness = OperatorInvariantHarness::capture(&mut world); + + dispatch_editor_command(&mut world, "selection.group"); + + harness.assert_committed(&mut world, 1, 1); + harness.assert_status(&world, "selection.group", OperatorPhase::Committed); + let committed = hierarchy_projection(&mut world); + assert!(committed + .iter() + .any(|(name, parent, _)| name == "First" && parent.as_deref() == Some("Group"))); + assert!(committed + .iter() + .any(|(name, parent, _)| name == "Second" && parent.as_deref() == Some("Group"))); + apply_command_undo(&mut world); + assert_eq!(hierarchy_projection(&mut world), initial); + apply_command_redo(&mut world); + assert_eq!(hierarchy_projection(&mut world), committed); + assert!(world.get::(unselected).is_none()); + apply_command_undo(&mut world); + assert_eq!(hierarchy_projection(&mut world), initial); + apply_command_redo(&mut world); + assert_eq!(hierarchy_projection(&mut world), committed); + assert!(world.get::(unselected).is_none()); + } + + #[test] + fn reset_lighting_dispatch_is_one_undoable_group() { + let mut world = command_world(); + let point = world + .spawn(( + LevelObject, + ActorKind::Light, + LightDesc { + kind: AuthoringLightKind::Point, + ..Default::default() + }, + )) + .id(); + let directional = world + .spawn(( + LevelObject, + ActorKind::Light, + LightDesc { + kind: AuthoringLightKind::Directional, + ..Default::default() + }, + )) + .id(); + let harness = OperatorInvariantHarness::capture(&mut world); + + dispatch_editor_command(&mut world, "scene.reset_lighting"); + + harness.assert_committed(&mut world, 1, 0); + harness.assert_status(&world, "scene.reset_lighting", OperatorPhase::Committed); + assert!(world.get::(point).is_none()); + assert!(world.get::(directional).is_none()); + apply_command_undo(&mut world); + assert!(world.get::(point).is_some()); + assert!(world.get::(directional).is_some()); + apply_command_redo(&mut world); + assert!(world.get::(point).is_none()); + assert!(world.get::(directional).is_none()); + } + + #[test] + fn modal_command_dispatch_retains_preview_ownership() { + let mut world = command_world(); + let harness = OperatorInvariantHarness::capture(&mut world); + + dispatch_editor_command(&mut world, "brush.draw"); + + harness.assert_status(&world, "brush.draw", OperatorPhase::Preview); + harness.assert_projection_unchanged(&mut world, 0, |world| { + world.resource::().undo_depth() + }); + assert!(!world.resource::().dirty); + assert!(world.resource::().active); + } + + #[test] + fn failed_csg_dispatch_terminates_without_preview_or_history() { + let mut world = command_world(); + let first = world + .spawn(( + LevelObject, + BrushDesc::cuboid(Vec3::splat(1.0)), + Transform::IDENTITY, + )) + .id(); + let mut invalid = BrushDesc::cuboid(Vec3::splat(1.0)); + invalid.faces.clear(); + let second = world + .spawn((LevelObject, invalid, Transform::IDENTITY)) + .id(); + world + .resource_mut::() + .selected_entities + .select_replace(first); + world + .resource_mut::() + .selected_entities + .select_maybe_add(second, true); + let harness = OperatorInvariantHarness::capture(&mut world); + + dispatch_editor_command(&mut world, "brush.intersect"); + + harness.assert_canceled(&mut world); + harness.assert_status(&world, "brush.intersect", OperatorPhase::Canceled); + assert!(!world.resource::().dirty); + } + + #[test] + fn csg_dispatch_blocks_read_only_selection_without_history() { + let mut world = command_world(); + let first = world + .spawn(( + LevelObject, + BrushDesc::cuboid(Vec3::splat(1.0)), + Transform::IDENTITY, + )) + .id(); + let second = world + .spawn(( + LevelObject, + BrushDesc::cuboid(Vec3::splat(1.0)), + Transform::from_xyz(0.5, 0.0, 0.0), + )) + .id(); + let mut hierarchy = crate::ui::hierarchy_state::HierarchyPanelState::default(); + hierarchy.locked.insert(second); + world.insert_resource(hierarchy); + world + .resource_mut::() + .selected_entities + .select_replace(first); + world + .resource_mut::() + .selected_entities + .select_maybe_add(second, true); + let harness = OperatorInvariantHarness::capture(&mut world); + + dispatch_editor_command(&mut world, "brush.merge_convex"); + + harness.assert_blocked(&mut world); + harness.assert_status(&world, "brush.merge_convex", OperatorPhase::Blocked); + assert!(!world.resource::().dirty); + } + + #[test] + fn disabled_command_dispatch_is_a_blocked_no_op() { + let mut world = command_world(); + let harness = OperatorInvariantHarness::capture(&mut world); + + dispatch_editor_command(&mut world, "selection.group"); + + harness.assert_blocked(&mut world); + harness.assert_status(&world, "selection.group", OperatorPhase::Blocked); + } } diff --git a/crates/editor/src/history/commands.rs b/crates/editor/src/history/commands.rs index 464ba21..549bebb 100644 --- a/crates/editor/src/history/commands.rs +++ b/crates/editor/src/history/commands.rs @@ -106,6 +106,11 @@ pub struct PrefabEditState { #[derive(Debug, Clone)] pub enum EditorCommand { + GroupSelection { + snapshot: EditorEntitySnapshot, + group: Option, + changes: Vec, + }, Spawn { snapshot: EditorEntitySnapshot, entity: Option, @@ -152,6 +157,12 @@ pub enum EditorCommand { old: Option, new: LightDesc, }, + SetLightGroup { + entities: Vec, + olds: Vec>, + news: Vec>, + label: &'static str, + }, SetAnimationController { entity: Entity, old: Option, @@ -293,6 +304,7 @@ pub enum EditorCommand { impl EditorCommand { pub fn label(&self) -> &'static str { match self { + EditorCommand::GroupSelection { .. } => "Group Selection", EditorCommand::Spawn { .. } => "Spawn", EditorCommand::SpawnMany { .. } => "Spawn Brushes", EditorCommand::Despawn { .. } => "Delete", @@ -303,6 +315,7 @@ impl EditorCommand { EditorCommand::SetMaterialGroup { .. } => "Apply Material to Selection", EditorCommand::SetMaterialOverride { .. } => "Set Material Override", EditorCommand::SetLight { .. } => "Set Light", + EditorCommand::SetLightGroup { label, .. } => label, EditorCommand::SetAnimationController { .. } => "Set Animation Controller", EditorCommand::SetAudioSource { .. } => "Set Audio Source", EditorCommand::SetAudioListener { .. } => "Set Audio Listener", diff --git a/crates/editor/src/history/mod.rs b/crates/editor/src/history/mod.rs index 9d96ba3..8d5e836 100644 --- a/crates/editor/src/history/mod.rs +++ b/crates/editor/src/history/mod.rs @@ -17,6 +17,7 @@ use shared::{ TriggerVolume, WeaponSpawn, }; +use crate::operators::{ActiveOperator, OperatorPhase, OperatorStatus}; use crate::scene_io::{SceneIo, SceneIoRequest}; use crate::selection::SelectedEntity; use crate::state::scene_tools_active; @@ -560,6 +561,40 @@ pub fn set_light_with_history(world: &mut World, entity: Entity, new: LightDesc) push_history(world, EditorCommand::SetLight { entity, old, new }); } +pub fn set_light_group_with_history( + world: &mut World, + changes: impl IntoIterator)>, + label: &'static str, +) { + let mut entities = Vec::new(); + let mut olds = Vec::new(); + let mut news = Vec::new(); + for (entity, new) in changes { + if !is_mutable_level_object(world, entity) { + continue; + } + let old = world.get::(entity).cloned(); + if old.is_none() && new.is_none() { + continue; + } + apply_light(world, entity, &new); + entities.push(entity); + olds.push(old); + news.push(new); + } + if !entities.is_empty() { + push_history( + world, + EditorCommand::SetLightGroup { + entities, + olds, + news, + label, + }, + ); + } +} + pub fn set_audio_source_with_history(world: &mut World, entity: Entity, new: AudioSourceDesc) { if !is_mutable_level_object(world, entity) { return; @@ -752,9 +787,13 @@ pub fn apply_brush_csg_with_history( new_transform: Transform, new_brush: BrushDesc, delete_entities: &[Entity], -) { - if !is_level_object(world, primary) { - return; +) -> bool { + if !is_mutable_level_object(world, primary) + || delete_entities + .iter() + .any(|entity| !is_mutable_level_object(world, *entity)) + { + return false; } let old_transform = world.get::(primary).copied().unwrap_or_default(); let old_brush = world.get::(primary).cloned(); @@ -769,7 +808,7 @@ pub fn apply_brush_csg_with_history( && old_brush.as_ref() == Some(&new_brush) && deleted.is_empty() { - return; + return false; } if let Ok(mut entity_mut) = world.get_entity_mut(primary) { entity_mut.insert(new_transform); @@ -790,6 +829,7 @@ pub fn apply_brush_csg_with_history( }, ); select_one(world, primary); + true } pub fn set_static_mesh_renderer_with_history( @@ -943,14 +983,16 @@ pub fn group_selection_with_history(world: &mut World, entities: &[Entity]) { children: Vec::new(), }; let group = spawn_snapshot(world, &snapshot); + let changes = reorder_entities_under_parent(world, &entities, Some(group), i32::MAX); push_history( world, - EditorCommand::Spawn { + EditorCommand::GroupSelection { snapshot, - entity: Some(group), + group: Some(group), + changes, }, ); - reorder_siblings_with_history(world, &entities, Some(group), i32::MAX); + select_one(world, group); } pub fn create_empty_child_with_history(world: &mut World, parent: Entity) { @@ -1637,6 +1679,13 @@ fn prepare_prefab_source_history( fn undo_command(world: &mut World, command: &mut EditorCommand) { match command { + EditorCommand::GroupSelection { group, changes, .. } => { + for change in changes.iter().rev() { + apply_sibling_change(world, change); + } + despawn_entity(world, *group); + clear_selection(world); + } EditorCommand::Spawn { entity, .. } => { despawn_entity(world, entity.take()); clear_selection(world); @@ -1682,6 +1731,11 @@ fn undo_command(world: &mut World, command: &mut EditorCommand) { EditorCommand::SetLight { entity, old, .. } => { apply_light(world, *entity, old); } + EditorCommand::SetLightGroup { entities, olds, .. } => { + for (entity, old) in entities.iter().zip(olds.iter()) { + apply_light(world, *entity, old); + } + } EditorCommand::SetAnimationController { entity, old, .. } => { apply_animation_controller(world, *entity, old); } @@ -1818,6 +1872,25 @@ fn undo_command(world: &mut World, command: &mut EditorCommand) { fn redo_command(world: &mut World, command: &mut EditorCommand) { match command { + EditorCommand::GroupSelection { + snapshot, + group, + changes, + } => { + let previous_group = *group; + let spawned = spawn_snapshot(world, snapshot); + *group = Some(spawned); + for change in changes.iter_mut() { + if Some(change.entity) == previous_group { + change.entity = spawned; + } + if change.new_parent == previous_group { + change.new_parent = Some(spawned); + } + apply_sibling_change_new(world, change); + } + select_one(world, spawned); + } EditorCommand::Spawn { snapshot, entity } => { let spawned = spawn_snapshot(world, snapshot); *entity = Some(spawned); @@ -1881,6 +1954,11 @@ fn redo_command(world: &mut World, command: &mut EditorCommand) { entity_mut.insert(new.clone()); } } + EditorCommand::SetLightGroup { entities, news, .. } => { + for (entity, new) in entities.iter().zip(news.iter()) { + apply_light(world, *entity, new); + } + } EditorCommand::SetAnimationController { entity, new, .. } => { if let Ok(mut entity_mut) = world.get_entity_mut(*entity) { entity_mut.insert(new.clone()); @@ -2614,25 +2692,41 @@ fn capture_gizmo_transform_edits(world: &mut World) { let start_group = active.map(|(entity, transform)| selected_transforms(world, &selection, entity, transform)); + let mut started_edit = false; let (finished_edit, live_edit) = { let mut tracker = world.resource_mut::(); if tracker.active.is_none() { if let (Some((entity, transform)), Some(group)) = (active, start_group) { tracker.active = Some((entity, transform, group)); - return; + started_edit = true; } } - match tracker.active.take() { - Some((primary, old, group)) if active.is_none() => (Some((primary, old, group)), None), - Some(state) if active.is_some() => { - let live = Some(state.clone()); - tracker.active = Some(state); - (None, live) + if started_edit { + (None, None) + } else { + match tracker.active.take() { + Some((primary, old, group)) if active.is_none() => { + (Some((primary, old, group)), None) + } + Some(state) if active.is_some() => { + let live = Some(state.clone()); + tracker.active = Some(state); + (None, live) + } + _ => (None, None), } - _ => (None, None), } }; + if started_edit { + set_transform_operator_status( + world, + OperatorPhase::Preview, + "Transform gizmo drag in progress", + ); + return; + } + if let Some((primary, old_primary, group)) = live_edit { let Some(new_primary) = world.get::(primary).copied() else { return; @@ -2645,14 +2739,50 @@ fn capture_gizmo_transform_edits(world: &mut World) { if let Some((primary, old_primary, group)) = finished_edit { let Some(new_primary) = world.get::(primary).copied() else { + for (entity, old) in group { + if let Some(mut transform) = world.get_mut::(entity) { + *transform = old; + } + } + set_transform_operator_status( + world, + OperatorPhase::Canceled, + "Transform target disappeared; surviving previews restored", + ); return; }; + let undo_depth = world.resource::().undo_depth(); if group.len() <= 1 { set_transform_with_history(world, primary, old_primary, new_primary); - return; + } else { + let changes = transform_group_changes(old_primary, new_primary, group); + set_transform_group_with_history(world, changes); } - let changes = transform_group_changes(old_primary, new_primary, group); - set_transform_group_with_history(world, changes); + if world.resource::().undo_depth() > undo_depth { + set_transform_operator_status( + world, + OperatorPhase::Committed, + "Transform gizmo edit committed", + ); + } else { + set_transform_operator_status( + world, + OperatorPhase::Canceled, + "Transform gizmo edit made no changes", + ); + } + } +} + +fn set_transform_operator_status(world: &mut World, phase: OperatorPhase, hint: impl Into) { + if let Some(mut active) = world.get_resource_mut::() { + active.status = Some(OperatorStatus { + id: "transform.gizmo".to_string(), + label: "Transform Gizmo".to_string(), + phase, + hint: hint.into(), + warnings: Vec::new(), + }); } } @@ -2796,7 +2926,7 @@ pub(crate) fn material_eq(a: &MaterialDesc, b: &MaterialDesc) -> bool { mod tests { use super::*; use crate::operators::test_harness::{assert_undo_redo_round_trip, OperatorInvariantHarness}; - use crate::operators::{ActiveOperator, OperatorPhase, OperatorStatus}; + use crate::operators::{ActiveOperator, OperatorPhase}; #[derive(Component, Reflect, Default, Debug, Clone, PartialEq)] #[reflect(Component, Default)] @@ -3074,6 +3204,7 @@ mod tests { world.init_resource::(); world.init_resource::(); world.init_resource::(); + world.init_resource::(); let first = world.spawn((LevelObject, Transform::default())).id(); let second = world .spawn(( @@ -3085,30 +3216,19 @@ mod tests { let committed = vec![Vec3::Z, Vec3::new(2.0, 0.0, 1.0)]; let harness = OperatorInvariantHarness::capture(&mut world); - set_transform_group_with_history( - &mut world, - [ - ( - first, - Transform::default(), - Transform::from_translation(committed[0]), - ), - ( - second, - Transform::from_translation(initial[1]), - Transform::from_translation(committed[1]), - ), + world.resource_mut::().active = Some(( + first, + Transform::default(), + vec![ + (first, Transform::default()), + (second, Transform::from_translation(initial[1])), ], - ); - world.resource_mut::().status = Some(OperatorStatus { - id: "transform.gizmo".to_string(), - label: "Transform Gizmo".to_string(), - phase: OperatorPhase::Committed, - hint: "Committed multi-selection drag".to_string(), - warnings: Vec::new(), - }); + )); + *world.get_mut::(first).unwrap() = Transform::from_translation(committed[0]); + capture_gizmo_transform_edits(&mut world); harness.assert_committed(&mut world, 1, 0); + harness.assert_status(&world, "transform.gizmo", OperatorPhase::Committed); assert_undo_redo_round_trip(&mut world, initial, committed, |world| { [first, second] .into_iter() @@ -3117,6 +3237,63 @@ mod tests { }); } + #[test] + fn no_op_transform_release_terminates_preview_without_history() { + let mut world = World::new(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let transform = Transform::from_xyz(1.0, 2.0, 3.0); + let entity = world.spawn((LevelObject, transform)).id(); + world.resource_mut::().active = + Some((entity, transform, vec![(entity, transform)])); + set_transform_operator_status( + &mut world, + OperatorPhase::Preview, + "Transform gizmo drag in progress", + ); + let harness = OperatorInvariantHarness::capture(&mut world); + + capture_gizmo_transform_edits(&mut world); + + harness.assert_canceled(&mut world); + harness.assert_status(&world, "transform.gizmo", OperatorPhase::Canceled); + assert_eq!(world.get::(entity), Some(&transform)); + } + + #[test] + fn missing_primary_transform_rolls_back_surviving_preview_targets() { + let mut world = World::new(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let primary_old = Transform::from_xyz(1.0, 0.0, 0.0); + let survivor_old = Transform::from_xyz(2.0, 0.0, 0.0); + let primary = world.spawn((LevelObject, primary_old)).id(); + let survivor = world.spawn((LevelObject, survivor_old)).id(); + world.resource_mut::().active = Some(( + primary, + primary_old, + vec![(primary, primary_old), (survivor, survivor_old)], + )); + *world.get_mut::(survivor).unwrap() = Transform::from_xyz(8.0, 0.0, 0.0); + world.despawn(primary); + set_transform_operator_status( + &mut world, + OperatorPhase::Preview, + "Transform gizmo drag in progress", + ); + let harness = OperatorInvariantHarness::capture(&mut world); + + capture_gizmo_transform_edits(&mut world); + + harness.assert_canceled(&mut world); + harness.assert_status(&world, "transform.gizmo", OperatorPhase::Canceled); + assert_eq!(world.get::(survivor), Some(&survivor_old)); + } + #[test] fn unpack_prefab_is_one_undoable_history_command() { let mut world = World::new(); diff --git a/crates/editor/src/operators.rs b/crates/editor/src/operators.rs index c5ed841..de6d435 100644 --- a/crates/editor/src/operators.rs +++ b/crates/editor/src/operators.rs @@ -59,6 +59,12 @@ pub enum OperatorAvailability { Disabled(String), } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum OperatorAction { + Commit, + ContinuePreview, +} + pub trait EditorOperator { fn id(&self) -> &str; fn label(&self) -> &str; @@ -221,6 +227,51 @@ pub fn run_immediate_operator( run_operator(world, &mut operator) } +/// Runs an editor command that may either complete immediately or hand ownership to a modal tool. +/// Modal starters retain the preview status installed by their production entry point. +pub fn run_operator_action( + world: &mut World, + id: &str, + label: &str, + availability: OperatorAvailability, + action: impl FnOnce(&mut World) -> Result, +) -> bool { + if let OperatorAvailability::Disabled(reason) = availability { + set_operator_status( + world, + OperatorStatus::new(id, label, OperatorPhase::Blocked, reason), + ); + return false; + } + + set_operator_status( + world, + OperatorStatus::new(id, label, OperatorPhase::Preview, "Starting"), + ); + match action(world) { + Ok(OperatorAction::Commit) => { + set_operator_status( + world, + OperatorStatus::new( + id, + label, + OperatorPhase::Committed, + format!("Completed {label}"), + ), + ); + true + } + Ok(OperatorAction::ContinuePreview) => true, + Err(error) => { + set_operator_status( + world, + OperatorStatus::new(id, label, OperatorPhase::Canceled, error), + ); + false + } + } +} + fn set_operator_status(world: &mut World, status: OperatorStatus) { if let Some(mut active) = world.get_resource_mut::() { active.status = Some(status); @@ -355,6 +406,52 @@ mod tests { harness.assert_no_helpers::(&mut world); } + #[test] + fn preview_failure_runs_cancel_and_cleans_helpers() { + struct PreviewFailure(CounterOperator); + + impl EditorOperator for PreviewFailure { + fn id(&self) -> &str { + self.0.id() + } + + fn label(&self) -> &str { + self.0.label() + } + + fn begin(&mut self, world: &mut World) -> Result<(), String> { + self.0.begin(world) + } + + fn preview(&mut self, world: &mut World) -> Result<(), String> { + self.0.preview(world)?; + Err("preview failed".to_string()) + } + + fn commit(&mut self, _world: &mut World) -> Result<(), String> { + panic!("commit must not run after preview failure") + } + + fn cancel(&mut self, world: &mut World) { + self.0.cancel(world); + } + } + + let mut world = test_world(); + let harness = OperatorInvariantHarness::capture(&mut world); + let mut operator = PreviewFailure(CounterOperator { + before: 0, + helper: None, + }); + + assert!(!run_operator(&mut world, &mut operator)); + + harness.assert_canceled(&mut world); + harness.assert_status(&world, "test.counter", OperatorPhase::Canceled); + harness.assert_no_helpers::(&mut world); + assert_eq!(world.resource::().0, 0); + } + #[test] fn blocked_operator_never_begins_or_mutates_world() { let mut world = test_world(); diff --git a/crates/editor/src/operators/test_harness.rs b/crates/editor/src/operators/test_harness.rs index ec63567..755dd7d 100644 --- a/crates/editor/src/operators/test_harness.rs +++ b/crates/editor/src/operators/test_harness.rs @@ -47,6 +47,12 @@ impl OperatorInvariantHarness { world.resource::().dirty, "a committed authored mutation must mark the scene dirty" ); + } else { + assert_eq!( + world.resource::().dirty, + self.dirty, + "a committed non-authoring operator must preserve the prior dirty state" + ); } assert_eq!( authored_count(world) as isize, @@ -93,6 +99,39 @@ impl OperatorInvariantHarness { ); } + pub(crate) fn assert_status( + &self, + world: &World, + expected_id: &str, + expected_phase: OperatorPhase, + ) { + let status = world + .resource::() + .status + .as_ref() + .expect("operator must retain an inspectable lifecycle status"); + assert_eq!(status.id, expected_id, "operator reported the wrong id"); + assert_eq!( + status.phase, expected_phase, + "operator reported the wrong phase" + ); + } + + pub(crate) fn assert_projection_unchanged( + &self, + world: &mut World, + expected: T, + mut read: impl FnMut(&mut World) -> T, + ) where + T: Debug + PartialEq, + { + assert_eq!( + read(world), + expected, + "operator changed authored state without a committed history transaction" + ); + } + fn assert_phase(&self, world: &World, expected: OperatorPhase) { let phase = world .resource::() diff --git a/crates/editor/src/ui/helpers.rs b/crates/editor/src/ui/helpers.rs index a1fa265..5033305 100644 --- a/crates/editor/src/ui/helpers.rs +++ b/crates/editor/src/ui/helpers.rs @@ -11,7 +11,10 @@ use sim::Player; use crate::assets::{snapshot_for_asset, EditorAsset, EditorAssetKind}; use crate::camera::EditorCamera; use crate::gizmos::{EditorGizmoMode, EditorGizmoSpace}; -use crate::history::{set_material_group_with_history, spawn_with_history, EditorEntitySnapshot}; +use crate::history::{ + set_light_group_with_history, set_material_group_with_history, spawn_with_history, + EditorEntitySnapshot, +}; use crate::play::default_player_spawn; use crate::scene_io::SceneIo; use crate::viewport::{recall_camera_bookmark, save_camera_bookmark, CameraBookmarks}; @@ -191,37 +194,23 @@ pub fn ensure_player_spawn_for_edit(world: &mut World) -> Entity { /// Removes all authored [`LightDesc`] so project sun/ambient drive outdoor lighting. pub fn reset_scene_lighting_to_project_defaults(world: &mut World) { - let mut query = world.query_filtered::>(); - let with_lights: Vec = query + let mut query = world.query_filtered::<(Entity, &LightDesc), With>(); + let changes = query .iter(world) - .filter(|entity| world.get::(*entity).is_some()) - .collect(); - for entity in with_lights { - if let Ok(mut entity_mut) = world.get_entity_mut(entity) { - entity_mut.remove::(); - } - } - world - .resource_mut::() - .mark_dirty(); + .map(|(entity, _)| (entity, None)) + .collect::>(); + set_light_group_with_history(world, changes, "Reset Scene Lighting"); } /// Removes authored directional lights so [`ProjectSun`] drives outdoor lighting. pub fn use_project_sun(world: &mut World) { let mut query = world.query_filtered::<(Entity, &LightDesc), With>(); - let directionals: Vec = query + let changes = query .iter(world) .filter(|(_, light)| matches!(light.kind, AuthoringLightKind::Directional)) - .map(|(e, _)| e) - .collect(); - for entity in directionals { - if let Ok(mut entity_mut) = world.get_entity_mut(entity) { - entity_mut.remove::(); - } - } - world - .resource_mut::() - .mark_dirty(); + .map(|(entity, _)| (entity, None)) + .collect::>(); + set_light_group_with_history(world, changes, "Use Project Sun"); } pub fn create_scene_sun_override_from_project_settings(world: &mut World) -> Entity { diff --git a/crates/editor/src/viewport/brush_csg.rs b/crates/editor/src/viewport/brush_csg.rs index 1d3fcab..9332007 100644 --- a/crates/editor/src/viewport/brush_csg.rs +++ b/crates/editor/src/viewport/brush_csg.rs @@ -4,7 +4,7 @@ use bevy::math::Affine3A; use bevy::prelude::*; use shared::{ brush_math::{validate_brush, BrushDiagnosticSeverity}, - BrushDesc, LevelObject, + BrushDesc, }; use crate::history::apply_brush_csg_with_history; @@ -64,24 +64,27 @@ pub fn selected_brush_count(world: &World) -> usize { .resource::() .selected_entities .iter() - .filter(|entity| world.get::(*entity).is_some()) + .filter(|entity| { + world.get::(*entity).is_some() + && crate::ui::selection_ops::is_mutable_level_object(world, *entity) + }) .count() } -pub fn intersect_selected_brushes(world: &mut World) { - run_bounds_csg(world, BrushCsgOp::Intersect); +pub fn intersect_selected_brushes(world: &mut World) -> Result<(), String> { + run_bounds_csg(world, BrushCsgOp::Intersect) } -pub fn merge_selected_brushes(world: &mut World) { - run_bounds_csg(world, BrushCsgOp::Merge); +pub fn merge_selected_brushes(world: &mut World) -> Result<(), String> { + run_bounds_csg(world, BrushCsgOp::Merge) } -pub fn subtract_selected_brushes(world: &mut World) { - run_bounds_csg(world, BrushCsgOp::Subtract); +pub fn subtract_selected_brushes(world: &mut World) -> Result<(), String> { + run_bounds_csg(world, BrushCsgOp::Subtract) } #[derive(Resource, Default, Debug, Clone)] -struct BrushCsgPreview { +pub(crate) struct BrushCsgPreview { pending: Option, } @@ -99,18 +102,18 @@ enum BrushCsgOp { Subtract, } -fn run_bounds_csg(world: &mut World, op: BrushCsgOp) { +fn run_bounds_csg(world: &mut World, op: BrushCsgOp) -> Result<(), String> { let selected: Vec = world .resource::() .selected_entities .iter() .filter(|entity| { - world.get::(*entity).is_some() && world.get::(*entity).is_some() + world.get::(*entity).is_some() + && crate::ui::selection_ops::is_mutable_level_object(world, *entity) }) .collect(); if selected.len() < 2 { - set_status(world, "Select at least two brushes for CSG"); - return; + return csg_failure(world, "Select at least two editable brushes for CSG"); } for entity in &selected { let Some(brush) = world.get::(*entity) else { @@ -118,20 +121,18 @@ fn run_bounds_csg(world: &mut World, op: BrushCsgOp) { }; let validation = validate_brush(brush); if !validation.is_valid() { - set_status( + return csg_failure( world, format!( "CSG failed: selected brush is invalid: {}", first_brush_error(&validation) ), ); - return; } } let Some(primary_bounds) = brush_world_bounds(world, selected[0]) else { - set_status(world, "Primary brush has no valid bounds"); - return; + return csg_failure(world, "Primary brush has no valid bounds"); }; let result = match op { @@ -147,8 +148,7 @@ fn run_bounds_csg(world: &mut World, op: BrushCsgOp) { continue; }; let Some(intersection) = result.intersection(bounds) else { - set_status(world, "Brush intersect failed: no overlap"); - return; + return csg_failure(world, "Brush intersect failed: no overlap"); }; result = intersection; } @@ -156,16 +156,13 @@ fn run_bounds_csg(world: &mut World, op: BrushCsgOp) { } BrushCsgOp::Subtract => { let Some(cutter_bounds) = brush_world_bounds(world, selected[1]) else { - set_status(world, "Subtract failed: cutter brush has no valid bounds"); - return; + return csg_failure(world, "Subtract failed: cutter brush has no valid bounds"); }; let Some(overlap) = primary_bounds.intersection(cutter_bounds) else { - set_status(world, "Subtract failed: brushes do not overlap"); - return; + return csg_failure(world, "Subtract failed: brushes do not overlap"); }; let Some(result) = largest_remaining_slab(primary_bounds, overlap) else { - set_status(world, "Subtract failed: result would be empty"); - return; + return csg_failure(world, "Subtract failed: result would be empty"); }; result } @@ -175,14 +172,13 @@ fn run_bounds_csg(world: &mut World, op: BrushCsgOp) { let new_brush = BrushDesc::cuboid(size); let validation = validate_brush(&new_brush); if !validation.is_valid() { - set_status( + return csg_failure( world, format!( "CSG failed: result is invalid: {}", first_brush_error(&validation) ), ); - return; } world.resource_mut::().pending = Some(PendingBrushCsg { @@ -197,6 +193,13 @@ fn run_bounds_csg(world: &mut World, op: BrushCsgOp) { }; set_status(world, hint); set_csg_operator_status(world, OperatorPhase::Preview, hint); + Ok(()) +} + +fn csg_failure(world: &mut World, message: impl Into) -> Result<(), String> { + let message = message.into(); + set_status(world, message.clone()); + Err(message) } fn brush_csg_preview_input(world: &mut World) { @@ -253,6 +256,22 @@ fn commit_csg_preview(world: &mut World, pending: PendingBrushCsg) { ); return; } + if pending + .selected + .iter() + .any(|entity| !crate::ui::selection_ops::is_mutable_level_object(world, *entity)) + { + set_status( + world, + "Brush CSG commit blocked: selected content is no longer editable", + ); + set_csg_operator_status( + world, + OperatorPhase::Blocked, + "Commit blocked: selected content is no longer editable", + ); + return; + } let new_brush = BrushDesc::cuboid(pending.result.size()); let to_delete: Vec<_> = if matches!(pending.op, BrushCsgOp::Merge | BrushCsgOp::Intersect) { pending @@ -265,7 +284,11 @@ fn commit_csg_preview(world: &mut World, pending: PendingBrushCsg) { } else { Vec::new() }; - apply_bounds_result(world, primary, pending.result, new_brush, &to_delete); + if !apply_bounds_result(world, primary, pending.result, new_brush, &to_delete) { + set_status(world, "Brush CSG commit made no changes"); + set_csg_operator_status(world, OperatorPhase::Canceled, "No changes"); + return; + } let hint = match pending.op { BrushCsgOp::Intersect => "Brush intersect committed", BrushCsgOp::Merge => "Brush convex merge committed", @@ -332,14 +355,14 @@ fn apply_bounds_result( bounds: BrushBounds, new_brush: BrushDesc, delete_entities: &[Entity], -) { +) -> bool { let center = bounds.center(); let old_transform = world.get::(entity).copied().unwrap_or_default(); let mut new_transform = old_transform; new_transform.translation = center; new_transform.rotation = Quat::IDENTITY; new_transform.scale = Vec3::ONE; - apply_brush_csg_with_history(world, entity, new_transform, new_brush, delete_entities); + apply_brush_csg_with_history(world, entity, new_transform, new_brush, delete_entities) } fn brush_world_bounds(world: &World, entity: Entity) -> Option { @@ -426,6 +449,7 @@ mod tests { use crate::history::{EditorCommand, EditorHistory}; use crate::operators::test_harness::{assert_undo_redo_round_trip, OperatorInvariantHarness}; use crate::selection::SelectedEntity; + use shared::LevelObject; fn bounds(min: Vec3, max: Vec3) -> BrushBounds { BrushBounds { min, max } diff --git a/crates/editor/src/viewport/brush_edit.rs b/crates/editor/src/viewport/brush_edit.rs index 821dd66..5b90438 100644 --- a/crates/editor/src/viewport/brush_edit.rs +++ b/crates/editor/src/viewport/brush_edit.rs @@ -1308,4 +1308,55 @@ mod tests { assert!(state.old_brush.is_none()); assert!(!state.changed); } + + #[test] + fn brush_element_release_uses_production_finalizer_and_round_trips() { + let mut app = App::new(); + app.init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .add_systems(Update, apply_brush_element_gizmo); + let original = BrushDesc::cuboid(Vec3::ONE); + let preview = BrushDesc::cuboid(Vec3::new(1.5, 1.0, 1.0)); + let brush_entity = app + .world_mut() + .spawn((LevelObject, preview.clone(), GlobalTransform::default())) + .id(); + let helper = app + .world_mut() + .spawn(( + BrushElementGizmo, + Transform::default(), + GizmoTarget::default(), + )) + .id(); + *app.world_mut().resource_mut::() = BrushEditMode::Vertex; + *app.world_mut().resource_mut::() = BrushElementSelection { + brush: Some(brush_entity), + elements: vec![BrushElementKey::Vertex { + face: original.faces[0].id.clone(), + index: 0, + }], + }; + *app.world_mut().resource_mut::() = BrushElementGizmoState { + entity: Some(helper), + brush: Some(brush_entity), + old_brush: Some(original.clone()), + old_gizmo: Some(Transform::default()), + last_gizmo: Some(Transform::from_translation(Vec3::X * 0.25)), + changed: true, + }; + let harness = OperatorInvariantHarness::capture(app.world_mut()); + + app.update(); + + harness.assert_committed(app.world_mut(), 1, 0); + harness.assert_status(app.world(), "brush.edit_mode", OperatorPhase::Committed); + assert_undo_redo_round_trip(app.world_mut(), original, preview, |world| { + world.get::(brush_entity).unwrap().clone() + }); + } } diff --git a/crates/editor/src/viewport/material_drop.rs b/crates/editor/src/viewport/material_drop.rs index 69cf5b8..d9571a2 100644 --- a/crates/editor/src/viewport/material_drop.rs +++ b/crates/editor/src/viewport/material_drop.rs @@ -779,31 +779,17 @@ fn update_material_drop_session(world: &mut World) { .is_some_and(|buttons| buttons.just_pressed(MouseButton::Right)); if cancel_requested && (selection.is_some() || state.preview.is_some()) { - restore_material_drop_preview(world, state.preview.take()); + cancel_material_drop_session(world, &mut state, true); clear_drag_and_viewport_click(world); - state.feedback = None; - state.captures_viewport_input = false; - state.cancel_consumed = true; - set_drop_operator_status( - world, - OperatorPhase::Canceled, - "Surface assignment canceled", - ); world.insert_resource(state); return; } let Some(selection) = selection else { - let had_preview = state.preview.is_some(); - restore_material_drop_preview(world, state.preview.take()); - state.feedback = None; - state.captures_viewport_input = false; - if had_preview { - set_drop_operator_status( - world, - OperatorPhase::Canceled, - "Surface assignment canceled", - ); + if state.preview.is_some() { + cancel_material_drop_session(world, &mut state, false); + } else { + reset_material_drop_session(&mut state, false); } world.insert_resource(state); return; @@ -877,19 +863,44 @@ fn update_material_drop_session(world: &mut World) { ); if primary_released { - let preview = state.preview.take(); - if let Some(preview) = preview.as_ref() { - preview.snapshot.restore(world); - } - if let Some(preview) = preview { - commit_material_drop_preview(world, preview); - } + commit_material_drop_session(world, &mut state); clear_drag_and_viewport_click(world); - state.captures_viewport_input = false; } world.insert_resource(state); } +fn cancel_material_drop_session( + world: &mut World, + state: &mut MaterialDropState, + cancel_consumed: bool, +) { + restore_material_drop_preview(world, state.preview.take()); + reset_material_drop_session(state, cancel_consumed); + set_drop_operator_status( + world, + OperatorPhase::Canceled, + "Surface assignment canceled", + ); +} + +fn commit_material_drop_session(world: &mut World, state: &mut MaterialDropState) -> bool { + let Some(preview) = state.preview.take() else { + reset_material_drop_session(state, false); + return false; + }; + preview.snapshot.restore(world); + let committed = commit_material_drop_preview(world, preview); + reset_material_drop_session(state, false); + committed +} + +fn reset_material_drop_session(state: &mut MaterialDropState, cancel_consumed: bool) { + state.resolved = None; + state.feedback = None; + state.captures_viewport_input = false; + state.cancel_consumed = cancel_consumed; +} + fn restore_material_drop_preview(world: &mut World, preview: Option) { if let Some(preview) = preview { preview.snapshot.restore(world); @@ -1029,7 +1040,8 @@ fn draw_material_drop_target( #[cfg(test)] mod tests { use super::*; - use crate::history::{apply_command_undo, EditorHistory}; + use crate::history::EditorHistory; + use crate::operators::test_harness::{assert_undo_redo_round_trip, OperatorInvariantHarness}; use shared::{ActorKind, RendererMaterialSet, RendererMaterialSlot, SharedTypesPlugin}; fn test_app() -> App { @@ -1098,6 +1110,7 @@ mod tests { hit_point: Vec3::ZERO, hit_normal: Vec3::Y, }; + let harness = OperatorInvariantHarness::capture(world); let preview = begin_material_drop_preview( world, AssetSelection::File("assets/materials/steel.ron".into()), @@ -1123,18 +1136,25 @@ mod tests { assert!(!world.resource::().dirty); assert_eq!(world.resource::().undo_depth(), 0); - preview.snapshot.restore(world); - assert!(world - .get::(entity) - .unwrap() - .materials - .slot(&slot_b) - .unwrap() - .material - .is_none()); - assert!(commit_material_drop_preview(world, preview)); - assert_eq!(world.resource::().undo_depth(), 1); - assert!(world.resource::().dirty); + let mut state = MaterialDropState { + resolved: Some(target), + preview: Some(preview), + feedback: Some(MaterialDropFeedback { + valid: true, + action: "Assign material".into(), + target: "Crate / B".into(), + }), + captures_viewport_input: true, + cancel_consumed: false, + }; + assert!(commit_material_drop_session(world, &mut state)); + harness.assert_committed(world, 1, 0); + harness.assert_status(world, "assets.assign_surface", OperatorPhase::Committed); + assert!(state.preview.is_none()); + assert!(state.resolved.is_none()); + assert!(state.feedback.is_none()); + assert!(!state.captures_viewport_input); + assert!(!state.cancel_consumed); assert!(world .get::(entity) .unwrap() @@ -1143,16 +1163,17 @@ mod tests { .unwrap() .material .is_none()); - - apply_command_undo(world); - assert!(world - .get::(entity) - .unwrap() - .materials - .slot(&slot_b) - .unwrap() - .material - .is_none()); + assert_undo_redo_round_trip(world, None, Some("steel".to_string()), |world| { + world + .get::(entity) + .unwrap() + .materials + .slot(&slot_b) + .unwrap() + .material + .as_ref() + .map(|material| material.0.asset_id.clone()) + }); } #[test] @@ -1172,6 +1193,7 @@ mod tests { hit_point: Vec3::ZERO, hit_normal: Vec3::Y, }; + let harness = OperatorInvariantHarness::capture(world); let preview = begin_material_drop_preview( world, AssetSelection::File("assets/materials/steel.ron".into()), @@ -1196,9 +1218,26 @@ mod tests { .material .is_none()); - preview.snapshot.restore(world); + let mut state = MaterialDropState { + resolved: Some(target), + preview: Some(preview), + feedback: Some(MaterialDropFeedback { + valid: true, + action: "Assign material".into(), + target: "Blockout / face".into(), + }), + captures_viewport_input: true, + cancel_consumed: false, + }; + cancel_material_drop_session(world, &mut state, true); + harness.assert_canceled(world); + harness.assert_status(world, "assets.assign_surface", OperatorPhase::Canceled); assert_eq!(world.get::(entity), Some(&brush)); - assert!(!world.resource::().dirty); + assert!(state.preview.is_none()); + assert!(state.resolved.is_none()); + assert!(state.feedback.is_none()); + assert!(!state.captures_viewport_input); + assert!(state.cancel_consumed); } #[test] diff --git a/crates/editor/src/viewport/physics_placement.rs b/crates/editor/src/viewport/physics_placement.rs index 59059ce..189e075 100644 --- a/crates/editor/src/viewport/physics_placement.rs +++ b/crates/editor/src/viewport/physics_placement.rs @@ -6,7 +6,7 @@ use std::time::Duration; use avian3d::prelude::*; use bevy::prelude::*; -use crate::history::set_transform_group_with_history; +use crate::history::{set_transform_group_with_history, EditorHistory}; use crate::operators::{ActiveOperator, OperatorPhase, OperatorStatus}; use crate::state::EditorMode; use crate::ui::selection_ops::{entity_name, is_mutable_level_object}; @@ -256,6 +256,7 @@ fn finish_physics_placement(world: &mut World, commit: bool) { } } + let undo_depth = world.resource::().undo_depth(); if commit { let changes = session.selected.iter().filter_map(|snapshot| { final_transforms @@ -265,6 +266,7 @@ fn finish_physics_placement(world: &mut World, commit: bool) { }); set_transform_group_with_history(world, changes); } + let committed = commit && world.resource::().undo_depth() > undo_depth; state.phase = PhysicsPlacementPhase::Inactive; state.request = PhysicsPlacementRequest::None; @@ -272,7 +274,14 @@ fn finish_physics_placement(world: &mut World, commit: bool) { state.simulated_steps = 0; state.last_error = None; world.insert_resource(state); - clear_operator_status(world); + let (phase, hint) = if committed { + (OperatorPhase::Committed, "Physics placement committed") + } else if commit { + (OperatorPhase::Canceled, "Physics placement made no changes") + } else { + (OperatorPhase::Canceled, "Physics placement canceled") + }; + set_operator_status(world, phase, hint.to_string(), Vec::new()); } fn tick_physics_placement(world: &mut World) { @@ -437,22 +446,12 @@ fn set_operator_status( } } -fn clear_operator_status(world: &mut World) { - if let Some(mut operator) = world.get_resource_mut::() { - if operator - .status - .as_ref() - .is_some_and(|status| status.id == "physics.placement") - { - operator.status = None; - } - } -} - #[cfg(test)] mod tests { use super::*; - use crate::history::{apply_command_undo, EditorHistory}; + use crate::history::EditorHistory; + use crate::operators::test_harness::{assert_undo_redo_round_trip, OperatorInvariantHarness}; + use crate::scene_io::SceneIo; use bevy::asset::{AssetApp, AssetPlugin}; use bevy::state::app::StatesPlugin; use shared::{ColliderDesc, LevelObject, RigidBodyDesc}; @@ -469,7 +468,9 @@ mod tests { .init_asset::() .init_state::() .init_resource::() - .init_resource::(); + .init_resource::() + .init_resource::() + .init_resource::(); app.finish(); app.cleanup(); app.update(); @@ -498,12 +499,16 @@ mod tests { .world_mut() .spawn((Name::new("No Physics"), LevelObject, Transform::default())) .id(); + let harness = OperatorInvariantHarness::capture(app.world_mut()); let error = begin_physics_placement(app.world_mut(), [entity]).unwrap_err(); assert!(error.contains("add and enable a Rigid Body")); assert!(error.contains("add and enable a non-trigger Collider")); assert!(!app.world().resource::().active()); + harness.assert_blocked(app.world_mut()); + harness.assert_status(app.world(), "physics.placement", OperatorPhase::Blocked); + harness.assert_no_helpers::(app.world_mut()); } #[test] @@ -531,6 +536,7 @@ mod tests { )) .id(); app.update(); + let harness = OperatorInvariantHarness::capture(app.world_mut()); begin_physics_placement(app.world_mut(), [selected]).unwrap(); assert_eq!( @@ -581,6 +587,9 @@ mod tests { Some(&LinearVelocity(Vec3::X * 3.0)) ); assert_eq!(app.world().resource::().undo_depth(), 0); + harness.assert_canceled(app.world_mut()); + harness.assert_status(app.world(), "physics.placement", OperatorPhase::Canceled); + harness.assert_no_helpers::(app.world_mut()); } #[test] @@ -594,6 +603,7 @@ mod tests { let start = Transform::from_xyz(0.0, 4.0, 0.0); let prop = spawn_placeable(app.world_mut(), "Drop Prop", start); app.update(); + let harness = OperatorInvariantHarness::capture(app.world_mut()); begin_physics_placement(app.world_mut(), [prop]).unwrap(); for _ in 0..400 { @@ -613,15 +623,75 @@ mod tests { commit_physics_placement(app.world_mut()); - assert_eq!(app.world().resource::().undo_depth(), 1); + harness.assert_committed(app.world_mut(), 1, 0); + harness.assert_status(app.world(), "physics.placement", OperatorPhase::Committed); + harness.assert_no_helpers::(app.world_mut()); assert_eq!( app.world().resource::().status, "Undo: Move Selection" ); assert_eq!(app.world().get::(prop), Some(&RigidBody::Static)); assert_eq!(*app.world().get::(prop).unwrap(), settled); + assert_undo_redo_round_trip(app.world_mut(), start, settled, |world| { + *world.get::(prop).unwrap() + }); + } - apply_command_undo(app.world_mut()); + #[test] + fn no_op_commit_terminates_without_dirtying_or_history() { + let mut app = app_with_physics(); + let start = Transform::from_xyz(0.0, 4.0, 0.0); + let prop = spawn_placeable(app.world_mut(), "Static Prop", start); + app.update(); + let harness = OperatorInvariantHarness::capture(app.world_mut()); + + begin_physics_placement(app.world_mut(), [prop]).unwrap(); + commit_physics_placement(app.world_mut()); + + harness.assert_canceled(app.world_mut()); + harness.assert_status(app.world(), "physics.placement", OperatorPhase::Canceled); + harness.assert_no_helpers::(app.world_mut()); assert_eq!(*app.world().get::(prop).unwrap(), start); + assert_eq!(app.world().get::(prop), Some(&RigidBody::Static)); + } + + #[test] + fn multi_select_commit_is_one_transaction_and_cleans_every_preview_helper() { + let mut app = app_with_physics(); + let starts = [ + Transform::from_xyz(-1.0, 4.0, 0.0), + Transform::from_xyz(1.0, 6.0, 0.0), + ]; + let first = spawn_placeable(app.world_mut(), "First", starts[0]); + let second = spawn_placeable(app.world_mut(), "Second", starts[1]); + app.update(); + let harness = OperatorInvariantHarness::capture(app.world_mut()); + + begin_physics_placement(app.world_mut(), [first, second, first]).unwrap(); + assert_eq!( + app.world() + .resource::() + .selected_count, + 2, + "duplicate selections must collapse to one placement snapshot" + ); + let committed = [ + Transform::from_xyz(-1.0, 0.5, 0.0), + Transform::from_xyz(1.0, 0.5, 0.0), + ]; + app.world_mut().entity_mut(first).insert(committed[0]); + app.world_mut().entity_mut(second).insert(committed[1]); + + commit_physics_placement(app.world_mut()); + + harness.assert_committed(app.world_mut(), 1, 0); + harness.assert_status(app.world(), "physics.placement", OperatorPhase::Committed); + harness.assert_no_helpers::(app.world_mut()); + assert_undo_redo_round_trip(app.world_mut(), starts, committed, |world| { + [ + *world.get::(first).unwrap(), + *world.get::(second).unwrap(), + ] + }); } } diff --git a/crates/editor/src/viewport/terrain_paint.rs b/crates/editor/src/viewport/terrain_paint.rs index ed80ded..1b00aa7 100644 --- a/crates/editor/src/viewport/terrain_paint.rs +++ b/crates/editor/src/viewport/terrain_paint.rs @@ -6,7 +6,7 @@ use shared::{TerrainDesc, AUTHORING_COMPONENT_TERRAIN, COMPONENT_TERRAIN_DESC}; use super::terrain_sculpt::{active_scene_ray, smooth_falloff, terrain_ray_hit, TerrainHit}; use crate::camera::EditorCamera; -use crate::history::reflected_component_transaction; +use crate::history::{reflected_component_transaction, EditorHistory}; use crate::operators::{ActiveOperator, OperatorPhase, OperatorStatus}; use crate::scene_io::SceneIo; use crate::selection::{SelectedEntity, ViewportClick}; @@ -125,16 +125,30 @@ fn terrain_paint_input( viewport_click.0 = None; state.clamp_settings(); let Some(entity) = state.target.or(selected.0) else { - cancel_stroke(&mut state, &mut terrains); - state.stop(); + stop_tool_with_rollback( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + OperatorPhase::Blocked, + "Terrain paint stopped: target is unavailable", + "Target unavailable", + ); return Ok(()); }; if selected.0 != Some(entity) { selected.0 = Some(entity); } if display.clean_game_view { - cancel_stroke(&mut state, &mut terrains); - state.stop(); + stop_tool_with_rollback( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + OperatorPhase::Canceled, + "Terrain paint stopped for clean game view", + "Clean game view enabled; weights restored", + ); return Ok(()); } let ctx = contexts.ctx_mut()?; @@ -142,12 +156,11 @@ fn terrain_paint_input( keys.just_pressed(KeyCode::Escape) || buttons.just_pressed(MouseButton::Right); if cancel_requested { if state.is_stroking() { - cancel_stroke(&mut state, &mut terrains); - scene_io.status = "Terrain paint stroke canceled".into(); - set_status( + cancel_active_stroke( + &mut state, + &mut terrains, + &mut scene_io, &mut active_operator, - OperatorPhase::Canceled, - "Weights restored", ); } else { state.stop(); @@ -157,14 +170,29 @@ fn terrain_paint_input( return Ok(()); } let Ok((global, mut terrain)) = terrains.get_mut(entity) else { - state.stroke = None; - state.stop(); + stop_tool_with_rollback( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + OperatorPhase::Blocked, + "Terrain paint stopped: target no longer exists", + "Target no longer exists", + ); return Ok(()); }; let layer_count = terrain.material_layers.len(); if layer_count == 0 { + if let Some(stroke) = state.stroke.take() { + *terrain = stroke.original; + } state.stop(); scene_io.status = "Add a terrain material layer before painting".into(); + set_status( + &mut active_operator, + OperatorPhase::Blocked, + "Add a material layer before painting", + ); return Ok(()); } state.active_layer = state.active_layer.min(layer_count - 1); @@ -227,17 +255,8 @@ fn terrain_paint_input( let final_terrain = terrain.clone(); let mode = state.mode; commands.queue(move |world: &mut World| { - if let Err(error) = commit_paint_stroke(world, stroke, final_terrain) { - world.resource_mut::().status = - format!("Terrain paint commit failed: {error}"); - } + finish_paint_stroke(world, stroke, final_terrain, mode); }); - scene_io.status = format!("{} terrain material weights committed", mode.label()); - set_status( - &mut active_operator, - OperatorPhase::Committed, - format!("{} weights; Ctrl+Z to undo", mode.label()), - ); } } Ok(()) @@ -248,10 +267,11 @@ fn commit_paint_stroke( stroke: TerrainPaintStroke, final_terrain: TerrainDesc, ) -> Result<(), String> { + let original = stroke.original; if let Ok(mut actor) = world.get_entity_mut(stroke.entity) { - actor.insert(stroke.original); + actor.insert(original.clone()); } - reflected_component_transaction( + let result = reflected_component_transaction( world, stroke.entity, "Paint Terrain Material", @@ -261,7 +281,77 @@ fn commit_paint_stroke( world.entity_mut(entity).insert(final_terrain); Ok(()) }, - ) + ); + if result.is_err() { + if let Ok(mut actor) = world.get_entity_mut(stroke.entity) { + actor.insert(original); + } + } + result +} + +fn finish_paint_stroke( + world: &mut World, + stroke: TerrainPaintStroke, + final_terrain: TerrainDesc, + mode: TerrainPaintMode, +) { + let undo_depth = world.resource::().undo_depth(); + match commit_paint_stroke(world, stroke, final_terrain) { + Ok(()) if world.resource::().undo_depth() > undo_depth => { + world.resource_mut::().status = + format!("{} terrain material weights committed", mode.label()); + set_status( + &mut world.resource_mut::(), + OperatorPhase::Committed, + format!("{} weights; Ctrl+Z to undo", mode.label()), + ); + } + Ok(()) => { + world.resource_mut::().status = + format!("{} terrain stroke made no changes", mode.label()); + set_status( + &mut world.resource_mut::(), + OperatorPhase::Canceled, + format!("{} stroke made no changes", mode.label()), + ); + } + Err(error) => { + world.resource_mut::().status = + format!("Terrain paint commit failed: {error}"); + set_status( + &mut world.resource_mut::(), + OperatorPhase::Blocked, + format!("Commit failed: {error}"), + ); + } + } +} + +fn cancel_active_stroke( + state: &mut TerrainPaintState, + terrains: &mut Query<(&GlobalTransform, &mut TerrainDesc)>, + scene_io: &mut SceneIo, + active_operator: &mut ActiveOperator, +) { + cancel_stroke(state, terrains); + scene_io.status = "Terrain paint stroke canceled".into(); + set_status(active_operator, OperatorPhase::Canceled, "Weights restored"); +} + +fn stop_tool_with_rollback( + state: &mut TerrainPaintState, + terrains: &mut Query<(&GlobalTransform, &mut TerrainDesc)>, + scene_io: &mut SceneIo, + active_operator: &mut ActiveOperator, + phase: OperatorPhase, + message: &str, + hint: &str, +) { + cancel_stroke(state, terrains); + state.stop(); + scene_io.status = message.to_string(); + set_status(active_operator, phase, hint); } fn cancel_stroke( @@ -415,7 +505,8 @@ fn draw_terrain_paint_preview(state: Res, mut gizmos: Gizmos) #[cfg(test)] mod tests { use super::*; - use crate::history::{apply_command_redo, apply_command_undo, EditorHistory}; + use crate::history::EditorHistory; + use crate::operators::test_harness::{assert_undo_redo_round_trip, OperatorInvariantHarness}; use shared::{ActorKind, LevelObject}; #[test] @@ -452,8 +543,10 @@ mod tests { .register_type::() .register_type::(); let world = app.world_mut(); + world.init_resource::(); world.init_resource::(); world.init_resource::(); + world.init_resource::(); let mut original = TerrainDesc::flat(9); original.material_layers = vec![Default::default(), Default::default()]; let entity = world @@ -471,23 +564,121 @@ mod tests { ); } world.entity_mut(entity).insert(final_terrain.clone()); - commit_paint_stroke( + let stroke = TerrainPaintStroke { + entity, + original: original.clone(), + last_local: Vec3::ZERO, + }; + { + let mut state = world.resource_mut::(); + state.start(entity, 2); + state.stroke = Some(stroke); + } + let stroke = world + .resource_mut::() + .stroke + .take() + .unwrap(); + let harness = OperatorInvariantHarness::capture(world); + + finish_paint_stroke( + world, + stroke, + final_terrain.clone(), + TerrainPaintMode::Paint, + ); + + harness.assert_committed(world, 1, 0); + harness.assert_status(world, "terrain.paint", OperatorPhase::Committed); + assert_eq!(world.get::(entity), Some(&final_terrain)); + let state = world.resource::(); + assert!(state.active); + assert_eq!(state.target, Some(entity)); + assert!(!state.is_stroking()); + assert_undo_redo_round_trip(world, original, final_terrain, |world| { + world.get::(entity).unwrap().clone() + }); + } + + #[test] + fn no_op_paint_release_terminates_without_dirtying_or_history() { + let mut app = App::new(); + app.register_type::() + .register_type::() + .register_type::() + .register_type::(); + let world = app.world_mut(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let mut original = TerrainDesc::flat(5); + original.material_layers = vec![Default::default(), Default::default()]; + original.ensure_material_weights(); + let entity = world + .spawn((LevelObject, ActorKind::Terrain, original.clone())) + .id(); + let harness = OperatorInvariantHarness::capture(world); + + finish_paint_stroke( world, TerrainPaintStroke { entity, original: original.clone(), last_local: Vec3::ZERO, }, - final_terrain.clone(), - ) - .unwrap(); + original.clone(), + TerrainPaintMode::Erase, + ); - assert_eq!(world.resource::().undo_depth(), 1); - assert_eq!(world.get::(entity), Some(&final_terrain)); - apply_command_undo(world); + harness.assert_canceled(world); + harness.assert_status(world, "terrain.paint", OperatorPhase::Canceled); assert_eq!(world.get::(entity), Some(&original)); - apply_command_redo(world); - assert_eq!(world.get::(entity), Some(&final_terrain)); + } + + #[test] + fn commit_failure_restores_preview_and_reports_blocked_without_history() { + let mut app = App::new(); + let world = app.world_mut(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let mut original = TerrainDesc::flat(5); + original.material_layers = vec![Default::default(), Default::default()]; + let mut preview = original.clone(); + apply_weight_dab( + &mut preview, + Vec3::ZERO, + 3.0, + 0.8, + 1, + TerrainPaintMode::Paint, + ); + let entity = world + .spawn((LevelObject, ActorKind::Terrain, preview.clone())) + .id(); + let mut hierarchy = crate::ui::hierarchy_state::HierarchyPanelState::default(); + hierarchy.locked.insert(entity); + world.insert_resource(hierarchy); + let harness = OperatorInvariantHarness::capture(world); + + finish_paint_stroke( + world, + TerrainPaintStroke { + entity, + original: original.clone(), + last_local: Vec3::ZERO, + }, + preview, + TerrainPaintMode::Paint, + ); + + harness.assert_blocked(world); + harness.assert_status(world, "terrain.paint", OperatorPhase::Blocked); + assert_eq!(world.get::(entity), Some(&original)); + assert!(world + .resource::() + .status + .starts_with("Terrain paint commit failed:")); } #[test] @@ -495,11 +686,21 @@ mod tests { fn cancel_once( mut state: ResMut, mut terrains: Query<(&GlobalTransform, &mut TerrainDesc)>, + mut scene_io: ResMut, + mut active_operator: ResMut, ) { - cancel_stroke(&mut state, &mut terrains); + cancel_active_stroke( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + ); } let mut app = App::new(); + app.init_resource::() + .init_resource::() + .init_resource::(); let mut original = TerrainDesc::flat(5); original.material_layers = vec![Default::default(), Default::default()]; let mut preview = original.clone(); @@ -513,7 +714,7 @@ mod tests { ); let entity = app .world_mut() - .spawn((GlobalTransform::default(), preview)) + .spawn((LevelObject, GlobalTransform::default(), preview)) .id(); let mut state = TerrainPaintState::default(); state.start(entity, 2); @@ -523,9 +724,72 @@ mod tests { last_local: Vec3::ZERO, }); app.insert_resource(state).add_systems(Update, cancel_once); + let harness = OperatorInvariantHarness::capture(app.world_mut()); app.update(); + harness.assert_canceled(app.world_mut()); + harness.assert_status(app.world(), "terrain.paint", OperatorPhase::Canceled); assert_eq!(app.world().get::(entity), Some(&original)); - assert!(!app.world().resource::().is_stroking()); + let state = app.world().resource::(); + assert!(state.active); + assert_eq!(state.target, Some(entity)); + assert!(!state.is_stroking()); + } + + #[test] + fn clean_view_interruption_restores_weights_and_terminates_tool() { + fn interrupt_once( + mut state: ResMut, + mut terrains: Query<(&GlobalTransform, &mut TerrainDesc)>, + mut scene_io: ResMut, + mut active_operator: ResMut, + ) { + stop_tool_with_rollback( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + OperatorPhase::Canceled, + "Terrain paint stopped for clean game view", + "Clean game view enabled; weights restored", + ); + } + + let mut app = App::new(); + app.init_resource::() + .init_resource::() + .init_resource::(); + let mut original = TerrainDesc::flat(5); + original.material_layers = vec![Default::default(), Default::default()]; + let mut preview = original.clone(); + apply_weight_dab( + &mut preview, + Vec3::ZERO, + 3.0, + 0.8, + 1, + TerrainPaintMode::Paint, + ); + let entity = app + .world_mut() + .spawn((LevelObject, GlobalTransform::default(), preview)) + .id(); + let mut state = TerrainPaintState::default(); + state.start(entity, 2); + state.stroke = Some(TerrainPaintStroke { + entity, + original: original.clone(), + last_local: Vec3::ZERO, + }); + app.insert_resource(state) + .add_systems(Update, interrupt_once); + let harness = OperatorInvariantHarness::capture(app.world_mut()); + + app.update(); + + harness.assert_canceled(app.world_mut()); + harness.assert_status(app.world(), "terrain.paint", OperatorPhase::Canceled); + assert_eq!(app.world().get::(entity), Some(&original)); + assert!(!app.world().resource::().active); } } diff --git a/crates/editor/src/viewport/terrain_sculpt.rs b/crates/editor/src/viewport/terrain_sculpt.rs index 6495217..2f4cd81 100644 --- a/crates/editor/src/viewport/terrain_sculpt.rs +++ b/crates/editor/src/viewport/terrain_sculpt.rs @@ -5,7 +5,7 @@ use bevy_egui::EguiContexts; use shared::{TerrainDesc, AUTHORING_COMPONENT_TERRAIN, COMPONENT_TERRAIN_DESC}; use crate::camera::EditorCamera; -use crate::history::reflected_component_transaction; +use crate::history::{reflected_component_transaction, EditorHistory}; use crate::operators::{ActiveOperator, OperatorPhase, OperatorStatus}; use crate::scene_io::SceneIo; use crate::selection::{SelectedEntity, ViewportClick}; @@ -143,8 +143,15 @@ fn terrain_sculpt_input( state.clamp_settings(); let Some(entity) = state.target.or(selected.0) else { - cancel_stroke(&mut state, &mut terrains); - state.stop(); + stop_tool_with_rollback( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + OperatorPhase::Blocked, + "Terrain sculpt stopped: target is unavailable", + "Target unavailable", + ); return Ok(()); }; if selected.0 != Some(entity) { @@ -152,8 +159,15 @@ fn terrain_sculpt_input( } if display.clean_game_view { - cancel_stroke(&mut state, &mut terrains); - state.stop(); + stop_tool_with_rollback( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + OperatorPhase::Canceled, + "Terrain sculpt stopped for clean game view", + "Clean game view enabled; terrain restored", + ); return Ok(()); } @@ -163,12 +177,11 @@ fn terrain_sculpt_input( keys.just_pressed(KeyCode::Escape) || buttons.just_pressed(MouseButton::Right); if cancel_requested { if state.is_stroking() { - cancel_stroke(&mut state, &mut terrains); - scene_io.status = "Terrain sculpt stroke canceled".to_string(); - set_status( + cancel_active_stroke( + &mut state, + &mut terrains, + &mut scene_io, &mut active_operator, - OperatorPhase::Canceled, - "Stroke canceled; terrain restored", ); } else { state.stop(); @@ -179,8 +192,15 @@ fn terrain_sculpt_input( } let Ok((global, mut terrain)) = terrains.get_mut(entity) else { - state.stroke = None; - state.stop(); + stop_tool_with_rollback( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + OperatorPhase::Blocked, + "Terrain sculpt stopped: target no longer exists", + "Target no longer exists", + ); return Ok(()); }; @@ -253,18 +273,8 @@ fn terrain_sculpt_input( let label = sculpt_history_label(state.mode); let mode_label = state.mode.label(); commands.queue(move |world: &mut World| { - let result = commit_terrain_stroke(world, stroke, final_terrain, label); - if let Err(error) = result { - world.resource_mut::().status = - format!("Terrain sculpt commit failed: {error}"); - } + finish_terrain_stroke(world, stroke, final_terrain, label, mode_label); }); - scene_io.status = format!("{mode_label} terrain stroke committed"); - set_status( - &mut active_operator, - OperatorPhase::Committed, - format!("{mode_label} stroke; Ctrl+Z to undo"), - ); } } Ok(()) @@ -276,10 +286,11 @@ fn commit_terrain_stroke( final_terrain: TerrainDesc, label: &'static str, ) -> Result<(), String> { + let original = stroke.original; if let Ok(mut actor) = world.get_entity_mut(stroke.entity) { - actor.insert(stroke.original); + actor.insert(original.clone()); } - reflected_component_transaction( + let result = reflected_component_transaction( world, stroke.entity, label, @@ -289,7 +300,82 @@ fn commit_terrain_stroke( world.entity_mut(entity).insert(final_terrain); Ok(()) }, - ) + ); + if result.is_err() { + if let Ok(mut actor) = world.get_entity_mut(stroke.entity) { + actor.insert(original); + } + } + result +} + +fn finish_terrain_stroke( + world: &mut World, + stroke: TerrainStroke, + final_terrain: TerrainDesc, + label: &'static str, + mode_label: &'static str, +) { + let undo_depth = world.resource::().undo_depth(); + match commit_terrain_stroke(world, stroke, final_terrain, label) { + Ok(()) if world.resource::().undo_depth() > undo_depth => { + world.resource_mut::().status = + format!("{mode_label} terrain stroke committed"); + set_status( + &mut world.resource_mut::(), + OperatorPhase::Committed, + format!("{mode_label} stroke; Ctrl+Z to undo"), + ); + } + Ok(()) => { + world.resource_mut::().status = + format!("{mode_label} terrain stroke made no changes"); + set_status( + &mut world.resource_mut::(), + OperatorPhase::Canceled, + format!("{mode_label} stroke made no changes"), + ); + } + Err(error) => { + world.resource_mut::().status = + format!("Terrain sculpt commit failed: {error}"); + set_status( + &mut world.resource_mut::(), + OperatorPhase::Blocked, + format!("Commit failed: {error}"), + ); + } + } +} + +fn cancel_active_stroke( + state: &mut TerrainSculptState, + terrains: &mut Query<(&GlobalTransform, &mut TerrainDesc)>, + scene_io: &mut SceneIo, + active_operator: &mut ActiveOperator, +) { + cancel_stroke(state, terrains); + scene_io.status = "Terrain sculpt stroke canceled".to_string(); + set_status( + active_operator, + OperatorPhase::Canceled, + "Stroke canceled; terrain restored", + ); +} + +fn stop_tool_with_rollback( + state: &mut TerrainSculptState, + terrains: &mut Query<(&GlobalTransform, &mut TerrainDesc)>, + scene_io: &mut SceneIo, + active_operator: &mut ActiveOperator, + phase: OperatorPhase, + message: &str, + hint: &str, +) { + cancel_stroke(state, terrains); + state.stop(); + scene_io.status = message.to_string(); + set_status(active_operator, phase, hint); } fn cancel_stroke( @@ -561,7 +647,8 @@ fn draw_terrain_sculpt_preview(state: Res, mut gizmos: Gizmo #[cfg(test)] mod tests { use super::*; - use crate::history::{apply_command_redo, apply_command_undo, EditorHistory}; + use crate::history::EditorHistory; + use crate::operators::test_harness::{assert_undo_redo_round_trip, OperatorInvariantHarness}; use shared::{ActorKind, LevelObject}; fn sample(terrain: &TerrainDesc, x: u32, z: u32) -> f32 { @@ -657,8 +744,10 @@ mod tests { app.register_type::() .register_type::(); let world = app.world_mut(); + world.init_resource::(); world.init_resource::(); world.init_resource::(); + world.init_resource::(); let original = TerrainDesc::flat(9); let entity = world .spawn((LevelObject, ActorKind::Terrain, original.clone())) @@ -676,26 +765,126 @@ mod tests { ); } world.entity_mut(entity).insert(final_terrain.clone()); - commit_terrain_stroke( + let stroke = TerrainStroke { + entity, + original: original.clone(), + last_local: Vec3::ZERO, + flatten_height: 0.0, + seed: 7, + }; + { + let mut state = world.resource_mut::(); + state.start(entity); + state.stroke = Some(stroke); + } + let stroke = world + .resource_mut::() + .stroke + .take() + .unwrap(); + let harness = OperatorInvariantHarness::capture(world); + + finish_terrain_stroke( + world, + stroke, + final_terrain.clone(), + "Raise Terrain", + "Raise", + ); + + harness.assert_committed(world, 1, 0); + harness.assert_status(world, "terrain.sculpt", OperatorPhase::Committed); + assert_eq!(world.get::(entity), Some(&final_terrain)); + let state = world.resource::(); + assert!(state.active); + assert_eq!(state.target, Some(entity)); + assert!(!state.is_stroking()); + assert_undo_redo_round_trip(world, original, final_terrain, |world| { + world.get::(entity).unwrap().clone() + }); + } + + #[test] + fn no_op_stroke_terminates_without_dirtying_or_history() { + let mut app = App::new(); + app.register_type::() + .register_type::(); + let world = app.world_mut(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let original = TerrainDesc::flat(5); + let entity = world + .spawn((LevelObject, ActorKind::Terrain, original.clone())) + .id(); + let harness = OperatorInvariantHarness::capture(world); + + finish_terrain_stroke( world, TerrainStroke { entity, original: original.clone(), last_local: Vec3::ZERO, flatten_height: 0.0, - seed: 7, + seed: 1, }, - final_terrain.clone(), - "Raise Terrain", - ) - .unwrap(); + original.clone(), + "Smooth Terrain", + "Smooth", + ); - assert_eq!(world.resource::().undo_depth(), 1); - assert_eq!(world.get::(entity), Some(&final_terrain)); - apply_command_undo(world); + harness.assert_canceled(world); + harness.assert_status(world, "terrain.sculpt", OperatorPhase::Canceled); assert_eq!(world.get::(entity), Some(&original)); - apply_command_redo(world); - assert_eq!(world.get::(entity), Some(&final_terrain)); + } + + #[test] + fn commit_failure_restores_preview_and_reports_blocked_without_history() { + let mut app = App::new(); + let world = app.world_mut(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let original = TerrainDesc::flat(5); + let mut preview = original.clone(); + apply_dab( + &mut preview, + Vec3::ZERO, + 3.0, + 0.8, + TerrainSculptMode::Raise, + 0.0, + 11, + ); + let entity = world + .spawn((LevelObject, ActorKind::Terrain, preview.clone())) + .id(); + let mut hierarchy = crate::ui::hierarchy_state::HierarchyPanelState::default(); + hierarchy.locked.insert(entity); + world.insert_resource(hierarchy); + let harness = OperatorInvariantHarness::capture(world); + + finish_terrain_stroke( + world, + TerrainStroke { + entity, + original: original.clone(), + last_local: Vec3::ZERO, + flatten_height: 0.0, + seed: 11, + }, + preview, + "Raise Terrain", + "Raise", + ); + + harness.assert_blocked(world); + harness.assert_status(world, "terrain.sculpt", OperatorPhase::Blocked); + assert_eq!(world.get::(entity), Some(&original)); + assert!(world + .resource::() + .status + .starts_with("Terrain sculpt commit failed:")); } #[test] @@ -703,11 +892,21 @@ mod tests { fn cancel_once( mut state: ResMut, mut terrains: Query<(&GlobalTransform, &mut TerrainDesc)>, + mut scene_io: ResMut, + mut active_operator: ResMut, ) { - cancel_stroke(&mut state, &mut terrains); + cancel_active_stroke( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + ); } let mut app = App::new(); + app.init_resource::() + .init_resource::() + .init_resource::(); let original = TerrainDesc::flat(5); let mut preview = original.clone(); apply_dab( @@ -721,7 +920,7 @@ mod tests { ); let entity = app .world_mut() - .spawn((GlobalTransform::default(), preview)) + .spawn((LevelObject, GlobalTransform::default(), preview)) .id(); let mut state = TerrainSculptState::default(); state.start(entity); @@ -733,9 +932,74 @@ mod tests { seed: 11, }); app.insert_resource(state).add_systems(Update, cancel_once); + let harness = OperatorInvariantHarness::capture(app.world_mut()); app.update(); + harness.assert_canceled(app.world_mut()); + harness.assert_status(app.world(), "terrain.sculpt", OperatorPhase::Canceled); assert_eq!(app.world().get::(entity), Some(&original)); - assert!(!app.world().resource::().is_stroking()); + let state = app.world().resource::(); + assert!(state.active); + assert_eq!(state.target, Some(entity)); + assert!(!state.is_stroking()); + } + + #[test] + fn clean_view_interruption_restores_stroke_and_terminates_tool() { + fn interrupt_once( + mut state: ResMut, + mut terrains: Query<(&GlobalTransform, &mut TerrainDesc)>, + mut scene_io: ResMut, + mut active_operator: ResMut, + ) { + stop_tool_with_rollback( + &mut state, + &mut terrains, + &mut scene_io, + &mut active_operator, + OperatorPhase::Canceled, + "Terrain sculpt stopped for clean game view", + "Clean game view enabled; terrain restored", + ); + } + + let mut app = App::new(); + app.init_resource::() + .init_resource::() + .init_resource::(); + let original = TerrainDesc::flat(5); + let mut preview = original.clone(); + apply_dab( + &mut preview, + Vec3::ZERO, + 3.0, + 0.8, + TerrainSculptMode::Raise, + 0.0, + 11, + ); + let entity = app + .world_mut() + .spawn((LevelObject, GlobalTransform::default(), preview)) + .id(); + let mut state = TerrainSculptState::default(); + state.start(entity); + state.stroke = Some(TerrainStroke { + entity, + original: original.clone(), + last_local: Vec3::ZERO, + flatten_height: 0.0, + seed: 11, + }); + app.insert_resource(state) + .add_systems(Update, interrupt_once); + let harness = OperatorInvariantHarness::capture(app.world_mut()); + + app.update(); + + harness.assert_canceled(app.world_mut()); + harness.assert_status(app.world(), "terrain.sculpt", OperatorPhase::Canceled); + assert_eq!(app.world().get::(entity), Some(&original)); + assert!(!app.world().resource::().active); } } diff --git a/docs/README.md b/docs/README.md index dc34c21..ad59d4a 100644 --- a/docs/README.md +++ b/docs/README.md @@ -91,6 +91,7 @@ Immutable-style log of significant decisions. Add a new numbered ADR when changi | [editor/evaluations/physics-placement/](editor/evaluations/physics-placement/) | Live screenshot and acceptance results for physics settling, cancel, commit, undo, and redo | | [editor/evaluations/collider-diagnostics/](editor/evaluations/collider-diagnostics/) | Live screenshot and acceptance results for collider overlays, diagnostics, shape history, and placement preflight | | [editor/evaluations/navigation-authoring/](editor/evaluations/navigation-authoring/) | Live screenshots and acceptance results for navigation authoring and runtime parity | +| [editor/evaluations/operator-invariants/](editor/evaluations/operator-invariants/) | Source acceptance results for production operator lifecycle, rollback, cleanup, and undo/redo invariants | | [editor/evaluations/production-readiness/](editor/evaluations/production-readiness/) | Current versioned production gate, evidence matrix, candidate commands, soak protocol, and independent sign-off requirements | ## Working plans (not canonical long-term) @@ -114,6 +115,7 @@ Detailed milestone and feature plans live in [`.cursor/plans/`](../.cursor/plans | `production_readiness_acceptance_*.plan.md` | Release-candidate evidence matrix, blocker sequence, clean-checkout checks, soak, budgets, and independent sign-off | | `material_library_and_targeted_drop_*.plan.md` | Dedicated Material Library, exact viewport slot/primitive/brush targeting, hover preview, cancel, and grouped history | | `terrain_material_layers_*.plan.md` | Terrain shared-material layers, normalized weights, blended hydration, and modal painting | +| `operator_invariants_completion_*.plan.md` | Production operator dispatch, interruption, rollback, cleanup, and undo/redo acceptance | ## Crate responsibilities (quick reference) diff --git a/docs/editor/README.md b/docs/editor/README.md index b310bc7..8bbc626 100644 --- a/docs/editor/README.md +++ b/docs/editor/README.md @@ -39,6 +39,7 @@ Docs for the in-process egui editor (`crates/editor/`). Update this index when a | [evaluations/physics-placement/](evaluations/physics-placement/) | Live screenshot and verification record for multi-prop settling, cancel, commit, undo, and redo | | [evaluations/collider-diagnostics/](evaluations/collider-diagnostics/) | Live screenshot and verification record for semantic collider overlays, health linking, shape switching, and placement reuse | | [evaluations/navigation-authoring/](evaluations/navigation-authoring/) | Live screenshots and verification record for navigation authoring and runtime parity | +| [evaluations/operator-invariants/](evaluations/operator-invariants/) | Source acceptance record for production operator lifecycle, rollback, cleanup, and undo/redo invariants | | [evaluations/production-readiness/](evaluations/production-readiness/) | Current #50 release-candidate matrix and required clean-checkout, soak, performance, limitation, and independent-signoff evidence | ## Subsystems (code → doc) diff --git a/docs/editor/architecture.md b/docs/editor/architecture.md index 5c2b73f..10d50fc 100644 --- a/docs/editor/architecture.md +++ b/docs/editor/architecture.md @@ -245,13 +245,16 @@ the Edit-to-Play boundary restore the complete runtime snapshot. See Structural edits (spawn, delete, transform, rename) go through `EditorHistory` command objects in `history.rs`. User-facing actions route through the operator lifecycle in `operators.rs`: begin, preview, -commit, cancel, disabled reason, and status text. The first bridge wraps command-palette and queued -editor commands as immediate operators, while existing `EditorHistory` commands remain the undo -payload. Asset placement, mesh-subasset placement, texture apply, and material apply actions now -use immediate operators from `assets::operators`; texture/material changes across a selection use -one `SetMaterialGroup` command. Draw Brush, brush CSG, clip, and element gizmo paths publish the -same preview/commit/cancel phases. `operators/test_harness.rs` verifies authored deltas, dirty -state, helper cleanup, undo grouping, and undo/redo restoration; see +commit, cancel, disabled reason, stable ID, and status text. Registered commands return typed +`OperatorAction::Commit` or `OperatorAction::ContinuePreview`, so immediate commands finish while +Draw Brush and CSG retain modal Preview ownership; validation failures terminate instead of leaving +stale status. Existing `EditorHistory` commands remain the undo payload. Group Selection is one +atomic command that remaps its transient group identity on every redo without reparenting unrelated +siblings. Lighting reset and Project Sun changes use one grouped light command. Asset placement, +assignment, exact material drops, brush tools, terrain strokes, physics placement, and the real +transform tracker all publish terminal lifecycle state. `operators/test_harness.rs` verifies semantic +projections, dirty-state preservation, stable IDs, helper cleanup, undo grouping, repeated undo/redo, +and automatic interruption rollback; see [operator-regression-testing.md](operator-regression-testing.md). PIE stop restores player simulation state only; authored `LevelObject` edits made during PIE remain in the scene. diff --git a/docs/editor/debt-audit.md b/docs/editor/debt-audit.md index 2eb0634..7c7c842 100644 --- a/docs/editor/debt-audit.md +++ b/docs/editor/debt-audit.md @@ -10,6 +10,7 @@ Living checklist for the production editor program ([ADR 0012](../adr/0012-zero- | Monolithic `hydrate_authoring_entities` | Done | `crates/shared/src/hydration/*` modules | | Optional / inferred `ActorKind` as long-term save path | Done | Schema v2 + migration; save validates | | Hydrated components in committed `.scn.ron` | CI | `scene::validate_scene_authoring_only` + `repo_editor_scene_has_no_hydrated_components` | +| Production mutation without commit/cancel/failure/undo invariants | Done | Shared operator harness plus typed history projections cover palette, asset, brush, terrain, physics, grouping, lighting, material-drop, and transform paths; see [evaluation](evaluations/operator-invariants/) | | Dual FBX thumbnail ad-hoc path (parallel to unified pipeline) | Partial | Phase 5 `assets/thumbnails/` refactor | | `failed_keys` thumbnail cache without retry API | Partial | `asset_thumbnails.rs`; Phase 5 `ThumbnailState` | @@ -21,7 +22,7 @@ Living checklist for the production editor program ([ADR 0012](../adr/0012-zero- | P1 | `ActorKind` required; `validate_actor` on save | | P2 | No runtime components in default inspector | | P3 | `HierarchySiblingIndex`; no undefined sibling order | -| P6 | `ActorInspectorSection` registry; command palette commands; BRP authoring policy | +| P6 | `ActorInspectorSection` registry; typed immediate/modal command dispatch; BRP authoring policy; production mutation invariants | | P7 | ADRs 0009–0012 indexed; CI `shared` + `scene` + `editor` checks | ## Manual verification (release) @@ -29,3 +30,4 @@ Living checklist for the production editor program ([ADR 0012](../adr/0012-zero- - [ ] R1–R6 rendering/inspector scenarios ([roadmap.md](roadmap.md)) - [ ] Save/load roundtrip: hierarchy, lights, materials unchanged - [ ] PIE 60s stop: project lighting profile unchanged unless scene overrides edited +- [ ] Rerun the [operator invariant matrix](operator-regression-testing.md) from the nominated release-candidate commit diff --git a/docs/editor/evaluations/operator-invariants/README.md b/docs/editor/evaluations/operator-invariants/README.md new file mode 100644 index 0000000..d6c7871 --- /dev/null +++ b/docs/editor/evaluations/operator-invariants/README.md @@ -0,0 +1,45 @@ +# Operator Invariants Evaluation + +Date: 2026-07-12 + +Gitea issue: +[`#33`](https://git.spacetrainclubhouse.com/Falling-Metal-Interactive/Blacksite/issues/33) + +This acceptance record covers the source-level production operator contract. It does not nominate a +release candidate or replace the clean-checkout rerun required by production-readiness gate G3. + +## Accepted Coverage + +- Registered commands distinguish immediate completion from modal Preview ownership; validation + failures terminate with their stable command ID. +- Group Selection is one history transaction and preserves unrelated hierarchy rows through repeated + undo/redo even though the transient group entity is respawned. +- Reset Lighting and Project Sun changes are grouped, undoable light transactions. +- Asset/sub-asset placement; material/texture/audio/animation assignment; and exact viewport material + drops cover commit, block/failure, cancel where applicable, and semantic undo/redo projections. +- Draw Brush, CSG, clip, and element-gizmo paths exercise production modal finalizers and helper + cleanup. +- Terrain sculpt/paint cover multi-dab commit, failure rollback, cancel, clean-view interruption, and + resource cleanup. +- Physics placement covers prerequisite block, exact cancel, real settle commit, multi-selection + grouping, helper cleanup, undo, and redo. +- The transform tracker covers continuous multi-target grouping and no-op release termination. + +## Verification + +The publication worktree passed: + +- `cargo test -p editor --lib --no-fail-fast`: 245 passed, 0 failed. +- `cargo clippy -p editor --lib --tests -- -D warnings`. +- `cargo fmt --all -- --check`. +- `cargo test --workspace --all-targets --no-fail-fast`. +- `cargo clippy --workspace --all-targets -- -D warnings`. +- `cargo validate-levels --project .`: 62 dependencies, 5 expected non-blocking findings, 0 blocking + errors. + +The Gitea closure comment records these results with the exact publication commit. + +No screenshot is required for this ticket: the accepted surface is lifecycle, rollback, cleanup, and +history behavior exercised through headless production entry points. Live command/status checks add +confidence but are not substituted for semantic assertions. Packaged-runtime testing remains +deferred by project-owner request. diff --git a/docs/editor/evaluations/production-readiness/README.md b/docs/editor/evaluations/production-readiness/README.md index 2a2be69..4e77a9f 100644 --- a/docs/editor/evaluations/production-readiness/README.md +++ b/docs/editor/evaluations/production-readiness/README.md @@ -32,12 +32,12 @@ another commit, a dirty worktree, or an older package do not transfer to the can |----|-------------|-------|--------------------------| | G1 | Project create/open/resume, scene authoring, autosave/recovery, hierarchy, prefab, and asset integrity pass | Partial | Project/recovery/session/multi-scene/prefab implementations are documented in [project launcher](../../project-launcher.md), [session recovery](../../session-recovery.md), [multi-scene composition](../../multi-scene-composition.md), and [prefab authoring](../../prefab-authoring.md). Collaborative file safety `#49` and non-blocking native dialogs `#52` passed live acceptance. Candidate-specific end-to-end reruns remain. | | G2 | Brush, material, terrain, physics placement, animation, audio, navigation, PIE, and build/package samples pass | Fail | Animation, audio, navigation, renderer/material foundation `#51`, Material Library, targeted material drops, terrain `#22`-`#24`, physics placement `#25`, and build foundations are implemented with source/live fixtures. Brush acceptance `#37` remains incomplete. Packaged testing is owner-deferred. | -| G3 | Undo/redo/cancel invariants and helper cleanup cover every production mutation path | Partial | The reusable harness and current tool coverage are documented in [operator regression testing](../../operator-regression-testing.md). Terrain stroke and physics settle/cancel/undo fixtures now exist; Gitea `#33` remains open for full mutation-path coverage. | +| G3 | Undo/redo/cancel invariants and helper cleanup cover every production mutation path | Partial | Source implementation and focused evidence are complete in [operator regression testing](../../operator-regression-testing.md) and the [operator-invariants evaluation](../operator-invariants/); Gitea `#33` is ready to close. A clean, exact-candidate rerun is still required for `Pass`. | | G4 | Representative project completes an eight-hour soak without unbounded memory/target growth or unrecoverable failure | Missing | No candidate soak log, resource timeline, failure ledger, or target-growth measurement exists. | | G5 | Cold start, scene open/save, asset refresh, common manipulation, and package-build budgets are documented and measured | Missing | Gitea `#34` is open; no ratified budgets or candidate measurement record exists. | | G6 | Headless content validation and CI are green from a clean checkout | Missing | [CI configuration](../../../../.github/workflows/ci.yml) exists and local source/headless checks have passed during feature work, but no clean-checkout candidate run is linked. The current Gitea server does not expose an Actions run endpoint for this repository. | | G7 | First-hour UX and recovery QA are signed off by someone other than the implementer | External | No independent sign-off exists. Gitea `#36` remains open; historical H1-H6 implementation-pass notes do not count. | -| G8 | Known limitations have severity/workaround and no P0 blocker remains | Fail | Limitations are distributed across feature docs rather than one candidate ledger. Open P0 work includes `#32` and `#33`; `#50` cannot close while a gate-relevant P0 remains. Renderer foundation `#51`, roadmap audit `#35`, Material Library `#16`, targeted drops `#18`, collaborative safety `#49`, and dialog responsiveness `#52` are closed. Property-block application `#53` is P1; dynamic deformed Solari geometry `#54` is a documented P2 limitation with Forward/raster fallback. | +| G8 | Known limitations have severity/workaround and no P0 blocker remains | Fail | Limitations are distributed across feature docs rather than one candidate ledger. The remaining open P0 implementation blocker is `#32`; `#50` cannot close while it remains. Operator invariants `#33`, renderer foundation `#51`, roadmap audit `#35`, Material Library `#16`, targeted drops `#18`, collaborative safety `#49`, and dialog responsiveness `#52` are complete. Property-block application `#53` is P1; dynamic deformed Solari geometry `#54` is a documented P2 limitation with Forward/raster fallback. | ## Deliverables diff --git a/docs/editor/operator-regression-testing.md b/docs/editor/operator-regression-testing.md index 600f352..7b88c6e 100644 --- a/docs/editor/operator-regression-testing.md +++ b/docs/editor/operator-regression-testing.md @@ -11,11 +11,13 @@ Every production operator test must cover the paths it exposes: | Path | Required proof | |------|----------------| -| Commit | Expected authored entity delta, exact undo-group delta, dirty state, committed phase | -| Cancel or commit failure | No authored entity delta, no new history, prior dirty state, canceled phase | +| Commit | Expected authored projection, exact undo-group delta, dirty state, stable operator ID, committed phase | +| Zero-history commit | Authored projection and prior dirty state remain unchanged | +| Cancel or commit failure | Exact authored projection restored, no new history, prior dirty state, stable operator ID, terminal canceled/blocked phase | | Blocked start | Commit closure never runs; authored state, history, and dirty state are unchanged | +| Preview failure | Cancel finalizer runs, authored state is restored, and helpers are removed | | Preview helpers | No helper component remains after commit or cancel | -| Undoable commit | `assert_undo_redo_round_trip` restores both initial and committed projections | +| Undoable commit | `assert_undo_redo_round_trip` restores both initial and committed semantic projections | | Continuous edit | The entire interaction creates one history command, not one per frame or target | The harness intentionally fails with the invariant name in the assertion message. Tests may add @@ -26,12 +28,12 @@ domain assertions, but should not replace these shared lifecycle checks. 1. Build a minimal `World` with `ActiveOperator`, `EditorHistory`, and `SceneIo`. Add tool-specific selection/resources and `SelectedEntity` when history helpers update selection. 2. Capture `OperatorInvariantHarness` immediately before starting the operation. -3. Run the real operator entry point or the same production commit/cancel function used by its - input system. +3. Run the real dispatch path or production finalizer used by the input system. Do not duplicate its + state transition in the test. 4. Call `assert_committed`, `assert_canceled`, or `assert_blocked` with the expected authored and - undo deltas. + undo deltas, then `assert_status` with the stable operator ID and terminal phase. 5. For helpers, call `assert_no_helpers::`. For undoable work, project the meaningful - state through `assert_undo_redo_round_trip`. + state through `assert_undo_redo_round_trip`; use `assert_projection_unchanged` for no-op paths. Do not satisfy a lifecycle test by constructing an `EditorCommand` and checking only its label. The test must mutate a world and prove restoration. @@ -40,15 +42,17 @@ The test must mutate a world and prove restoration. | Workflow | Coverage | |----------|----------| -| Generic `EditorOperator` | Commit cleanup, commit failure rollback, blocked no-op | -| Asset placement | Commit, dirty state, single undo, authored spawn round trip | -| Sub-asset placement | Missing dependency commit failure leaves no side effects | -| Texture/material assignment | Multi-actor grouped undo; missing material failure; empty-selection block | -| Draw Brush | Full modal-state cancel; decomposed multi-brush grouped commit and round trip | -| Brush CSG | Pending-preview cancel; merge commit/deletion grouped round trip | -| Brush clip and element gizmo | Clip grouped geometry round trip; Escape rollback and helper cleanup | -| Transform gizmo finalization | Multi-target continuous edit grouped into one undo command | +| Generic `EditorOperator` | Commit cleanup, preview/commit failure rollback, blocked no-op, terminal ID/phase | +| Palette and registered commands | Typed immediate commit versus modal preview ownership; failed CSG start terminates | +| Selection and scene commands | Atomic Group Selection across repeated undo/redo; grouped Reset Lighting and Project Sun history | +| Asset workflows | Asset/sub-asset placement; material/texture group assignment; audio/animation assignment and incompatible targets | +| Viewport material drop | Exact renderer slot/primitive/brush-face preview, cancel, commit, cleanup, undo, and redo | +| Draw Brush and CSG | Modal cancel; decomposed commit; CSG validation/read-only block, cancel, grouped commit, and deleted-brush restoration | +| Brush clip and element gizmo | Production finalizers, geometry round trip, Escape rollback, and helper cleanup | +| Terrain sculpt and paint | Multi-dab grouped commit, no-op release, commit failure rollback, cancel, clean-view interruption, and resource cleanup | +| Physics placement | Prerequisite block, no-op commit, exact cancel, real settle commit, multi-selection grouping, helper cleanup, undo, and redo | +| Transform gizmo finalization | Actual tracker finalizer, continuous multi-target grouping, no-op release, and missing-primary survivor rollback | -Terrain strokes and physics-settle sessions are roadmap tools, not current production operators. -Their implementation tickets must add harness fixtures before those tools can pass their own exit -gates; Gitea issue BS-JD-502 remains the umbrella until that coverage exists. +The shared harness covers formal operators and multi-frame modal paths. Atomic inspector and history +mutations that do not own `ActiveOperator` use focused typed history tests with equivalent semantic +projection and undo/redo assertions. diff --git a/docs/editor/roadmap.md b/docs/editor/roadmap.md index 828fcb4..545f9ab 100644 --- a/docs/editor/roadmap.md +++ b/docs/editor/roadmap.md @@ -134,6 +134,7 @@ Phased work for the in-process editor (`crates/editor`). Status reflects the cur | `ActorInspectorSection` registry | Done | `ext/extensibility.rs` + `game_inspector.rs` | | Command registry | Done | Play, lighting reset, group, focus | | Command palette (Ctrl+P) | Done | Executes registered commands | +| Typed operator dispatch and mutation invariants | Done | Immediate commands commit, modal commands retain Preview ownership, and production asset/brush/terrain/physics/history paths pass the shared lifecycle and undo contract ([testing guide](operator-regression-testing.md), [Gitea #33](https://git.spacetrainclubhouse.com/Falling-Metal-Interactive/Blacksite/issues/33)) | | `EditorPlugin` trait | Done | `register_editor_plugin`; dogfood panel from `game::editor_ext` | | Undo: `SetActorKind`, add/remove component | Done | `SetActorKind` + `AddComponent` / `RemoveComponent` | | BRP authoring-only policy | Done | [brp.md](brp.md) |