Fix response curve action test and add a new test case demonstrating how to modify action parameters in the test#624
Conversation
…ate other variable names and comments.
…where we were using an absolute tolerance suitable for integer axes, not Gremlin-style normalized ones.
|
Sounds good, I should also be done with the sweeping changes caused by renaming the intermediate output system. |
|
Added the remaining test cases, please take a look. I think I found a bug - the "symmetry" option for the cubic splines does not work, and I had to explicitly add points on both sides. In the UI, if I enable symmetry and add a point, the symmetric point is not added. I can fix that in a follow-up PR if you like. |
|
Okay, added a commit for that as well. I had to change some expected values very slightly for the cubic Bezier spline if I didn't add (0, 0) explicitly after enabling symmetry - not sure if it matters and it might actually be expected? |
| Args: | ||
| x: x-coordinate of the control point | ||
| y: y-coordinate of the control point | ||
| x: x-coordinate of the control point (will be clamped). |
There was a problem hiding this comment.
One could think of a RangeValue data type that enforces this and also makes the behavior/intent clear as this sort of "value has to be within" comes up in a few places.
There was a problem hiding this comment.
Here's one way to do it, let me know what you think and I'll open a PR
| points = self.control_points() | ||
| points[idx].x = x | ||
| points[idx].y = y | ||
| if self.is_symmetric: |
There was a problem hiding this comment.
The code didn't have this before either, but it may be worthwhile adding a check for the implied invariant that there is an odd number of control points when the symmetry mode is active.
There was a problem hiding this comment.
Added check and added a test case that uses this function.
|
Looks good to me so far. I think there is also something horribly broken with the UI for the response curve, at least it misbehaved quite badly when I quickly tried it before. So another thing to revisit. To me this looks fine and I can go ahead and merge it unless you have more tweaks you want to make to it. |
|
Please go ahead and merge it if the latest commits look fine, thanks! I didn't find the UI too broken, although I did notice that you can create spline curves that go out of bounds (didn't try running it): 
|
|
Hang on, not sure why the test is failing - investigating. |
…an even number of points
|
Okay, looks like adding that assert exposed a bug. Added a commit to fix the bug, let me know if more changes are desired, otherwise please go ahead with merging this PR. |
|
Looks all good to me. And the curves going outside the valid range is also in R13 so not a regression. I think there is code later in the pipeline that would result in those curve values being clamped if one attempted to send them to vJoy (but not 100% certain). |
2 participants
I will add more tests to this PR in the next few days to cover the other types of curves.