cubing.js icon indicating copy to clipboard operation
cubing.js copied to clipboard

Roux-style slice moves not supported by bluetooth input

Open anicolao opened this issue 3 years ago • 15 comments

To Reproduce Steps to reproduce the behavior:

  1. Use alpha.twizzle.net's editor with a GAN bluetooth cube as the input method.
  2. Do an 'M' slice move
  3. The recorded and visualized move is L' R or R L' depending on whether you did an M or M'

Expected behavior

An M move should be chosen and visualized.

Difficulty

Unfortunately it isn't really easy to fix this. Here is a hacky fix that makes it easy to see further problems:

diff --git a/src/cubing/bluetooth/smart-puzzle/gan.ts b/src/cubing/bluetooth/smart-puzzle/gan.ts
index 8e9d8eda..3dfb0772 100644
--- a/src/cubing/bluetooth/smart-puzzle/gan.ts
+++ b/src/cubing/bluetooth/smart-puzzle/gan.ts
@@ -30,6 +30,13 @@ const ganMoveToBlockMove: { [i: number]: Move } = {
   0x0e: new Move("L", -1),
   0x0f: new Move("B"),
   0x11: new Move("B", -1),
+  // fake moves for slices
+  0x0b00: new Move("E"),
+  0x0209: new Move("E", -1),
+  0x030e: new Move("M"),
+  0x050c: new Move("M", -1),
+  0x0611: new Move("S", -1),
+  0x080f: new Move("S"),
 };
 
 let homeQuatInverse: Quaternion | null = null;
@@ -130,8 +137,24 @@ class PhysicalState {
     if (n < 0 || n > MAX_LATEST_MOVES) {
       throw new Error(`Must ask for 0 to 6 latest moves. (Asked for ${n})`);
     }
-    return Array.from(this.arr.slice(19 - n, 19)).map(
-      (i) => ganMoveToBlockMove[i],
+    let rawGanMoves = this.arr.slice(19 - n, 19);
+    let moves = [];
+		let i = 1;
+		for (i = 1; i < rawGanMoves.length; i++) {
+			if (ganMoveToBlockMove[rawGanMoves[i-1]<<8 | rawGanMoves[i]]) {
+				moves.push(rawGanMoves[i-1]<<8 | rawGanMoves[i]);
+				i++;
+			} else if (ganMoveToBlockMove[rawGanMoves[i]<<8 | rawGanMoves[i-1]]) {
+				moves.push(rawGanMoves[i]<<8 | rawGanMoves[i-1]);
+				i++;
+			} else {
+				moves.push(rawGanMoves[i-1]);
+			}
+		}
+		if (i == rawGanMoves.length) {
+			moves.push(rawGanMoves[rawGanMoves.length-1]);
+		}
+    return moves.map((i) => ganMoveToBlockMove[i],
     );
   }

With this patch applied, the first slice move will be fine. But then the centers will be in different spots, and so M' U will turn into M B because the blue face will be on top after the M move, but the mapping from gan's encoding to the player is wrong, since the player is displaying blue on top but the blue face twist is mapped to 'B' by the fixed orientation in the ganMoveToBlockMove array.

This problem also occurs in the existing implementation, but it's less confusing because the cube is visually in the wrong orientation after the L' R and so when the wrong face turns when you move the U face you quickly notice that it is at least the face of the correct colour that is moving.

So the bluetooth code needs to know which face of the player is oriented up to match it with the face of the cube that is oriented up, ideally...

anicolao avatar Aug 18 '22 00:08 anicolao

Thanks for filing, this is something I really want to dig into, but have been setting aside for exactly the challenges you detailed. I don't have time to lay out a full plan, but if someone wants to tackle this, I think the best approach would be:

  • Add "rough orientation" tracker to the bluetooth puzzle classes that keeps track of what's on U&F (or equivalent).
  • "Subtract" that "rough orientation" from the "detailed orientation", on each orientation update.
  • When the "rough orientation" reaches a threshold, update the "rough rotation correspondingly", send a puzzle rotation x together with an orientation update based on the new "rough rotation".
  • Use smoothing, debouncing, and drift heuristics to minimize false positives for orientations.

An intermediate solution might be to detect moves like "L R' x'" specifically, and not rotations in general. However, I think that would result in more counterintuitive results than full orientation tracking.

Eventually, we need a way to stream puzzle move + orientation events with "rewrites", e.g. "remove the last move and replace it with this" (e.g. "replace that R'" with "L R'" and then "replace that L R' with M'"), but I think that can be worked on essentially independently

lgarron avatar Aug 18 '22 00:08 lgarron

I am not sure how compatible the way I am thinking about it is with the plan you have in mind. Let's see if they match up in a nice way.

Fundamentally, there are two different things the user might want to control: the view of the cube, and the state of the cube. The state of the cube unfortunately includes its orientation in 3-space, because the notation is designed to be agnostic to the colours on the faces but the user wants the colours to match up; and so we have notation that alters the orientation (x, y, z, and their inverses). So sometimes, when the user rotates the cube, they actually mean to rotate the orientation in their notation, and all future moves are relative to the new orientation; and other times, they do not.

So let's just talk orientation; if the gyro vector for the bluetooth cube clearly points a different face up, we need to generate an x or z move. This I think corresponds well to your idea of a "rough orientation" tracker that knows what's on U/F; each time the threshold for this "rough orientation" tracker is met, we generate the corresponding x, y, z moves. I propose for example that if the face is within a small number of degrees of on that should trigger it. I don't know yet what data we get from the cube but in a normal implementation we'd have a gyro sensor that gives us a "down" vector, and so for example we could say that if the "down" vector is within 5 degrees of an axis then that axis is vertical and use that to trigger an orientation move. Outside of these 6 "dots" of sensitivity in 3-space, the cube will not get an orientation move.

If an orientation move happens, the ganMoveToBlockMove mapping will be wrong, and either a different solution is needed or the mapping must be recomputed.

Let's say the user does an M slice move. This is equivalent to L R' x and so in that case the ganMoveToBlockMove will need updating. In my mind there is no need to detect M slice moves later; if I slowly do L R' x, the view, animation, and playback should reflect that I did it slowly. This means there is no need to rewrite moves 'after the fact'; if the user wants that they can hit simplify and have those sequences rewritten as slice moves, but that is a totally separate part of the system that isn't needed for bluetooth input. Since the bluetooth cube code already has an 150ms threshold for grouping moves together, that becomes the natural threshold for turning L R' x -> M; if you're not turning fast enough for that you didn't "really" do an M slice move.

Now in my mind fine orientation adjustments could either be ignored or exposed as view altering moves. So if I peek at the bottom face physically, the viewer could reflect that by adjusting the camera angle as I rotate the cube (or rotating the cube, if that's how rotation works; I suspect it works by moving the camera). Whether the code is really set up for this isn't clear to me; it seems likely to me that the design of the 'streaming' of moves from the various input devices supported probably only considered moves that change the state/alg on display and not view changes. However if I'm mistaken and there is a place to send the fine orientation to that can then choose to use that input to alter the view that the user has accordingly, that would be lovely.

How close is this to what you're thinking? If it's close enough I will put up a fork with patches.

anicolao avatar Aug 18 '22 13:08 anicolao

Here's an ugly patch that does most of what I want. Probably this needs to be rethought.

diff --git a/src/cubing/bluetooth/smart-puzzle/gan.ts b/src/cubing/bluetooth/smart-puzzle/gan.ts
index 8e9d8eda..d774eb67 100644
--- a/src/cubing/bluetooth/smart-puzzle/gan.ts
+++ b/src/cubing/bluetooth/smart-puzzle/gan.ts
@@ -13,25 +13,163 @@ import { debugLog } from "../debug";
 import { BluetoothConfig, BluetoothPuzzle } from "./bluetooth-puzzle";
 
 // This needs to be short enough to capture 6 moves (OBQTM).
-const DEFAULT_INTERVAL_MS = 150;
+const DEFAULT_INTERVAL_MS = 250;
 // Number of latest moves provided by the Gan 356i.
 const MAX_LATEST_MOVES = 6;
 
+const orientationMap: { [k: string]: string } = {
+	"U": "U",
+	"D": "D",
+	"F": "F",
+	"L": "L",
+	"B": "B",
+	"R": "R",
+	"E": "E",
+	"M": "M",
+	"S": "S",
+	"E'": "E'",
+	"M'": "M'",
+	"S'": "S'",
+}
 const ganMoveToBlockMove: { [i: number]: Move } = {
-  0x00: new Move("U"),
-  0x02: new Move("U", -1),
-  0x03: new Move("R"),
-  0x05: new Move("R", -1),
-  0x06: new Move("F"),
-  0x08: new Move("F", -1),
-  0x09: new Move("D"),
-  0x0b: new Move("D", -1),
-  0x0c: new Move("L"),
-  0x0e: new Move("L", -1),
-  0x0f: new Move("B"),
-  0x11: new Move("B", -1),
 };
 
+function makeGanMoveToBlockMove() {
+  ganMoveToBlockMove[0x00] = new Move(orientationMap["U"]);
+  ganMoveToBlockMove[0x02] = new Move(orientationMap["U"], -1);
+  ganMoveToBlockMove[0x03] = new Move(orientationMap["R"]);
+  ganMoveToBlockMove[0x05] = new Move(orientationMap["R"], -1);
+  ganMoveToBlockMove[0x06] = new Move(orientationMap["F"]);
+  ganMoveToBlockMove[0x08] = new Move(orientationMap["F"], -1);
+  ganMoveToBlockMove[0x09] = new Move(orientationMap["D"]);
+  ganMoveToBlockMove[0x0b] = new Move(orientationMap["D"], -1);
+  ganMoveToBlockMove[0x0c] = new Move(orientationMap["L"]);
+  ganMoveToBlockMove[0x0e] = new Move(orientationMap["L"], -1);
+  ganMoveToBlockMove[0x0f] = new Move(orientationMap["B"]);
+  ganMoveToBlockMove[0x11] = new Move(orientationMap["B"], -1);
+  // fake moves for slices
+  ganMoveToBlockMove[0x0b00] = new Move(orientationMap["E"]);
+  ganMoveToBlockMove[0x0209] = new Move(orientationMap["E'"]);
+  ganMoveToBlockMove[0x030e] = new Move(orientationMap["M"]);
+  ganMoveToBlockMove[0x050c] = new Move(orientationMap["M'"]);
+  ganMoveToBlockMove[0x0611] = new Move(orientationMap["S'"]);
+  ganMoveToBlockMove[0x080f] = new Move(orientationMap["S"]);
+}
+makeGanMoveToBlockMove();
+
+function eTransform() {
+	// F -> R, R -> B, B -> L, L -> F
+	let oldF = orientationMap["F"];
+	orientationMap["F"] = orientationMap["R"];
+	orientationMap["R"] = orientationMap["B"];
+	orientationMap["B"] = orientationMap["L"];
+	orientationMap["L"] = oldF;
+	// M -> S, S -> M', M' -> S', S' -> M
+	let oldM = orientationMap["M"]
+	orientationMap["M"] = orientationMap["S"];
+	orientationMap["S"] = orientationMap["M'"];
+	orientationMap["M'"] = orientationMap["S'"];
+	orientationMap["S'"] = oldM;
+	makeGanMoveToBlockMove();
+	return orientationMap["E"].length == 1 ? 0x0b00 : 0x0209;
+}
+
+function ePrimeTransform() {
+	// F <- R, R <- B, B <- L, L <- F
+	let oldF = orientationMap["F"];
+	orientationMap["F"] = orientationMap["L"];
+	orientationMap["L"] = orientationMap["B"];
+	orientationMap["B"] = orientationMap["R"];
+	orientationMap["R"] = oldF;
+	// M <- S, S <- M', M' <- S', S' <- M
+	let oldM = orientationMap["M"]
+	orientationMap["M"] = orientationMap["S'"];
+	orientationMap["S'"] = orientationMap["M'"];
+	orientationMap["M'"] = orientationMap["S"];
+	orientationMap["S"] = oldM;
+	makeGanMoveToBlockMove();
+	return 0x0209;
+}
+
+function mTransform() {
+	// U -> F, F -> D, D -> B, B -> U
+	let oldU = orientationMap["U"];
+	orientationMap["U"] = orientationMap["F"];
+	orientationMap["F"] = orientationMap["D"];
+	orientationMap["D"] = orientationMap["B"];
+	orientationMap["B"] = oldU;
+	// E' -> S, S -> E, E -> S', S' -> E'
+	let oldE = orientationMap["E'"]
+	orientationMap["E'"] = orientationMap["S"];
+	orientationMap["S"] = orientationMap["E"];
+	orientationMap["E"] = orientationMap["S'"];
+	orientationMap["S'"] = oldE;
+	makeGanMoveToBlockMove();
+	return 0x030e;
+}
+
+function mPrimeTransform() {
+	// U <- F, F <- D, D <- B, B <- U
+	let oldU = orientationMap["U"];
+	orientationMap["U"] = orientationMap["B"];
+	orientationMap["B"] = orientationMap["D"];
+	orientationMap["D"] = orientationMap["F"];
+	orientationMap["F"] = oldU;
+	// E' <- S, S <- E, E <- S', S' <- E'
+	let oldE = orientationMap["E'"]
+	orientationMap["E'"] = orientationMap["S'"];
+	orientationMap["S'"] = orientationMap["E"];
+	orientationMap["E"] = orientationMap["S"];
+	orientationMap["S"] = oldE;
+	makeGanMoveToBlockMove();
+	return 0x050c;
+}
+
+function sTransform() {
+	// U <- L, L <- D, D <- R, R <- U
+	let oldU = orientationMap["U"];
+	orientationMap["U"] = orientationMap["R"];
+	orientationMap["R"] = orientationMap["D"];
+	orientationMap["D"] = orientationMap["L"];
+	orientationMap["L"] = oldU;
+	// M -> E', E' -> M', M' -> E, E -> M
+	let oldM = orientationMap["M"]
+	orientationMap["M"] = orientationMap["E'"];
+	orientationMap["E'"] = orientationMap["M'"];
+	orientationMap["M'"] = orientationMap["E"];
+	orientationMap["E"] = oldM;
+	makeGanMoveToBlockMove();
+	return 0x080f;
+}
+
+function sPrimeTransform() {
+	// U -> L, L -> D, D -> R, R -> U
+	let oldU = orientationMap["U"];
+	orientationMap["U"] = orientationMap["L"];
+	orientationMap["L"] = orientationMap["D"];
+	orientationMap["D"] = orientationMap["R"];
+	orientationMap["R"] = oldU;
+	// M <- E', E' <- M', M' <- E, E <- M
+	let oldM = orientationMap["M"]
+	orientationMap["M"] = orientationMap["E"];
+	orientationMap["E"] = orientationMap["M'"];
+	orientationMap["M'"] = orientationMap["E'"];
+	orientationMap["E'"] = oldM;
+	makeGanMoveToBlockMove();
+	return 0x0611;
+}
+
+
+const sliceTransforms: { [i: number]: any } = {
+  0x0b00: eTransform,
+  0x0209: ePrimeTransform,
+  0x030e: mTransform,
+  0x050c: mPrimeTransform,
+  0x0611: sPrimeTransform,
+  0x080f: sTransform,
+}
+
+
 let homeQuatInverse: Quaternion | null = null;
 
 function probablyDecodedCorrectly(data: Uint8Array): boolean {
@@ -130,8 +268,26 @@ class PhysicalState {
     if (n < 0 || n > MAX_LATEST_MOVES) {
       throw new Error(`Must ask for 0 to 6 latest moves. (Asked for ${n})`);
     }
-    return Array.from(this.arr.slice(19 - n, 19)).map(
-      (i) => ganMoveToBlockMove[i],
+    let rawGanMoves = this.arr.slice(19 - n, 19);
+    let moves = [];
+		let i = 1;
+		for (i = 1; i < rawGanMoves.length; i++) {
+			let forwardKey = rawGanMoves[i-1]<<8 | rawGanMoves[i];
+			let backwardKey = rawGanMoves[i]<<8 | rawGanMoves[i-1];
+			let sliceTransform = sliceTransforms[forwardKey] || sliceTransforms[backwardKey];
+			if (sliceTransform) {
+				let moveCode = sliceTransform();
+				moveCode = sliceTransforms[forwardKey] ? forwardKey : backwardKey;
+				moves.push(moveCode);
+				i++;
+			} else {
+				moves.push(rawGanMoves[i-1]);
+			}
+		}
+		if (i == rawGanMoves.length) {
+			moves.push(rawGanMoves[rawGanMoves.length-1]);
+		}
+    return moves.map((i) => ganMoveToBlockMove[i],
     );
   }

anicolao avatar Aug 18 '22 19:08 anicolao

In trying to implement wide moves (r, u, etc) I discovered that the hacky approach is really quite the wrong way to go about it, and I will replace ganMoveToBlockMove with a map from GAN encoding to "WG" orientation faces (i.e. the white face will be U, green will be F, and so on in the map) and then use the same code as is already in src/cubing/stream/process/ReorientedStream.ts to map that to the correct face.

However ReorientedStream itself looks like more than I really need for the minimal patch, and the string "ULFRBD" already occurs in multiple places in the source code. Should I write the minimal patch for the GAN cube code, or should I refactor it to re-use ReorientedStream to do the job (which it is already fully capable of doing, but is not necessary to make the GAN code work properly...)?

anicolao avatar Aug 19 '22 22:08 anicolao

Here is what I consider the 'minimal' patch:

diff --git a/src/cubing/bluetooth/smart-puzzle/gan.ts b/src/cubing/bluetooth/smart-puzzle/gan.ts
index 8e9d8eda..85b29163 100644
--- a/src/cubing/bluetooth/smart-puzzle/gan.ts
+++ b/src/cubing/bluetooth/smart-puzzle/gan.ts
@@ -351,8 +351,13 @@ export class GanCube extends BluetoothPuzzle {
       );
       numInterveningMoves = MAX_LATEST_MOVES;
     }
-    for (const move of physicalState.latestMoves(numInterveningMoves)) {
-      // console.log(move);
+    for (const originalMove of physicalState.latestMoves(numInterveningMoves)) {
+      // console.log(originalMove);
+     const faces = "ULFRBD";
+     const faceIdx = faces.indexOf(originalMove.family);
+     const stateData = this.state.stateData;
+     const family = faces[stateData["CENTERS"].pieces.indexOf(faceIdx)];
+     const move = originalMove.modified({ family });
       this.state = this.state.applyMove(move);
       this.dispatchAlgLeaf({
         latestAlgLeaf: move,

anicolao avatar Aug 19 '22 23:08 anicolao

And here is a better patch based on that for adding all the slice moves. Orientation is coming but will take longer to write up as it took me a while to figure out how to best generate the quaternions and the code is currently a mess.

diff --git a/src/cubing/bluetooth/smart-puzzle/gan.ts b/src/cubing/bluetooth/smart-puzzle/gan.ts
index 85b29163..29c46cec 100644
--- a/src/cubing/bluetooth/smart-puzzle/gan.ts
+++ b/src/cubing/bluetooth/smart-puzzle/gan.ts
@@ -13,7 +13,7 @@ import { debugLog } from "../debug";
 import { BluetoothConfig, BluetoothPuzzle } from "./bluetooth-puzzle";
 
 // This needs to be short enough to capture 6 moves (OBQTM).
-const DEFAULT_INTERVAL_MS = 150;
+const DEFAULT_INTERVAL_MS = 250;
 // Number of latest moves provided by the Gan 356i.
 const MAX_LATEST_MOVES = 6;
 
@@ -30,6 +30,13 @@ const ganMoveToBlockMove: { [i: number]: Move } = {
   0x0e: new Move("L", -1),
   0x0f: new Move("B"),
   0x11: new Move("B", -1),
+  // fake moves for slices
+  0x0b00: new Move("E"),
+  0x0209: new Move("E'"),
+  0x030e: new Move("M"),
+  0x050c: new Move("M'"),
+  0x0611: new Move("S'"),
+  0x080f: new Move("S"),
 };
 
 let homeQuatInverse: Quaternion | null = null;
@@ -130,8 +137,25 @@ class PhysicalState {
     if (n < 0 || n > MAX_LATEST_MOVES) {
       throw new Error(`Must ask for 0 to 6 latest moves. (Asked for ${n})`);
     }
-    return Array.from(this.arr.slice(19 - n, 19)).map(
-      (i) => ganMoveToBlockMove[i],
+    let rawGanMoves = this.arr.slice(19 - n, 19);
+    let moves = [];
+		let i = 1;
+		for (i = 1; i < rawGanMoves.length; i++) {
+			let forwardKey = rawGanMoves[i-1]<<8 | rawGanMoves[i];
+			let backwardKey = rawGanMoves[i]<<8 | rawGanMoves[i-1];
+			let sliceMove = ganMoveToBlockMove[forwardKey] || ganMoveToBlockMove[backwardKey];
+			if (sliceMove) {
+				let moveCode = ganMoveToBlockMove[forwardKey] ? forwardKey : backwardKey;
+				moves.push(moveCode);
+				i++;
+			} else {
+				moves.push(rawGanMoves[i-1]);
+			}
+		}
+		if (i == rawGanMoves.length) {
+			moves.push(rawGanMoves[rawGanMoves.length-1]);
+		}
+    return moves.map((i) => ganMoveToBlockMove[i],
     );
   }
 

anicolao avatar Aug 19 '22 23:08 anicolao

Heh, you keep making it better! I don't see a reason to stop now . . .

On Fri, Aug 19, 2022 at 4:15 PM anicolao @.***> wrote:

And here is a better patch based on that for adding all the slice moves. Orientation is coming but will take longer to write up as it took me a while to figure out how to best generate the quaternions and the code is currently a mess.

diff --git a/src/cubing/bluetooth/smart-puzzle/gan.ts b/src/cubing/bluetooth/smart-puzzle/gan.ts index 85b29163..29c46cec 100644 --- a/src/cubing/bluetooth/smart-puzzle/gan.ts +++ b/src/cubing/bluetooth/smart-puzzle/gan.ts @@ -13,7 +13,7 @@ import { debugLog } from "../debug"; import { BluetoothConfig, BluetoothPuzzle } from "./bluetooth-puzzle";

// This needs to be short enough to capture 6 moves (OBQTM). -const DEFAULT_INTERVAL_MS = 150; +const DEFAULT_INTERVAL_MS = 250; // Number of latest moves provided by the Gan 356i. const MAX_LATEST_MOVES = 6;

@@ -30,6 +30,13 @@ const ganMoveToBlockMove: { [i: number]: Move } = { 0x0e: new Move("L", -1), 0x0f: new Move("B"), 0x11: new Move("B", -1),

  • // fake moves for slices
  • 0x0b00: new Move("E"),
  • 0x0209: new Move("E'"),
  • 0x030e: new Move("M"),
  • 0x050c: new Move("M'"),
  • 0x0611: new Move("S'"),
  • 0x080f: new Move("S"), };

let homeQuatInverse: Quaternion | null = null; @@ -130,8 +137,25 @@ class PhysicalState { if (n < 0 || n > MAX_LATEST_MOVES) { throw new Error(Must ask for 0 to 6 latest moves. (Asked for ${n})); }

  • return Array.from(this.arr.slice(19 - n, 19)).map(
  •  (i) => ganMoveToBlockMove[i],
    
  • let rawGanMoves = this.arr.slice(19 - n, 19);
  • let moves = [];
  • let i = 1;
    
  • for (i = 1; i < rawGanMoves.length; i++) {
    
  • 	let forwardKey = rawGanMoves[i-1]<<8 | rawGanMoves[i];
    
  • 	let backwardKey = rawGanMoves[i]<<8 | rawGanMoves[i-1];
    
  • 	let sliceMove = ganMoveToBlockMove[forwardKey] || ganMoveToBlockMove[backwardKey];
    
  • 	if (sliceMove) {
    
  • 		let moveCode = ganMoveToBlockMove[forwardKey] ? forwardKey : backwardKey;
    
  • 		moves.push(moveCode);
    
  • 		i++;
    
  • 	} else {
    
  • 		moves.push(rawGanMoves[i-1]);
    
  • 	}
    
  • }
    
  • if (i == rawGanMoves.length) {
    
  • 	moves.push(rawGanMoves[rawGanMoves.length-1]);
    
  • }
    
  • return moves.map((i) => ganMoveToBlockMove[i], ); }

— Reply to this email directly, view it on GitHub https://github.com/cubing/cubing.js/issues/201#issuecomment-1221163369, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOLS4PGERK4XMTLTRX37TV2AIRVANCNFSM563LPDKA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

rokicki avatar Aug 20 '22 13:08 rokicki

OK after a day of futzing with it I have come to the conclusion that the way to keep the bluetooth layer robust and simple is to have it issue what it knows: face moves and orientation changes. Slice moves will have to be handled upstream, by translating L R' x -> M', for example, or L x -> r for wide moves.

For orientation tracking, this first pass uses almost a full π/2 dot on each axis to do orientation changes, and the effect is pretty satisfactory. Some orientation changes do get missed and there's probably cause to periodically reset the 'home' quaternion and everything based upon it so that if the user is slowly shifting their position in an absolute sense it doesn't make the rotations hard to get to.

The end result is not too complicated, does a good job of knowing the facing of the cube at any given time (tracked as colour up + colour front - "WG" being the assumed facing when the user connects the cube).

Tomorrow I'll look for the right place to add in the translation to better notation (ie. wide and slice moves) and clues would be appreciated.

Here is the whole thing as one patch:

diff --git a/src/cubing/bluetooth/debug.ts b/src/cubing/bluetooth/debug.ts
index 57a1792e..6fd34fa8 100644
--- a/src/cubing/bluetooth/debug.ts
+++ b/src/cubing/bluetooth/debug.ts
@@ -1,4 +1,4 @@
-let DEBUG_LOGGING_ENABLED = false;
+let DEBUG_LOGGING_ENABLED = true;
 
 export function enableDebugLogging(enable: boolean): void {
   DEBUG_LOGGING_ENABLED = enable;
diff --git a/src/cubing/bluetooth/smart-puzzle/gan.ts b/src/cubing/bluetooth/smart-puzzle/gan.ts
index 8e9d8eda..28876c10 100644
--- a/src/cubing/bluetooth/smart-puzzle/gan.ts
+++ b/src/cubing/bluetooth/smart-puzzle/gan.ts
@@ -1,6 +1,6 @@
 /* tslint:disable no-bitwise */
 
-import { Quaternion } from "three";
+import { Vector3, Quaternion } from "three";
 import { Move } from "../../alg";
 import type { KPuzzle, KStateData } from "../../kpuzzle";
 import { KState } from "../../kpuzzle";
@@ -31,6 +31,8 @@ const ganMoveToBlockMove: { [i: number]: Move } = {
   0x0f: new Move("B"),
   0x11: new Move("B", -1),
 };
+const facings: [ string ] = [];
+const facingToRotationMove: { [k: string]: Move } = {};
 
 let homeQuatInverse: Quaternion | null = null;
 
@@ -126,13 +128,16 @@ class PhysicalState {
   // Due to the design of the Gan356i protocol, it's common to query for the
   // latest physical state and find 0 moves have been performed since the last
   // query. Therefore, it's useful to allow 0 as an argument.
-  public latestMoves(n: number): Move[] {
+  public latestMoves(n: number, rotation: string): Move[] {
     if (n < 0 || n > MAX_LATEST_MOVES) {
       throw new Error(`Must ask for 0 to 6 latest moves. (Asked for ${n})`);
     }
-    return Array.from(this.arr.slice(19 - n, 19)).map(
-      (i) => ganMoveToBlockMove[i],
-    );
+		let moves = Array.from(this.arr.slice(19 - n, 19)).map((i) => ganMoveToBlockMove[i]);
+		const rotationMove = facingToRotationMove[rotation];
+		if (rotationMove) {
+			moves.push(rotationMove);
+		}
+    return moves;
   }
 
   public debugInfo(): { arr: Uint8Array } {
@@ -289,6 +294,84 @@ export class GanCube extends BluetoothPuzzle {
   }
 
   public INTERVAL_MS: number = DEFAULT_INTERVAL_MS;
+  public facing = "WG";
+
+	private quaternionToOrientationMap: [{q: Quaternion, facing: string}] = [];
+	private kpuzzleToFacing = function(state: KState) {
+		const colours = "WOGRBY";
+		let centers = state.stateData["CENTERS"].pieces;
+		const axisToIndex = { "x": 3, "y": 0, "z": 2 };
+		const topIndex = centers[axisToIndex["y"]];
+		const frontIndex = centers[axisToIndex["z"]];
+		const topFace = colours[topIndex];
+		const frontFace = colours[frontIndex];
+		return topFace + frontFace;
+	}
+	private initQuaternionToOrientationMap = function(kpuzzle: KPuzzle) {
+		let WGOrientation = new Quaternion(0, 0, 0, 1);
+		let zMove = new Quaternion();
+		let yMove = new Quaternion();
+		let xMove = new Quaternion();
+		zMove.setFromAxisAngle(new Vector3(1, 0, 0), Math.PI/2);
+		yMove.setFromAxisAngle(new Vector3(0, 1, 0), -Math.PI/2);
+		xMove.setFromAxisAngle(new Vector3(0, 0, 1), -Math.PI/2);
+		// put some dead space in so that the orientation doesn't
+		// flip back and forth due to sensor noise
+		const threshold = Math.PI/4 - 0.15;
+		let facingStates: { string: KState } = {};
+		let currentOrientation = WGOrientation;
+		let state: KState = kpuzzle.startState();
+		let centers = state.stateData["CENTERS"].pieces;
+		const rotations = [ yMove, xMove.clone().invert(), zMove, 
+			xMove, zMove.clone().invert(), yMove.clone().invert() ]
+		function rotateCube(axis: string) {
+			const axisToIndex = { "x": 3, "y": 0, "z": 2 };
+			let move = rotations[centers[axisToIndex[axis]]];
+			currentOrientation = currentOrientation.clone().multiply(move);
+			state = state.applyMove(axis);
+			centers = state.stateData["CENTERS"].pieces;
+		}
+		for (let zxRotation = 0; zxRotation < 6; ++zxRotation) {
+			if (zxRotation > 0 && zxRotation < 4) {
+				rotateCube("z");
+			} else if (zxRotation == 4) {
+				rotateCube("z");
+				rotateCube("x");
+			} else if (zxRotation == 5) {
+				rotateCube("x");
+				rotateCube("x");
+			}
+			for (let yRotation = 0; yRotation < 4; ++yRotation) {
+				if (yRotation > 0) {
+					rotateCube("y");
+				}
+				const currentFacing = this.kpuzzleToFacing(state);
+				facings.push(currentFacing);
+				facingStates[currentFacing] = state;
+				this.quaternionToOrientationMap.push({
+					q: currentOrientation,
+					facing: currentFacing
+				});
+			}
+			rotateCube("y");
+		}
+		// For every facing, generate all the cube rotations from that
+		// facing that we want to recognize.
+		const recognizableCubeRotations = [
+			new Move("x"), new Move("x", -1), new Move("x2"),
+			new Move("y"), new Move("y", -1), new Move("y2"),
+			new Move("z"), new Move("z", -1)
+		];
+		facings.map(startFacing => {
+			recognizableCubeRotations.map(move => {
+				let endState = facingStates[startFacing].applyMove(move);
+				let endFacing = this.kpuzzleToFacing(endState);
+				let key = `${endFacing}<${startFacing}`;
+				facingToRotationMove[key] = move;
+			});
+		});
+	}
+
   private intervalHandle: number | null = null;
   private state: KState;
   private cachedFaceletStatus1Characteristic: Promise<BluetoothRemoteGATTCharacteristic>;
@@ -308,6 +391,7 @@ export class GanCube extends BluetoothPuzzle {
     super();
     this.state = kpuzzle.startState();
     this.startTrackingMoves();
+    this.initQuaternionToOrientationMap(kpuzzle);
   }
 
   public name(): string | undefined {
@@ -351,8 +435,27 @@ export class GanCube extends BluetoothPuzzle {
       );
       numInterveningMoves = MAX_LATEST_MOVES;
     }
-    for (const move of physicalState.latestMoves(numInterveningMoves)) {
-      // console.log(move);
+		const oldFacing = this.kpuzzleToFacing(this.state);
+    const threshold = Math.PI/4 - 0.15;
+		for (let i = 0; i < this.quaternionToOrientationMap.length; ++i) {
+			let facingAngle = this.quaternionToOrientationMap[i].q;
+			let faces = this.quaternionToOrientationMap[i].facing;
+			let offset = physicalState.rotQuat().angleTo(facingAngle);
+			if (Math.abs(offset) < threshold) {
+				if (faces !== this.facing) {
+					this.facing = faces;
+					debugLog(`Rotated to ${this.facing} from ${oldFacing}`);
+				}
+			}
+		}
+		const rotation = this.facing + "<" + oldFacing;
+    for (const originalMove of physicalState.latestMoves(numInterveningMoves, rotation)) {
+      // console.log(originalMove);
+			const faces = "ULFRBD";
+			const faceIdx = faces.indexOf(originalMove.family);
+			const stateData = this.state.stateData;
+			const family = faces[stateData["CENTERS"].pieces.indexOf(faceIdx)];
+			const move = originalMove.modified({ family });
       this.state = this.state.applyMove(move);
       this.dispatchAlgLeaf({
         latestAlgLeaf: move,

anicolao avatar Aug 21 '22 04:08 anicolao

I haven't figured out how testing is meant to be done for this project, but meanwhile here is a version of the patch with all lint and typescript errors fixed.

diff --git a/src/cubing/bluetooth/debug.ts b/src/cubing/bluetooth/debug.ts
index 57a1792e..6fd34fa8 100644
--- a/src/cubing/bluetooth/debug.ts
+++ b/src/cubing/bluetooth/debug.ts
@@ -1,4 +1,4 @@
-let DEBUG_LOGGING_ENABLED = false;
+let DEBUG_LOGGING_ENABLED = true;
 
 export function enableDebugLogging(enable: boolean): void {
   DEBUG_LOGGING_ENABLED = enable;
diff --git a/src/cubing/bluetooth/smart-puzzle/gan.ts b/src/cubing/bluetooth/smart-puzzle/gan.ts
index 8e9d8eda..117c64c3 100644
--- a/src/cubing/bluetooth/smart-puzzle/gan.ts
+++ b/src/cubing/bluetooth/smart-puzzle/gan.ts
@@ -1,6 +1,6 @@
 /* tslint:disable no-bitwise */
 
-import { Quaternion } from "three";
+import { Vector3, Quaternion } from "three";
 import { Move } from "../../alg";
 import type { KPuzzle, KStateData } from "../../kpuzzle";
 import { KState } from "../../kpuzzle";
@@ -31,6 +31,8 @@ const ganMoveToBlockMove: { [i: number]: Move } = {
   0x0f: new Move("B"),
   0x11: new Move("B", -1),
 };
+const facings: string[] = [];
+const facingToRotationMove: { [k: string]: Move } = {};
 
 let homeQuatInverse: Quaternion | null = null;
 
@@ -126,13 +128,16 @@ class PhysicalState {
   // Due to the design of the Gan356i protocol, it's common to query for the
   // latest physical state and find 0 moves have been performed since the last
   // query. Therefore, it's useful to allow 0 as an argument.
-  public latestMoves(n: number): Move[] {
+  public latestMoves(n: number, rotation: string): Move[] {
     if (n < 0 || n > MAX_LATEST_MOVES) {
       throw new Error(`Must ask for 0 to 6 latest moves. (Asked for ${n})`);
     }
-    return Array.from(this.arr.slice(19 - n, 19)).map(
-      (i) => ganMoveToBlockMove[i],
-    );
+		const moves = Array.from(this.arr.slice(19 - n, 19)).map((i) => ganMoveToBlockMove[i]);
+		const rotationMove = facingToRotationMove[rotation];
+		if (rotationMove) {
+			moves.push(rotationMove);
+		}
+    return moves;
   }
 
   public debugInfo(): { arr: Uint8Array } {
@@ -289,6 +294,81 @@ export class GanCube extends BluetoothPuzzle {
   }
 
   public INTERVAL_MS: number = DEFAULT_INTERVAL_MS;
+  public facing = "WG";
+
+	private quaternionToOrientationMap: {q: Quaternion, facing: string}[] = [];
+	private kpuzzleToFacing = function(state: KState) {
+		const colours = "WOGRBY";
+		const centers = state.stateData["CENTERS"].pieces;
+		const axisToIndex = { "x": 3, "y": 0, "z": 2 };
+		const topIndex = centers[axisToIndex["y"]];
+		const frontIndex = centers[axisToIndex["z"]];
+		const topFace = colours[topIndex];
+		const frontFace = colours[frontIndex];
+		return topFace + frontFace;
+	}
+	private initQuaternionToOrientationMap = function(kpuzzle: KPuzzle) {
+		const WGOrientation = new Quaternion(0, 0, 0, 1);
+		const zMove = new Quaternion();
+		const yMove = new Quaternion();
+		const xMove = new Quaternion();
+		zMove.setFromAxisAngle(new Vector3(1, 0, 0), Math.PI/2);
+		yMove.setFromAxisAngle(new Vector3(0, 1, 0), -Math.PI/2);
+		xMove.setFromAxisAngle(new Vector3(0, 0, 1), -Math.PI/2);
+		const facingStates: { [key: string]: KState } = {};
+		let currentOrientation = WGOrientation;
+		let state: KState = kpuzzle.startState();
+		let centers = state.stateData["CENTERS"].pieces;
+		const rotations = [ yMove, xMove.clone().invert(), zMove, 
+			xMove, zMove.clone().invert(), yMove.clone().invert() ]
+		function rotateCube(axis: string) {
+			const axisToIndex: { [key: string]: number } = { "x": 3, "y": 0, "z": 2 };
+			const move = rotations[centers[axisToIndex[axis]]];
+			currentOrientation = currentOrientation.clone().multiply(move);
+			state = state.applyMove(axis);
+			centers = state.stateData["CENTERS"].pieces;
+		}
+		for (let zxRotation = 0; zxRotation < 6; ++zxRotation) {
+			if (zxRotation > 0 && zxRotation < 4) {
+				rotateCube("z");
+			} else if (zxRotation == 4) {
+				rotateCube("z");
+				rotateCube("x");
+			} else if (zxRotation == 5) {
+				rotateCube("x");
+				rotateCube("x");
+			}
+			for (let yRotation = 0; yRotation < 4; ++yRotation) {
+				if (yRotation > 0) {
+					rotateCube("y");
+				}
+				const currentFacing = this.kpuzzleToFacing(state);
+				facings.push(currentFacing);
+				facingStates[currentFacing] = state;
+				this.quaternionToOrientationMap.push({
+					q: currentOrientation,
+					facing: currentFacing
+				});
+			}
+			rotateCube("y");
+		}
+		// For every facing, generate all the cube rotations from that
+		// facing that we want to recognize.
+		const recognizableCubeRotations = [
+			new Move("x"), new Move("x", -1), new Move("x2"),
+			new Move("y"), new Move("y", -1), new Move("y2"),
+			new Move("z"), new Move("z", -1)
+		];
+		facings.map(startFacing => {
+			recognizableCubeRotations.map(move => {
+				const endState = facingStates[startFacing].applyMove(move);
+				const endFacing = this.kpuzzleToFacing(endState);
+				const key = `${endFacing}<${startFacing}`;
+				facingToRotationMove[key] = move;
+			});
+		});
+	}
+
   private intervalHandle: number | null = null;
   private state: KState;
   private cachedFaceletStatus1Characteristic: Promise<BluetoothRemoteGATTCharacteristic>;
@@ -308,6 +388,7 @@ export class GanCube extends BluetoothPuzzle {
     super();
     this.state = kpuzzle.startState();
     this.startTrackingMoves();
+    this.initQuaternionToOrientationMap(kpuzzle);
   }
 
   public name(): string | undefined {
@@ -351,8 +432,29 @@ export class GanCube extends BluetoothPuzzle {
       );
       numInterveningMoves = MAX_LATEST_MOVES;
     }
-    for (const move of physicalState.latestMoves(numInterveningMoves)) {
-      // console.log(move);
+		const oldFacing = this.kpuzzleToFacing(this.state);
+		// put some dead space in so that the orientation doesn't
+		// flip back and forth due to sensor noise
+    const threshold = Math.PI/4 - 0.15;
+		for (let i = 0; i < this.quaternionToOrientationMap.length; ++i) {
+			const facingAngle = this.quaternionToOrientationMap[i].q;
+			const faces = this.quaternionToOrientationMap[i].facing;
+			const offset = physicalState.rotQuat().angleTo(facingAngle);
+			if (Math.abs(offset) < threshold) {
+				if (faces !== this.facing) {
+					this.facing = faces;
+					debugLog(`Rotated to ${this.facing} from ${oldFacing}`);
+				}
+			}
+		}
+		const rotation = this.facing + "<" + oldFacing;
+    for (const originalMove of physicalState.latestMoves(numInterveningMoves, rotation)) {
+      // console.log(originalMove);
+			const faces = "ULFRBD";
+			const faceIdx = faces.indexOf(originalMove.family);
+			const stateData = this.state.stateData;
+			const family = faces[stateData["CENTERS"].pieces.indexOf(faceIdx)];
+			const move = originalMove.modified({ family });
       this.state = this.state.applyMove(move);
       this.dispatchAlgLeaf({
         latestAlgLeaf: move,

anicolao avatar Aug 21 '22 12:08 anicolao

I've started writing the slice move / wide move support and the best place for it seemed to be inside experimentalAddAlgLeaf similar to how coalesce is implemented now (besides which for cube timers, coalesce should be turned off and I think it'd be helpful for the bluetooth example to not coalesce moves).

I don't like how the testing is done and I stopped at that point; I want to redo it so that it does string comparisons so that when tests fail it is apparent what the failure was rather than just getting a 'true' vs 'false' failure. Also some helper methods to shorten up the tests would be nice in my opinion, something like

expectTransformation("L x", "r"); 

where there is just one expectTransformation that spells out the long boilerplate of actually calling the code under test. Would it be OK if I did it that way?

Thanks ... in progress patch below.

diff --git a/src/cubing/alg/operation.spec.ts b/src/cubing/alg/operation.spec.ts
index 15410d33..d71c7786 100644
--- a/src/cubing/alg/operation.spec.ts
+++ b/src/cubing/alg/operation.spec.ts
@@ -41,6 +41,34 @@ describe("operation", () => {
     ).to.be.true;
   });
 
+  it("can generate wide moves", () => {
+    expect(
+      experimentalAppendMove(new Alg("L"), new Move("x"), {
+        wideMoves: false,
+      }).isIdentical(new Alg("L x")),
+    ).to.be.true;
+    expect(
+      experimentalAppendMove(new Alg("L"), new Move("x"), {
+        wideMoves: true,
+      }).isIdentical(new Alg("r")),
+    ).to.be.true;
+    expect(
+      experimentalAppendMove(new Alg("L'"), new Move("x'"), {
+        wideMoves: true,
+      }).isIdentical(new Alg("r'")),
+    ).to.be.true;
+    expect(
+      experimentalAppendMove(new Alg("R'"), new Move("x"), {
+        wideMoves: true,
+      }).isIdentical(new Alg("l'")),
+    ).to.be.true;
+    expect(
+      experimentalAppendMove(new Alg("R"), new Move("x'"), {
+        wideMoves: true,
+      }).isIdentical(new Alg("l")),
+    ).to.be.true;
+  });
+
   it("can concat algs", () => {
     expect(
       new Alg("R U2").concat(new Alg("F' D")).isIdentical(new Alg("R U2 F' D")),
diff --git a/src/cubing/alg/operation.ts b/src/cubing/alg/operation.ts
index fa5e117c..8f995d73 100644
--- a/src/cubing/alg/operation.ts
+++ b/src/cubing/alg/operation.ts
@@ -6,19 +6,68 @@ export function experimentalAppendMove(
   newMove: Move,
   options?: {
     coalesce?: boolean; // defaults to false
+    wideMoves?: boolean; // defaults to false
+    sliceMoves?: boolean; // defaults to false
     mod?: number;
   },
 ): Alg {
   const oldAlgNodes = Array.from(alg.childAlgNodes());
-  const oldLastMove = oldAlgNodes[oldAlgNodes.length - 1] as Move | undefined;
+  const lastMove = oldAlgNodes[oldAlgNodes.length - 1] as Move | undefined;
+  const preLastMove = oldAlgNodes[oldAlgNodes.length - 2] as Move | undefined;
+  if (options?.sliceMoves && 
+    "xyz".indexOf(newMove.family) !== -1 &&
+    lastMove &&
+    lastMove.quantum &&
+    preLastMove &&
+    preLastMove.quantum
+  ) {
+    const index = "xyz".indexOf(newMove.family);
+    const forwardMove = "LDB"[index];
+    const inverseMove = "RUF"[index];
+    const newAlgNodes = oldAlgNodes.slice(0, oldAlgNodes.length - 2);
+    const lastFamily = lastMove.family;
+    const preLastFamily = preLastMove.family;
+    if ((forwardMove === lastFamily && inverseMove === preLastFamily) ||
+        (inverseMove === lastFamily && forwardMove === preLastFamily)) {
+      if (Math.abs(lastMove.amount) === Math.abs(newMove.amount)) {
+        const family = "MES"[index];
+        const amount = family === "E" ? -newMove.amount : newMove.amount;
+        newAlgNodes.push(newMove.modified({ family, amount }));
+        return new Alg(newAlgNodes);
+      }
+    }
+  } 
+  if (options?.wideMoves && 
+    "xyz".indexOf(newMove.family) !== -1 &&
+    lastMove &&
+    lastMove.quantum
+  ) {
+    const index = "xyz".indexOf(newMove.family);
+    const forwardMove = "LDB"[index];
+    const inverseMove = "RUF"[index];
+    const newAlgNodes = oldAlgNodes.slice(0, oldAlgNodes.length - 1);
+    const lastFamily = lastMove.family;
+    if (forwardMove === lastFamily || inverseMove === lastFamily) {
+      if (lastMove.amount === newMove.amount) {
+        const family = "ruf"[index];
+        newAlgNodes.push(lastMove.modified({ family }));
+        return new Alg(newAlgNodes);
+      }
+      if (lastMove.amount === -newMove.amount) {
+        const family = "ldb"[index];
+        newAlgNodes.push(lastMove.modified({ family }));
+        return new Alg(newAlgNodes);
+      }
+    }
+  } 
   if (
     options?.coalesce &&
-    oldLastMove &&
-    oldLastMove.quantum &&
-    oldLastMove.quantum.isIdentical(newMove.quantum)
+    lastMove &&
+    lastMove.quantum &&
+    lastMove.quantum.isIdentical(newMove.quantum)
   ) {
     const newAlgNodes = oldAlgNodes.slice(0, oldAlgNodes.length - 1);
-    let newAmount = oldLastMove.amount + newMove.amount;
+    let newAmount = lastMove.amount + newMove.amount;
     const mod = options?.mod;
     if (mod) {
       newAmount = ((newAmount % mod) + mod) % mod;
@@ -27,10 +76,9 @@ export function experimentalAppendMove(
       }
     }
     if (newAmount !== 0) {
-      newAlgNodes.push(oldLastMove.modified({ amount: newAmount }));
+      newAlgNodes.push(lastMove.modified({ amount: newAmount }));
     }
     return new Alg(newAlgNodes);
-  } else {
-    return new Alg([...oldAlgNodes, newMove]);
   }
+  return new Alg([...oldAlgNodes, newMove]);
 }
diff --git a/src/cubing/twisty/model/TwistyPlayerModel.ts b/src/cubing/twisty/model/TwistyPlayerModel.ts
index 5ddaecd0..aa4f98e8 100644
--- a/src/cubing/twisty/model/TwistyPlayerModel.ts
+++ b/src/cubing/twisty/model/TwistyPlayerModel.ts
@@ -225,7 +225,7 @@ export class TwistyPlayerModel {
 
   experimentalAddAlgLeaf(
     algLeaf: AlgLeaf,
-    options: { coalesce?: boolean; mod?: number } = {},
+    options: { coalesce?: boolean; sliceMoves?: boolean; wideMoves?: boolean; mod?: number } = {},
   ): void {
     const maybeMove = algLeaf.as(Move);
     if (maybeMove) {
@@ -245,17 +245,14 @@ export class TwistyPlayerModel {
   // TODO: Animate the new move.
   experimentalAddMove(
     flexibleMove: Move | string,
-    options: { coalesce?: boolean; mod?: number } = {},
+    options: { coalesce?: boolean; sliceMoves?: boolean; wideMoves?: boolean; mod?: number } = {},
   ): void {
     const move =
       typeof flexibleMove === "string" ? new Move(flexibleMove) : flexibleMove;
     this.alg.set(
       (async () => {
         const alg = (await this.alg.get()).alg;
-        const newAlg = experimentalAppendMove(alg, move, {
-          coalesce: options?.coalesce,
-          mod: options?.mod,
-        });
+        const newAlg = experimentalAppendMove(alg, move, options);
         this.timestampRequest.set("end");
         this.catchUpMove.set({
           move: move,
diff --git a/src/cubing/twisty/views/TwistyPlayer.ts b/src/cubing/twisty/views/TwistyPlayer.ts
index 4e38ddd4..76876259 100644
--- a/src/cubing/twisty/views/TwistyPlayer.ts
+++ b/src/cubing/twisty/views/TwistyPlayer.ts
@@ -409,7 +409,7 @@ export class TwistyPlayer
   // TODO: Animate the new move.
   experimentalAddMove(
     flexibleMove: Move | string,
-    options: { coalesce?: boolean } = {},
+    options: { coalesce?: boolean; wideMoves?: boolean; sliceMoves?: boolean; } = {},
   ): void {
     this.experimentalModel.experimentalAddMove(flexibleMove, options);
   }
@@ -417,7 +417,7 @@ export class TwistyPlayer
   // TODO: Animate the new move.
   experimentalAddAlgLeaf(
     algLeaf: AlgLeaf,
-    options: { coalesce?: boolean } = {},
+    options: { coalesce?: boolean; wideMoves?: boolean; sliceMoves?: boolean } = {},
   ): void {
     this.experimentalModel.experimentalAddAlgLeaf(algLeaf, options);
   }
diff --git a/src/sites/experiments.cubing.net/cubing.js/bluetooth/index.ts b/src/sites/experiments.cubing.net/cubing.js/bluetooth/index.ts
index 369acf5e..1f6255b7 100644
--- a/src/sites/experiments.cubing.net/cubing.js/bluetooth/index.ts
+++ b/src/sites/experiments.cubing.net/cubing.js/bluetooth/index.ts
@@ -51,7 +51,9 @@ window.addEventListener("DOMContentLoaded", async () => {
 
     puzzle.addAlgLeafListener((e: MoveEvent) => {
       twistyPlayer.experimentalAddAlgLeaf(e.latestAlgLeaf, {
-        coalesce: true,
+        coalesce: false,
+        wideMoves: true,
+        sliceMoves: true,
       });
     });
 
diff --git a/src/sites/experiments.cubing.net/cubing.js/play/input/SwipeyPuzzle.ts b/src/sites/experiments.cubing.net/cubing.js/play/input/SwipeyPuzzle.ts
index 391d3c4c..cc39d1d1 100644
--- a/src/sites/experiments.cubing.net/cubing.js/play/input/SwipeyPuzzle.ts
+++ b/src/sites/experiments.cubing.net/cubing.js/play/input/SwipeyPuzzle.ts
@@ -6,7 +6,7 @@ import {
   TwistyPlayer,
   TwistyPlayerConfig,
 } from "../../../../../cubing/twisty";
-import { coalesce, getSetup, PuzzleID } from "../url-params";
+import { coalesce, getSetup, PuzzleID, sliceMoves, wideMoves } from "../url-params";
 import { SwipeGrid, themes, ThemeType } from "./SwipeGrid";
 
 const DEFAULT_THEME: ThemeType = "transparent-grid";
@@ -192,6 +192,8 @@ export class SwipeyPuzzle extends HTMLElement {
       // TODO: allow`TwistyPlayer` to handle this directly.
       this.twistyPlayer.experimentalAddAlgLeaf(algLeaf, {
         coalesce: coalesce(),
+        wideMoves: wideMoves(),
+        sliceMoves: sliceMoves(),
       });
     } catch (e) {
       console.warn("Invalid alg leaf");
diff --git a/src/sites/experiments.cubing.net/cubing.js/play/url-params.ts b/src/sites/experiments.cubing.net/cubing.js/play/url-params.ts
index cc557504..a08dfc38 100644
--- a/src/sites/experiments.cubing.net/cubing.js/play/url-params.ts
+++ b/src/sites/experiments.cubing.net/cubing.js/play/url-params.ts
@@ -64,6 +64,24 @@ export function coalesce(): boolean {
   );
 }
 
+export function sliceMoves(): boolean {
+  return (
+    getURLParamChecked<"true" | "false">("sliceMoves", "true", [
+      "true",
+      "false",
+    ]) === "true"
+  );
+}
+
+export function wideMoves(): boolean {
+  return (
+    getURLParamChecked<"true" | "false">("wideMoves", "true", [
+      "true",
+      "false",
+    ]) === "true"
+  );
+}
+
 export function sendingSocketOrigin(): string | null {
   return new URL(document.location.href).searchParams.get(
     "sendingSocketOrigin",

anicolao avatar Aug 21 '22 19:08 anicolao

So I went ahead and wrote my tests the way I'd prefer. Will something like this be OK to land?

diff --git a/src/cubing/alg/operation.spec.ts b/src/cubing/alg/operation.spec.ts
index d71c7786..ced606d7 100644
--- a/src/cubing/alg/operation.spec.ts
+++ b/src/cubing/alg/operation.spec.ts
@@ -4,6 +4,15 @@ import { Alg } from "./Alg";
 import { experimentalAppendMove } from "./operation";
 import { Move } from "./alg-nodes";
 
+function testAppendMoveTransform({ start, move, result, options }) {
+  it(`experimentalAppendMove of ${start} + ${move} => ${result}`, () =>
+    expect(
+      experimentalAppendMove(
+      	new Alg(start), new Move(move), options).toString(),
+    ).to.equal(result)
+	);
+}
+
 describe("operation", () => {
   it("can append moves", () => {
     expect(
@@ -41,33 +50,53 @@ describe("operation", () => {
     ).to.be.true;
   });
 
-  it("can generate wide moves", () => {
-    expect(
-      experimentalAppendMove(new Alg("L"), new Move("x"), {
-        wideMoves: false,
-      }).isIdentical(new Alg("L x")),
-    ).to.be.true;
-    expect(
-      experimentalAppendMove(new Alg("L"), new Move("x"), {
-        wideMoves: true,
-      }).isIdentical(new Alg("r")),
-    ).to.be.true;
-    expect(
-      experimentalAppendMove(new Alg("L'"), new Move("x'"), {
-        wideMoves: true,
-      }).isIdentical(new Alg("r'")),
-    ).to.be.true;
-    expect(
-      experimentalAppendMove(new Alg("R'"), new Move("x"), {
-        wideMoves: true,
-      }).isIdentical(new Alg("l'")),
-    ).to.be.true;
-    expect(
-      experimentalAppendMove(new Alg("R"), new Move("x'"), {
-        wideMoves: true,
-      }).isIdentical(new Alg("l")),
-    ).to.be.true;
-  });
+	testAppendMoveTransform({
+		start: "L3", move: "L", result: "", 
+		options: { coalesce: true, mod: 4, }
+	});
+	testAppendMoveTransform({
+		start: "L3", move: "L3", result: "L2", 
+		options: { coalesce: true, mod: 4, }
+	});
+	testAppendMoveTransform({
+		start: "L3", move: "L6", result: "L", 
+		options: { coalesce: true, mod: 2, }
+	});
+
+	testAppendMoveTransform({
+		start: "L", move: "x", result: "L x", options: { wideMoves: false, }
+	});
+	testAppendMoveTransform({
+		start: "L", move: "x", result: "r", options: { wideMoves: true, }
+	});
+	testAppendMoveTransform({
+		start: "L'", move: "x'", result: "r'", options: { wideMoves: true, }
+	});
+	testAppendMoveTransform({
+		start: "R", move: "x'", result: "l", options: { wideMoves: true, }
+	});
+	testAppendMoveTransform({
+		start: "R", move: "x", result: "R x", options: { wideMoves: true, }
+	});
+	testAppendMoveTransform({
+		start: "R'", move: "x", result: "l'", options: { wideMoves: true, }
+	});
+	testAppendMoveTransform({
+		start: "R' R", move: "x", result: "R' R x",
+		options: { wideMoves: true }
+	});
+	testAppendMoveTransform({
+		start: "R' R", move: "x", result: "R' R x",
+		options: { wideMoves: true, sliceMoves: true }
+	});
+	testAppendMoveTransform({
+		start: "L' R", move: "x'", result: "M",
+		options: { wideMoves: true, sliceMoves: true }
+	});
+	testAppendMoveTransform({
+		start: "U' D", move: "y", result: "E'",
+		options: { wideMoves: true, sliceMoves: true }
+	});
 
   it("can concat algs", () => {
     expect(
diff --git a/src/cubing/alg/operation.ts b/src/cubing/alg/operation.ts
index 8f995d73..7183224f 100644
--- a/src/cubing/alg/operation.ts
+++ b/src/cubing/alg/operation.ts
@@ -31,7 +31,7 @@ export function experimentalAppendMove(
         (inverseMove === lastFamily && forwardMove === preLastFamily)) {
       if (Math.abs(lastMove.amount) === Math.abs(newMove.amount)) {
         const family = "MES"[index];
-        const amount = family === "E" ? -newMove.amount : newMove.amount;
+        const amount = family === "S" ? newMove.amount : -newMove.amount;
         newAlgNodes.push(newMove.modified({ family, amount }));
         return new Alg(newAlgNodes);
       }
@@ -45,9 +45,11 @@ export function experimentalAppendMove(
     const index = "xyz".indexOf(newMove.family);
     const forwardMove = "LDB"[index];
     const inverseMove = "RUF"[index];
+    const moveAlignment = newMove.amount * lastMove.amount;
+    const checkMove = moveAlignment > 0 ? forwardMove : inverseMove;
     const newAlgNodes = oldAlgNodes.slice(0, oldAlgNodes.length - 1);
     const lastFamily = lastMove.family;
-    if (forwardMove === lastFamily || inverseMove === lastFamily) {
+    if (checkMove === lastFamily) {
       if (lastMove.amount === newMove.amount) {
         const family = "ruf"[index];
         newAlgNodes.push(lastMove.modified({ family }));

anicolao avatar Aug 22 '22 14:08 anicolao

@rokicki thanks for the encouragement. @lgarron, @rokicki -- should I be putting up a fork with this work to get it upstreamed, or just maintaining it for myself? I need these fixes to build the site I want to build on top of cubing.js but it's not clear if this is moving sufficiently in the right direction for upstreaming or not. I don't need to bother with a fork and pull requests if I'm on the wrong path from your PoV, I can just maintain my patches in my own environment.

anicolao avatar Aug 23 '22 14:08 anicolao

Do a fork just so you are not blocked. I’m traveling and doing family stuff for another week. Thanks!

On Tue, Aug 23, 2022 at 9:22 AM anicolao @.***> wrote:

@rokicki https://github.com/rokicki thanks for the encouragement. @lgarron https://github.com/lgarron, @rokicki https://github.com/rokicki -- should I be putting up a fork with this work to get it upstreamed, or just maintaining it for myself? I need these fixes to build the site I want to build on top of cubing.js but it's not clear if this is moving sufficiently in the right direction for upstreaming or not. I don't need to bother with a fork and pull requests if I'm on the wrong path from your PoV, I can just maintain my patches in my own environment.

— Reply to this email directly, view it on GitHub https://github.com/cubing/cubing.js/issues/201#issuecomment-1224148802, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOLS2XCP4OAYXENHQ3PBLV2TND5ANCNFSM563LPDKA . You are receiving this because you were mentioned.Message ID: @.***>

--

rokicki avatar Aug 23 '22 14:08 rokicki

@rokicki Thanks again for being encouraging. But there is a big difference in the patches I will write and the work I will do if my expectation is that they're not destined to be upstreamed. If I'm on the wrong track from your PoV, then I will write absolutely minimal diffs so that it's easy to rebase my fork each week and stay up to date. It'll be easier to avoid refactors, because any significant refactoring would make the rebasing a huge pain.

For example, tonight I started refactoring the code in gan.ts to make it testable (it currently has no tests at all). In the process my unit tests found a bug in my rotation code, and I fixed it. Great! But the resulting diffs will be almost impossible to rebase against upstream. So I'll put up a fork and see if we can agree on some pull requests (next week, next month, whenever you have time or @lgarron has time); but if we know from now that this doesn't look good I will appreciate an early rejection so that I can focus on minimizing the rebasing work I will need to do instead.

It should only take me a day or two to organize my forked repo into branches you can take a look at and then we can figure out the longer term plan from there (next week-ish, hopefully).

anicolao avatar Aug 25 '22 02:08 anicolao

I've started a rewrite of the bluetooth support in my own application, but I'll leave this issue open here in case some enterprising soul wants to take on fixing cubing.js for this use case.

anicolao avatar Sep 11 '22 21:09 anicolao