feat(mouserangemanipulator): adds a scale parameter to mouse listener… #1900
Conversation
|
@agirault took a first pass at this with an optional param, passed in last, defaulting to 1. Let me know what you think and what changes you'd like to see here! |
There was a problem hiding this comment.
Hey @WillCMcC, great to see you here 👋 Thanks for the contribution!
So... which year did you come from?
Hoping you've seen that show, or my joke will fall flat.
I left a couple of comments below but haven't tested, so feel free to correct any of my assumptions below.
It would be ideal if you could update the example / API docs / tests... if those exist. If not, you're more than welcome to take a stab at adding them, but I understand if that's not high on your priority list. It would help to maintain the changes, if not we'll look into it eventually.
Thanks again!
Sources/Interaction/Manipulators/MouseRangeManipulator/index.js
Outdated
Show resolved
Hide resolved
Sources/Interaction/Manipulators/MouseRangeManipulator/index.js
Outdated
Show resolved
Hide resolved
Sources/Interaction/Manipulators/MouseRangeManipulator/index.js
Outdated
Show resolved
Hide resolved
|
Ok I made the changes that you requested. I played with the example locally and it appears to be behaving as you expected. I'm validating with my eyes so if you can think of something to log out to validate I'll post screenshots. Tests pass! I added some comments to the public API methods that will hopefully help the next user. Im unsure if my language is precise enough so let me know if those are helpful or confusing. I've updated the example as well so that horizontalListener has scale of 2 to demonstrate how to use that parameter. EDIT - I realized in the example that each listener (horizontal, vertical, scroll) controls a different parameter of the scene so my updates to the example might be confusing. lmk |
Sources/Interaction/Manipulators/MouseRangeManipulator/index.js
Outdated
Show resolved
Hide resolved
| listener.setValue(value); | ||
| incrementalDelta.set(listener, 0); // TODO: could still store partial delta that's ignored due to the rounding? | ||
| } else if (value === listener.max && newDelta > 0) { |
There was a problem hiding this comment.
Missing min & negative newDelta?
agirault
force-pushed
the
range_manipulator_scale
branch
2 times, most recently
from
76aa2bd to
8359a15
Compare
Adds a scale parameter to mouseRangeManipulator with a default value of 1. In processDelta, this scale is applied to the event delta. This allows users to achieve variable scroll speeds at a single step value. fix Kitware#1894 Co-Authored-By: Alexis Girault <alexis.girault@kitware.com>
Co-Authored-By: Will McCambley <willmccambley@gmail.com>
agirault
force-pushed
the
range_manipulator_scale
branch
from
8359a15 to
39b92da
Compare
|
@agirault Thank you for humoring me as I put this together, and your guidance throughout 🙏 |
|
🎉 This PR is included in version 18.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…s to scale the delta
Adds a scale parameter to mouseRangeManipulator with a default value of 1. In processDelta, this
scale is applied to the step value. This allows users to achieve variable scroll speeds at a single
step value.
re #1894
-A clear and concise description of what the problem was and how this pull request solves it.
fix #1894
This contribution is funded by Sirona.