Current rotation fixes #81
Conversation
…turns full circle. MAKES NO SENSE given pi IS half a rot but this way it works 100% on viz side...
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix rotation calculation bugs in the current-rotation function, which converts DMX pan/tilt values to 3D transformation matrices. The changes include improved variable naming, a parenthesis bug fix in the tilt calculation, and modifications to the rotation angle computations.
Key changes:
- Renamed variables from
dmx-pan/dmx-tilttopan-half-rotations/tilt-half-rotationsfor better clarity - Fixed a bug on line 71 where
tilt-scalewas incorrectly subtracted in the tilt calculation - Modified rotation calculations by adding a 0.5 multiplier to the angle conversions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (let [tilt-half-rotations (/ (- tilt (:tilt-center head)) tilt-scale) | ||
| adjust (Transform3D.)] | ||
| (.rotX adjust (* Math/PI dmx-tilt)) | ||
| (.rotX adjust (* Math/PI 0.5 tilt-half-rotations)) |
There was a problem hiding this comment.
The multiplication by 0.5 appears to be incorrect. The variable tilt-half-rotations already represents the number of half-circles (180-degree units), where one half-circle equals π radians. Therefore, the conversion to radians should be Math/PI * tilt-half-rotations, not Math/PI * 0.5 * tilt-half-rotations.
This can be verified by examining the inverse calculation in angle-to-dmx-value in transform.clj (line 229), which uses the formula: center-value + (angle / Math/PI) * half-circle-value. Rearranging this gives: angle = ((dmx - center) / half-circle-value) * Math/PI, which matches what the variable name suggests but contradicts the current implementation.
With the current code, a fixture will only rotate to half the intended angle.
There was a problem hiding this comment.
hmm might have had some other thing that was causing end result angles to be off then... will investigate
| (let [pan-half-rotations (/ (- pan (:pan-center head)) pan-scale) | ||
| adjust (Transform3D.)] | ||
| (.rotY adjust (* Math/PI dmx-pan)) | ||
| (.rotY adjust (* Math/PI 0.5 pan-half-rotations)) |
There was a problem hiding this comment.
The multiplication by 0.5 appears to be incorrect. The variable pan-half-rotations already represents the number of half-circles (180-degree units), where one half-circle equals π radians. Therefore, the conversion to radians should be Math/PI * pan-half-rotations, not Math/PI * 0.5 * pan-half-rotations.
This can be verified by examining the inverse calculation in angle-to-dmx-value in transform.clj (line 229), which uses the formula: center-value + (angle / Math/PI) * half-circle-value. Rearranging this gives: angle = ((dmx - center) / half-circle-value) * Math/PI, which matches what the variable name suggests but contradicts the current implementation.
With the current code, a fixture will only rotate to half the intended angle.
1 participant
tin