-
Notifications
You must be signed in to change notification settings - Fork 17
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
Update default controls #532
Conversation
- Consolidate default config - Update behavior for refresh scene inspector - Swap some mouse controls to get a slightly better compromise - Change zoom and FPS rotate keys
- Consolidate default config - Update behavior for refresh scene inspector - Swap some mouse controls to get a slightly better compromise - Change zoom and FPS rotate keys
8a5a89c
to
3a865ea
Compare
- Consolidate default config - Update behavior for refresh scene inspector - Swap some mouse controls to get a slightly better compromise - Change zoom and FPS rotate keys
The plan is to move the load/save persistence logic into ControlsParameters, so that it's always aligned with the actual model state. Then we can get rid of the PrefService and orig_* logic completely in the NavigationControlsSettings dialog, in favor of just getting the state from ControlsParameters upon dialog initialization, and setting the ControlsParameters state any time widgets change.
fe55b3c
to
92244ee
Compare
… into default-controls
And add a method to reset the values to defaults.
And load them from persisted storage upon construction.
92244ee
to
9a99d72
Compare
* Set min/max/step in the annotation rather than via MutableModule API. * Replace checkbox hack logic with a refresh button. * Add a reset button to put the values back to defaults. * Add constants for magic numeric values. * Simplify initial control parameter value loading. * Fix case of FPS functions to match elsewhere in the codebase. * Downcase sciview.
9a99d72
to
af2d419
Compare
@kephale I finished the revamp of the navigation control settings. Those five properties are now persisted when mutated through sciview or sciview.controls, but not when changed directly on the sciview.controls.parameters object itself. This was the easiest approach since the sciview.controls has access to the SciJava context and therefore the |
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 like these controls a lot more, I find them a lot more intuitive! 🏆
But there are still some things I find surprising.
Y axis rotation inversion
The Y axis motion is inverted on the arcball. Can we set it to behave the same way that napari does? I think it would be cool to add checkboxes for X and Y axis inversion respectively, but it would be nice if the rotation behave consistently in both X and Y by default.
Initial camera position does not match initial mouse drag
Reproduce by starting sciview, adding a box, then pressing and holding the left mouse button and dragging slightly to the left or right. You'll see the scene jump, with the camera suddenly being below the floor, and the floor disappearing temporarily. Here's a video illustrating it:
jumping-rotation.webm
Maybe some state somewhere is misaligned?
Ctrl + left drag corrupts object position
Using shift + left drag moves around in FPS mode moderately fast, while ctrl + left drag moves around very fast. But when using ctrl, the selected object position also changes at the same time. I wonder if there are two behaviors mapped to this same input combo? Here's a video of it happening:
box-position-corruption.webm
Arcball mode resets camera after FPS mode camera movement
I guess this is probably intended behavior, but if you move around in FPS mode, then zoom or rotate using mouse wheel or left drag respectively, the camera "snaps back" so that the selected object is recentered again. This is not how napari (nor IIRC VisAD nor 3D Viewer) behaves; rather, in napari if you hold shift and left drag it pans the object (or the camera moves, I'm not sure—either way, it comes across like the object moving off center), and then click and drag again to rotate, it does not snap back, but rather the arcball rotates around the new center, so the object sort of "swings" around more widely. Maybe it would make sense for arcball to operate around the current center of the view, rather than always the center of the selected object?
Left drag and right drag do the same thing?
At least, I couldn't tell the difference between them. It seems like a missed opportunity, since right drag is pretty easy one-handed operation to perform on a mouse. It appears that napari maps right drag to zoom by default, same as mouse wheel, which also seems like kind of waste.
Maybe we could make it manipulate objects instead? I'm guessing most people zoom with mouse wheel, not right mouse drag. I think in general, having left mouse do camera stuff and right mouse do object stuff would have a nice intuitive symmetry.
Description tooltips in Controls dialog need updating
Once we decide for sure how we want slow, fast, and very fast FPS mode to work, we should update the descriptions of those parameters in the NavigationControlsSettings
accordingly, since as written right now they are wrong. Or, given that controls are configurable anyway, maybe we should just remove those hardcoded statements, since they'll be wrong for those who change to non-default control schemes anyway.
P.S.
🍪 If you haven't already tried it, check out what happens in napari in 3D viewer if you shift + right drag 😄
… into default-controls
This could be simplified after scenerygraphics/scenery#591 is addressed
Yeah, napari's shift + right came up in the original issue thread. I haven't tried implementing that one yet, but maybe we can punt for now :) |
Arcball currently uses the active node as the center. I think this means that when a Node is selected we need to save its position, then when FPS movement happens we update that position. As a consequence Arcball will use a Vector as its center instead of a Node's position. |
This one needs more poking. I thought it was related to how Arcball's lastX and lastY are initialized at the center of the screen, but I still got funky behaviors. |
Addressed
Addressed
I'm open to using this for Node manipulation for sure. However, I believe one of the reasons napari uses right drag for zoom is because it was a request from trackpad users. Since I'm a trackpad user sometimes I don't quite understand that. |
I vote to remove the descriptions. This should be in the UI Behavior dialog, not hard coded strings. |
@ctrueden y-axis is flipped now. I propose punting on:
|
Some of the code duplication [between sciview and scenery] here is related to scenerygraphics/scenery#591 but that is easy to resolve down the road. |
It maybe should not be bounded at all, because who knows what sizes of things people will be viewing inside of a sciview window.
I kinda like the short descriptions you changed to—I vote to keep them like that for now, rather than having none whatsoever. I see that #541 covers the two remaining issues discussed above. I also filed one more additional issue #543. |
This has some changes that include:
Closes #209