[kwatters]

lots of stuff.. look at the commits.

[supertick]

💯

[supertick]

Code review: inline findings below, ranked roughly by severity. Jme3Util, InMoov2Arm (via WebXR) and the Runtime startup changes look most urgent.

One finding couldn't be attached inline because GitHub doesn't render the JMonkeyEngine.java diff (too large):

• JMonkeyEngine.java ~L1225: getAngle ignores the axis and the node's real rotation. It now returns the last angle passed to rotateTo. For a node never driven through rotateTo, or moved by animation, physics or a loaded scene, currentAngleDeg is 0, so getAngle returns mapper.calcInput(0) (e.g. 10 for omoplate) whatever the mesh actually shows. getAngle(name, "z") also returns the same value as getAngle(name, "x").

🤖 Generated with Claude Code

[supertick]

Non-arm joints now render offset by their rest angle. rotateTo now applies the mapped angle on top of the captured bind rotation (bind * R(axis, deg)) instead of setting one absolute Euler component. Only the arm mappers in InMoov2Config were re-based so that rest maps to 0°. The head, torso, eye, hand and finger mappers still output absolute angles. Example: torso.midStom (0..180 → 50..130 on y) at rest 90 used to set Euler Y=90°. Now it rotates 90° past the authored bind pose. The same goes for eyeY, head.rothead (no mapper, 90 at rest) and every finger.

[supertick]

Unit change breaks WebXR IK. getDHRobotArm now builds the arm in meters (0.04, 0.28 …) instead of millimeters, but WebXR.java (around line 88) still sends millimeter targets: ik.moveTo("left", new Point(70 + x, -550 + y, z)). That target is about 550 m away for an arm that reaches about 0.6 m, so the solver never converges and the arm saturates at its joint limits on every pose update. WebXR needs to switch to meters, or the units need to stay consistent.

[supertick]

Nested autoStart peers are no longer started. Auto-start sub-peers are now created with Runtime.create, and only that peer is queued to start. The old recursive Runtime.start also started the peer's own autoStart peers. If A auto-starts B and B auto-starts C, Runtime.start("a", type) creates B (and maybe C through the plan) but only calls startService on B, so C is never started.

[supertick]

Race: startService now runs outside processLock. Thread 1 creates service X, releases processLock and is about to call startService. Thread 2 calls Runtime.release("x") under the lock and unregisters it. Thread 1 then starts the released, unregistered X: its threads run, and it sends started for a service that no longer exists. Also, a concurrent Runtime.start(name) for a service that exists but isn't running yet can return it before it has started.

[supertick]

Possible native use-after-free. stopMicrophoneCapture only waits 2 s for the recognition thread, then stopService/loadModelFromPath call closeVosk(), which frees the native Recognizer and Model. If acceptWaveForm or a blocked microphone.read keeps the thread busy past the join, the thread later calls getFinalResult() on a closed recognizer and can crash the JVM. Join without a timeout (after interrupting or closing the mic), or check that the thread has exited before closing.

[supertick]

startSimulator bypasses startPeer. It hardcodes Runtime.create(simName, "JMonkeyEngine") and ignores the simulator peer type in config. If a service of another type already exists under that name, the (Simulator) cast in the existing branch throws ClassCastException. It also no longer calls broadcastState as startPeer did, so the WebGui misses the peer-state update.

[supertick]

startService runs pip install --upgrade on the calling thread every time. With py4jInstall=true, every OakD start (and InMoov2 startChestCamera) blocks on pip resolving and downloading depthai, numpy, opencv and blobconverter. That takes seconds online, and offline it stalls until timeout. Because of --upgrade, it hits the network even when everything is already installed. Consider installing only when the packages are missing, or doing it once at install time.

[supertick]

Old LWJGL jars stay on the classpath. LWJGL moves from 3.2.3 to 3.3.2 and the Ivy retrieve pattern changes, but existing libraries/jar/lwjgl-*-3.2.3.jar files aren't removed. Reinstalling JMonkeyEngine puts the 3.3.2 jars next to them, which mixes versions on the classpath. That can cause NoClassDefFoundError (e.g. CallbackI$V) or native version-mismatch errors when GLFW starts.

[supertick]

Data race on pipOut. pipOut is a plain StringBuilder that the reader thread appends to. If reader.join(8000) times out (e.g. a lingering child process keeps stdout open), tailLines(pipOut.toString()) races with append. It can throw or return garbled text, which hides the real pip error. Use a StringBuffer or synchronize, and/or join without a timeout after the process exits.

[kwatters]

From Grok:

The ten review comments on pull request 1444 are fixed in the current code. The focused tests for the mapper rebase, the joint-angle math, and the old-jar cleanup passed.

Simulator joints. getAngle now reads the bone's twist about the requested axis from its rotation relative to the bind pose, so X and Z are no longer the same cached value. Head, torso, eye, hand, and finger mappers in InMoov2Config are shifted so the servo rest angle maps to 0° from that bind pose, which is the same contract the arm mappers already used.

WebXR and the simulator peer. The leftover 1,000-step IK loop is gone. The left-hand target uses meter offsets (0.07, -0.55) so it matches the arm's meter-long links. startSimulator goes through startPeer again, so it uses the configured peer type and publishes state.

Startup. Auto-start peers are queued together with their own auto-start children, and those children are started first. startService runs only if that same instance is still registered. Release marks the instance first, so a start that loses the race does not launch threads for a service that has already been removed.

Vosk, OakD, and installs. Microphone capture closes the line, then waits until the recognition thread has exited, before the native recognizer is freed. OakD runs pip install only when depthai, numpy, or cv2 is missing, and that install no longer passes --upgrade. Pip output is collected in a StringBuffer, and the reader is joined again if it is still alive after the process exits. After an Ivy install, jars from an older revision of that same artifact are deleted so LWJGL 3.2.3 and 3.3.2 cannot both sit on the classpath.

[supertick]

Second review pass, covering the two newest commits (1ccb902d4 review fixes, 94cf39286 VinMoov download). The earlier findings look fixed. Four things remain, most severe first.

1. A failed VinMoov download is retried on the render thread. (JMonkeyEngine.java#L4708)
loadDelayed calls ensureVinMoovModel() off the render thread, as its comment says it should. loadDefaultModels() then runs inside runOnJmeThread(..., 120) and calls ensureVinMoovModel() again. If the first attempt failed (offline, firewall, HTTP error, stalled transfer), the render thread repeats the whole download: up to a 20 s connect plus 120 s per read, which is longer than the 120 s task timeout. The window freezes, then the code logs "continuing without full node mappers". Suggest removing the call from loadDefaultModels() and keeping only the one in loadDelayed, or skipping the download when app is the current thread.

2. The model is fetched from master with no integrity check. (JMonkeyEngine.java#L4751, #L4850)
VinMoov5.j3o is a binary file that JME's BinaryImporter deserializes, and that importer instantiates the classes named in the file. The only check is size > 1 MB, so any push to InMoov2 master changes what every fresh install loads. A truncated transfer that still exceeds 1 MB is also accepted, even though the server sends Content-Length: 67449927. Suggest pinning the URL to a commit SHA, checking a SHA-256, or at least checking part.length() == connection.getContentLengthLong().

3. Vosk: a capture started during loadModelFromPath can still use a freed recognizer. (VoskSpeechRecognition.java#L679)
stopMicrophoneCapture() now runs without the lock, and closeVosk() runs later in a separate synchronized block. In the gap between them, startListening(), startRecording() or apply() can call ensureReadyAndCapture(). voskRecognizer is still non-null at that point, so startMicrophoneCapture() starts a new RecognitionThread. closeVosk() then frees the native recognizer while that thread may be inside acceptWaveForm, which is the JVM crash this change was meant to prevent. Suggest checking again inside the locked block: if micRunning or recognitionThread is set, release the lock, stop capture again, and retry. Another option is a lock that the recognition thread holds around each acceptWaveForm / getResult call, which closeVosk also takes.

Smaller issue in the same area (#L863): the loop reads the microphone field twice, once in the while condition and once at microphone.read. stopMicrophoneCapture sets the field to null under the lock, so a thread between those two reads gets an NPE. The catch then publishes a spurious "recognition error". Reading microphone into a local once, when the thread starts, avoids this.

4. Minor: the 30 s timeout in Py4j.venvCanImport never fires. (Py4j.java#L596)
readAllBytes() blocks until the process exits, so waitFor(30, SECONDS) only runs after the child has finished. A hung import, for example depthai probing USB, blocks OakD.startService with no limit. Suggest draining the output on a separate thread, or calling redirectOutput(Redirect.DISCARD) before waitFor.

Everything else in these two commits looks good: the startup queue and cancelPendingStart, the twist-about-axis getAngle with the fromBind rebasing, the superseded-jar cleanup, and the WebXR offsets in meters.

🤖 Generated with Claude Code