Modifiers producing Become no longer include modifiers after themse…

…lves.

This fixes the core problem of <#522>:
if we want modifiers in general to be able to modify a block’s actions,
then they must always build on top of the actions they are given by the
*previous* modifier.

There probably need to be further changes, but I’ll deal with those as
they come up.

all-is-cubes/src/block/modifier/composite.rs

@@ -522,26 +522,6 @@ impl CompositeOperator {
             mem::swap(&mut source, &mut destination);
         }
 
-        // TODO: We haven't got *consistent* semantics for how modifiers interact with actions,
-        // which is demonstrated when `Composite` and `Move` are stacked.
-        //
-        // This breaks the `move_inside_composite_destination()` test in particular,
-        // because there are two potential semantics for modifiers acting on ops:
-        //
-        // 1. modifiers modify the ops to stack on their effects, and
-        // 2. modifiers are usually inert; the originally provided op applies to
-        //    the whole block,
-        //
-        // and `Move` kind of assumed 2 while `Composite` with the below code would be
-        // (necessarily?) doing 1 in order to incorporate the source block.
-        //
-        // Therefore, the rest of this code is stubbed out because it ends up doubling up
-        // modifiers, which would be potentially very bad, and leaves only the wrong behavior
-        // of ignoring the source’s operation.
-        if true {
-            return destination.cloned();
-        }
-
         // For now, `Become` is the only supported operation.
         // TODO: We should have a warning-reporting path so that this can be debugged when it fails.
         fn require_become(op: Option<&Operation>) -> Option<&Block> {
@@ -590,13 +570,6 @@ impl CompositeOperator {
                 reverse: ctx.was_reversed,
                 disassemblable: ctx.disassemblable,
             }));
-        // Include all modifiers stacked on the original block *following* this Composite modifier.
-        // TODO: This is probably not fully coherent and needs to take into account some of the later modifiers’ semantics.
-        new_block.modifiers_mut().extend(
-            ctx.block.modifiers()[(this_modifier_index + 1)..]
-                .iter()
-                .cloned(),
-        );
 
         Some(Operation::Become(new_block))
     }
@@ -1054,7 +1027,6 @@ mod tests {
         }
 
         #[test]
-        #[ignore = "TODO: implement operation merge to make this pass"]
         fn activation_action_is_composed() {
             let [result1, result2] = make_some_blocks();
             let b1 = &Block::builder()
@@ -1077,7 +1049,6 @@ mod tests {
         }
 
         #[test]
-        #[ignore = "TODO: implement operation merge to make this pass"]
         fn tick_action_is_composed() {
             let [result1, result2] = make_some_blocks();
             let b1 = &Block::builder()

all-is-cubes/src/block/modifier/mod.rs

@@ -107,6 +107,10 @@ impl Modifier {
     ///
     /// * `block` is the original block value (modifiers do not alter it).
     /// * `this_modifier_index` is the index in `block.modifiers()` of `self`.
+    ///   In cases where the modifier needs to produce another block value,
+    ///   such as generating a [`Modifier::Become`], all modifiers following
+    ///   `this_modifier_index` should be omitted.
+    ///   TODO: Make it easier to comply.
     /// * `value` is the output of the preceding modifier or primitive, which is what the
     ///   current modifier should be applied to.
     /// * `filter` controls evaluation options and listening, and its budget is decremented by

all-is-cubes/src/block/modifier/move.rs

@@ -143,15 +143,24 @@ impl Move {
                 matches!(&block.modifiers()[this_modifier_index], Modifier::Move(m) if m == self)
             );
             let mut new_block = block.clone();
-            if let Modifier::Move(Move {
-                distance, velocity, ..
-            }) = &mut new_block.modifiers_mut()[this_modifier_index]
             {
-                *distance = i32::from(*distance)
+                let modifiers = new_block.modifiers_mut();
+
+                // Update the distance for the next step.
+                if let Modifier::Move(Move {
+                    distance, velocity, ..
+                }) = &mut modifiers[this_modifier_index]
+                {
+                    *distance = i32::from(*distance)
                             .saturating_add(i32::from(*velocity))
                             .clamp(0, i32::from(u16::MAX))
                             .try_into()
                             .unwrap(/* clamped to range */);
+                }
+
+                // Do not include any other modifiers.
+                // The modifiers themselves are responsible for doing so.
+                modifiers.truncate(this_modifier_index + 1);
             }
             Some(Operation::Become(new_block))
         } else {
@@ -413,7 +422,6 @@ mod tests {
     /// Check the behavior of a `Move` modifier under a `Rotate` modifier.
     /// In particular, we want to make sure the outcome doesn’t end up doubly-rotated.
     #[test]
-    #[ignore = "TODO: modifier evaluation needs fixing"]
     fn move_inside_rotation() {
         let [base] = make_some_blocks();
         const R: Modifier = Modifier::Rotate(GridRotation::CLOCKWISE);
@@ -477,8 +485,6 @@ mod tests {
     }
 
     /// Test [`Move`] acting within the `source` position of a [`Modifier::Composite`].
-    ///
-    /// TODO: This is not yet implemented, but should be.
     #[test]
     fn move_inside_composite_source() {
         let [base, extra] = make_some_blocks();
@@ -496,22 +502,16 @@ mod tests {
         let expected_after_tick = extra.clone().with_modifier(Composite::new(
             base.clone().with_modifier(Move {
                 direction: Face6::PX,
-                distance: 10,
+                distance: 20,
                 velocity: 10,
                 schedule: time::Schedule::EVERY_TICK,
             }),
             block::CompositeOperator::Over,
         ));
 
-        if false {
-            // This is what we want to happen
-            assert_eq!(
-                block.evaluate().unwrap().attributes.tick_action,
-                Some(TickAction::from(Operation::Become(expected_after_tick)))
-            );
-        } else {
-            // Placeholder to fail if the current behavior changes
-            assert_eq!(block.evaluate().unwrap().attributes.tick_action, None);
-        }
+        assert_eq!(
+            block.evaluate().unwrap().attributes.tick_action,
+            Some(TickAction::from(Operation::Become(expected_after_tick)))
+        );
     }
 }