Add ability to rearrange axes#52
Conversation
apply_axis_convention converts between any named conventions (normalize to HID first) modify_device_info gains remap_axes; mutually exclusive with invert_axes
# Conflicts: # docs/mouseApi/examples.md
Reviewer's GuideAdds explicit axis remapping support and tracks axis conventions on DeviceInfo, generalizing convention conversion to work between multiple named frames (LEGACY, HID, HID_Z_UP, ROS, UNITY) while updating the public API, docs, and examples to expose ROS/Unity and the new remap_axes parameter. Sequence diagram for generalized axis convention conversionsequenceDiagram
participant caller as apply_axis_convention
participant normalizer as _normalize_to_hid
participant remaps as _HID_TO_CONVENTION
participant parser as _parse_axis_remap
participant copier as _copy_axis_spec
caller->>normalizer: _normalize_to_hid(base)
normalizer-->>caller: hid_base
caller->>remaps: read _HID_TO_CONVENTION[convention]
loop for each target_axis, src in remap
caller->>parser: _parse_axis_remap(target_axis, src)
parser-->>caller: source_axis, sign
caller->>copier: _copy_axis_spec(hid_base.mappings[source_axis], sign)
copier-->>caller: AxisSpec
end
caller-->>caller: construct DeviceInfo with convention=convention
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pyspacemouse/config_helpers.py" line_range="70-71" />
<code_context>
+}
+
+
+def _parse_axis_remap(
+ target_axis: str, remap: Axis | tuple[Axis, Literal[-1, 1]]
+) -> tuple[Axis, int]:
+ if target_axis not in AXIS_NAMES:
</code_context>
<issue_to_address>
**issue (bug_risk):** The use of `|` union syntax in type hints will prevent this module from importing on Python 3.8.
Because `Axis | tuple[...]` uses PEP 604 syntax, this function annotation will cause a SyntaxError on Python 3.8 at import time. For 3.8 compatibility, update this parameter type to `Union[Axis, Tuple[Axis, Literal[-1, 1]]]` (to align with `AxisRemap`) or remove the annotation.
</issue_to_address>
### Comment 2
<location path="pyspacemouse/config_helpers.py" line_range="119-121" />
<code_context>
+ # Invert HID→C to get C→HID:
+ # HID[hid_target] = C[c_source] * sign → C[c_source] = HID[hid_target] * sign
+ new_mappings = {}
+ for hid_target, src in _HID_TO_CONVENTION[spec.convention].items():
+ c_source, sign = _parse_axis_remap(hid_target, src)
+ new_mappings[hid_target] = _copy_axis_spec(spec.mappings[c_source], sign)
+
+ return DeviceInfo(
</code_context>
<issue_to_address>
**issue (bug_risk):** Normalizing to HID assumes all source axes exist, which can raise a KeyError for partially defined specs.
`spec.mappings[c_source]` will raise if a source axis is missing, whereas the previous `apply_axis_convention` handled missing axes. If partially defined specs are expected (e.g., rotation-only/translation-only devices), consider skipping entries when `c_source` is absent or raising a controlled `ValueError` instead of letting a KeyError bubble up.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
peter-mitrano-ar
left a comment
peter-mitrano-ar
left a comment
There was a problem hiding this comment.
In the future please add me as a reviewer, I just noticed this by chance! Anyways, looks mostly good to me. I still need to try it myself manually though before I would merge. But I am posting one or two comments/suggestions now and will come back to it later
| AxisConvention.LEGACY: { | ||
| "x": ("x", 1), | ||
| "y": ("y", -1), | ||
| "z": ("z", -1), | ||
| "roll": ("pitch", -1), | ||
| "pitch": ("roll", -1), | ||
| "yaw": ("yaw", 1), | ||
| }, | ||
| AxisConvention.HID_Z_UP: { | ||
| "x": ("x", 1), | ||
| "y": ("y", -1), | ||
| "z": ("z", -1), | ||
| "roll": ("roll", 1), | ||
| "pitch": ("pitch", -1), | ||
| "yaw": ("yaw", -1), | ||
| }, | ||
| AxisConvention.ROS: { | ||
| "x": ("y", -1), | ||
| "y": ("x", -1), | ||
| "z": ("z", -1), | ||
| "roll": ("pitch", -1), | ||
| "pitch": ("roll", -1), | ||
| "yaw": ("yaw", -1), | ||
| }, | ||
| AxisConvention.UNITY: { | ||
| "x": ("x", 1), | ||
| "y": ("z", -1), | ||
| "z": ("y", -1), | ||
| "roll": ("roll", -1), | ||
| "pitch": ("yaw", 1), | ||
| "yaw": ("pitch", 1), | ||
| }, | ||
| } |
There was a problem hiding this comment.
this looks deliciously unit-testable, you could convert from each convention to HID and back and check for self-cosistency, or from HID_Z_UP --> ROS --> HID_Z_UP and such. Would be great to add some unit tests to this code base at some point. Doesn't even have to be run in CI to start if that complicates things. If you have the time/energy to add this let me know, regardless we can merge once I've tested by hand.
There was a problem hiding this comment.
I went ahead and initialized some in this follow on PR #54
|
|
||
|
|
||
| def _parse_axis_remap( | ||
| target_axis: str, remap: Axis | tuple[Axis, Literal[-1, 1]] |
There was a problem hiding this comment.
| target_axis: str, remap: Axis | tuple[Axis, Literal[-1, 1]] | |
| target_axis: str, | |
| remap: Axis | tuple[Axis, Literal[-1, 1]] |
I can't believe the linter is happy with this 🫣
hehe just my opinion I guess
Co-authored-by: Peter Mitrano (AR) <peter.mitrano@agile-robots.com>
Co-authored-by: Peter Mitrano (AR) <peter.mitrano@agile-robots.com>
| "and will be removed in a future release. Pass " | ||
| "axis_convention=AxisConvention.HID_Z_UP for the recommended " | ||
| "right-handed Z-up frame, or AxisConvention.HID for raw HID axes.", | ||
| "right-handed Z-up frame, other values on AxisConvention for application-specific frames, or " |
There was a problem hiding this comment.
Feel free to ignore this, as this is at the discretion of the maintainers, but why would HID_Z_UP be the recommended frame when there are multiple to choose from?
I would expect plain HID (unchanged and standardized, what is sent over the wire) as the recommendation when no specific application is "closer" associated with the library. Or simply give no recommendation to begin with.
There was a problem hiding this comment.
I agree! I think this library inherited the odd legacy convention from the pyspacenavigator implementation, and in some sense the correct version of what it was trying to do is HID Z Up: it has the same translation axes just matched with conventional rotation axes.
But if there's an opportunity to reconsider, providing the device's native axes by default feels more neutral/parsimonious. If someone had written code against the raw device they wouldn't have to think much when swapping to use the library. Someone writing fresh code against the library is likely going to think about frame conventions on an application-specific basis, and will flip to the correct one in any case.
Thoughts @JakubAndrysek?
3 participants
Common alternative axis conventions (ROS, Unity #50 (comment)) would require swapping axes (not merely inverting them). This wasn't supported, so this PR adds that ability. Not 100% sure on this design but figure you may already be thinking about it
apply_axis_convention()generalized. Previously only accepted LEGACY specs as input. Now converts between any pair of named conventions via an internal HID normalization step.DeviceInfogained aconventionfield (defaultAxisConvention.LEGACY) to track this.modify_device_info()extended. Newremap_axes: AxisRemapparameter for axis swaps and sign flips in one step.remap_axesandinvert_axesare mutually exclusive. Returnsconvention=Nonewhen axes are modified, preserves the input convention (newly tracked on the object, per above) otherwise.New:
AxisConvention.ROS,AxisConvention.UNITYAxisRemaptype aliasmodify_device_info(remap_axes=...)new parametercreate_device_info(convention=...)new parameterDeviceInfo.conventionnew field (defaultAxisConvention.LEGACY, backward compatible)Summary by Sourcery
Add support for named axis conventions and flexible axis remapping to better match common coordinate frames such as ROS and Unity.
New Features:
Enhancements:
Documentation: