-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Joystick enhancements #8
base: main
Are you sure you want to change the base?
Conversation
Add an abstarct DriveCurve class and a concrete ExproDriveCurve class, both based on LemLib's implementation.
Allow users to assign a DriveCurve per axis to be used by the controller when updating joystick values.
…/joystick-enhancements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm from a design standpoint, but it needs more docs as well as some other nitpickings
include/gamepad/controller.hpp
Outdated
const float& RightX = m_RightX; | ||
const float& RightY = m_RightY; | ||
|
||
const Button& L1() { return m_L1; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't methods use lowerCamelCase
?
include/gamepad/controller.hpp
Outdated
float LeftX(bool use_curve = true) { | ||
if (use_curve && m_left_transformation) return m_left_transformation->get_value({m_LeftX, m_LeftY}).first; | ||
else return m_LeftX; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a specific reason why this is defined in the header file? This should be put in a source file to minimize hot package size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that since this function is included in a cold package source file, the version of the function that is included in the hot package binary should be discarded at link time, so there shouldn't be any noticeable difference in hot package size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (almost), just need to wrap up our convo on function defs in headers vs in source, and fix style issues
/** | ||
* @brief Get the transformed coordinate given the original. | ||
* | ||
* @param original The original value of the joystick | ||
* @return std::pair<float, float> The transformed value | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be aligned with the function. Usually clang-format catches this
/** | |
* @brief Get the transformed coordinate given the original. | |
* | |
* @param original The original value of the joystick | |
* @return std::pair<float, float> The transformed value | |
*/ | |
/** | |
* @brief Get the transformed coordinate given the original. | |
* | |
* @param original The original value of the joystick | |
* @return std::pair<float, float> The transformed value | |
*/ |
Test data:
|
(Note: Fisheye is still broken)
More tests:
|
Add controller deadband, curves and fisheye, also changes the const references to Button/floats into getter methods, and renames some stuff.
Test code:
Download the template for this pull request:
Note
This is auto generated from
Add Template to Pull Request