cubing / cubing.js

🛠 A library for displaying and working with twisty puzzles. Also currently home to the code for Twizzle.
https://js.cubing.net/cubing/
GNU General Public License v3.0
238 stars 44 forks source link

Moves repeat indefinitely when using bluetooth input #195

Closed anicolao closed 2 years ago

anicolao commented 2 years ago

To Reproduce Steps to reproduce the behavior:

  1. Go to https://alpha.twizzle.net/edit/
  2. Set input to a bluetooth cube (I am using a Gan i2)
  3. Twist a face
  4. That twist repeats indefinitely - e.g. U1 -> U2 -> U3 -> ...

Expected behavior One twist should be recorded.

Browser: Google Chrome 104.0.5112.79 (Official Build) (arm64) Revision 3cf3e8c8a07d104b9e1260c910efb8f383285dc5-refs/branch-heads/5112@{#1307} OS macOS Version 12.3.1 (Build 21E258) JavaScript V8 10.4.132.20

lgarron commented 2 years ago

Thanks for filing an issue.

I'm guessing this is because Gan they switched from their inefficient polling implementation to characteristic subscriptions. Do you have any other cube to test with?

The i2 seems to be discontinued, but I've just ordered an i3 to debug this. If that doesn't work, I might ask you for more help debugging.

anicolao commented 2 years ago

I believe but am not 100% sure that this was working better a few weeks ago when you responded to me about bluetooth support on the software forum on speedcubing.net (I am "Osric").

The GAN i3 doesn't work at all. It shows up in Chrome's pairing UI but after selecting it though the bluetooth symbol appears in the tabstrip, the button does not grey out and rotating the cube does not control the puzzle. So its problems are different than the i2, which almost works.

anicolao commented 2 years ago

In the JS console I get an infinite number of these after connecting:

gan.ts:111 Uncaught (in promise) TypeError: quat.clone(...).inverse is not a function
    at PhysicalState.rotQuat (gan.ts:111:38)
    at GanCube.intervalHandler (gan.ts:367:33)
anicolao commented 2 years ago

Here is a patch that will fix it:

diff --git a/src/cubing/bluetooth/smart-puzzle/gan.ts b/src/cubing/bluetooth/smart-puzzle/gan.ts
index b53dd60c..8e9d8eda 100644
--- a/src/cubing/bluetooth/smart-puzzle/gan.ts
+++ b/src/cubing/bluetooth/smart-puzzle/gan.ts
@@ -108,7 +108,7 @@ class PhysicalState {
     const quat = new Quaternion(x, y, z, w);

     if (!homeQuatInverse) {
-      homeQuatInverse = quat.clone().inverse();
+      homeQuatInverse = quat.clone().invert();
     }

     return quat.clone().multiply(homeQuatInverse.clone());
lgarron commented 2 years ago

Ooh, nice find!

lgarron commented 2 years ago

Alright, please let me know if it works now!

anicolao commented 2 years ago

Yes, this works. The problem was introduced when three.js dropped support for legacy features in commit a3c25a5418680e94e8860c29d4832cd24bf5c122 and a lot of the quaternion changes that were made in 2020 were gone. These used to log warnings to the console that might be worth looking for if you downgrade to an old three.js to see them in case there are more dependencies. This explains why it was working when I tried it a short while back and recently started failing, must have corresponded with a new deploy including the latest three.js code.

anicolao commented 2 years ago

Sorry that's the wrong (original) commit, the one that drops the feature entirely is https://github.com/mrdoob/three.js/commit/a3c25a5418680e94e8860c29d4832cd24bf5c122

lgarron commented 2 years ago

Yes, this works. The problem was introduced when three.js dropped support for legacy features in commit a3c25a5418680e94e8860c29d4832cd24bf5c122 and a lot of the quaternion changes that were made in 2020 were gone. These used to log warnings to the console that might be worth looking for if you downgrade to an old three.js to see them in case there are more dependencies. This explains why it was working when I tried it a short while back and recently started failing, must have corresponded with a new deploy including the latest three.js code.

Yeah, that definitely make sense. It's unfortunate that the type still exists and is merely marked as deprecated, since that doesn't cause a TypeScript error. :-/