Expose additional Camera parameters in GUI with 3D popup widget #7626
8000Conversation
|
Also, I thought it would be nice to use a magicgui widget to simplify the angle widgets and callbacks, but it results in this really odd appearance. Any reason to avoid or not avoid using magicgui for these things? I know @magicgui is used in other parts of the codebase |
|
Very nice feature! Another thing I'd like to mention is that the |
Thanks, I am worried it's exposed a few too many options -- and may just be confusing for people to have all the camera options, but maybe not. It's not like the perspective slider was particularly intuitive previously though.
Yep, that was the problem! Nice catch, something I had missed.
Yes, I realized the labels bit after I walked away. One reason I like using magicgui widgets directly is that it gets rid of a lot of the 'boilerplate' that I don't like about Qt, but that's really just personal preference. This does sort of do what I want, but the spinboxes having it all horizontal is too wide for sure. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7626 +/- ##
==========================================
- Coverage 92.88% 92.85% -0.03%
==========================================
Files 630 630
Lines 58994 59066 +72
==========================================
+ Hits 54794 54845 +51
- Misses 4200 4221 +21 ☔ View full report in Codecov by Sentry. |
|
Are decimals for the angles needed? is that sort of precision useful? |
I think they are not needed, especially when using it to set it. My thought was, I suppose, that if you rotated an object into exactly the spot you want it, then the additional precision could be useful? I'd think rounding to the closest whole number should be fine still in that case. Good feedback! |
|
currently: |
|
When I heard a "camera control widget" I was thinking of using sliders rather than spin boxes. The slider widgets of magicgui and superqt supports direct user input, and they have smoother interactivity. I wonder which other people prefer. |
for more information, see https://pre-commit.ci
|
Updated original PR description. Made lots of changes to layout, callbacks, and now use superqt's labeled sliders. It will also now not open outside the napari window. Thanks for pointing me in this direction @hanjinliu |
…to camera-widget
| @@ -204,28 +204,152 @@ def eventFilter(self, qobject, event): | |||
| return False | |||
|
|
|||
| def open_perspective_popup(self): | |||
There was a problem hiding this comment.
@TimMonko this is a super badass PR! 😍
The functionality looks awesome and using the QLabeledDoubleSlider is 👌. I find it crazy that I looked away for like two seconds and you, @psobolewskiPhD and @hanjinliu have already iterated like 20 times. 😂
My only comment is now about code style: one of the principles I like to stick to is that code for a function should fit in about 1 screen vertically. (And not a crazy widescreen rotated to portrait mode. 😂) So that's about 25-50 lines. So, do you think you could break this function up to use a few helper functions/methods, so that it's easier to read at a glance? e.g. self.zoom = make_zoom_slider(...) etc.
|
@jni, thanks for giving me this advice. In the second example, I extract out just the _add_3d_camera_controls, but still create the sliders using their attributes directly. This styling is something I struggled with (so I appreciate your feedback!) because I never like separating out too much logic from the repetitiveness. Like yeah, it repeats to constantly set the methods of the slider, but the slider is a repeated part of the gui, ya know, 5 times. So then, if I abstract out too much its suddenly harder to understand how the code works without zooming all around the document. Thoughts? |
| self.rx.valueChanged.connect(partial(self._update_camera_angles, 0)) | ||
|
|
||
| self.ry = QLabeledDoubleSlider(popup) | ||
| self.ry.setRange(-89, 89) |
There was a problem hiding this comment.
not (-90, 90)? I am able to set that value no problem:
viewer.camera.angles
Out[1]: (np.float64(0.0), np.float64(0.0), np.float64(89.99999999999999))
viewer.camera.angles = (0, 90, 0)
viewer.camera.angles
Out[3]: (np.float64(-0.0), np.float64(90.0), np.float64(0.0))
There was a problem hiding this comment.
Try it locally with 90 and you'll see the camera gets gimbal locked, it's really strange behavior. You can prevent it by calling back the a angle changes but the sliders start zooming around and it's not intuitive
There was a problem hiding this comment.
I think any angle != (0,90,0) results in recalculation. ie. (0,90,90) gets converted to (-90,90,0) so it gets locked
There was a problem hiding this comment.
This is a fair point, though this is inevitable when doing Euler angles. I don't love that I can set the angles and then the slider is incorrect (just checked, it gets pinned at 89), but at the same time, it prevents some user issues, so 🤷 ok, we can leave it at 89. Thank you for the discussion!
There was a problem hiding this comment.
I really do wish we could correctly have 0,90,0 / 0,-90,0 be correct in the slider, but its the only instance where camera.angles can have a 90 y-value. It's annoying I agree, but I couldnt find a work around
| self.perspective = QLabeledDoubleSlider(popup) | ||
| self.perspective.setRange(0, 90) | ||
| self.perspective.setValue(self.viewer.camera.perspective) | ||
| self.perspective.setDecimals(0) | ||
| self.perspective.valueChanged.connect(self._update_perspective) |
There was a problem hiding this comment.
Yeah, so for me, the repetition is absolutely killing me. 😂 I totally get that too many layers of abstraction are annoying. My rule of thumb is that functions should be named and parametrised in such a way that you can read the code, basically trust that the function is doing what it's promising to do, and then carry on with your life. (ie you don't need to jump to another part of the code to understand it.)
This is possible with functions that are pure (input -> output), more difficult with methods with no arguments that rely on attributes in the class instance. So my advice here would be:
def labeled_double_slider(*, parent, range, initial_value, callback, decimals=0):
slider = QLabeledDoubleSlider(parent)
slider.setRange(*range)
slider.setValue(initial_value)
slider.valueChanged.connect(callback)
slider.setDecimals(decimals)
return sliderThen
self.perspective = labeled_double_slider(
parent=popup,
range=(0, 90),
initial_value=self.viewer.camera.perspective,
callback=self._update_perspective,
)And so on.
The key differences with your early attempt are:
- self isn't used, so make it a function, not a method
- use keyword-only arguments, which makes the calls more readable
Indeed, black doesn't let us save much/any space (though the default value of decimals helps). But there's other advantages, such as helping us with an API for when/if we want to become Qt-agnostic.
There was a problem hiding this comment.
Ok, I tried again!
Thanks for clarifying why to make something a function vs method -- again these are things I read but just don't make sense until its actually in practice.
Also, learned something new about the * today :)
I think you will like this next refactor more, let me know!
I definitely like how you say its important if there is goal to become Qt-agnostic. Then, my question becomes how does that work with typing? I typed all my functions and such because I find it really helps me navigate the codebase better. Does a future version without Qt use some kind of parent Type everywhere to mask whether its Qt or some other backend? (kind of like how importing qtpy stuff is either pyside or pyqt). Or do you just update arguments everywhere (seem tedious) to be like arg: QType | newType
There was a problem hiding this comment.
Then, my question becomes how does that work with typing?
we haven't got that far yet 😂
But I expect something like:
Widget = QType | IPyType | WxType | TogaType | whatever
def func(...) -> Widget|
Sorry I didn't get a chance to play with this, but why does z and x offer -180 to 180, which are identical, while the other two go from -90 to 90 (well 89)? Is that from viewer.camera? |
If by other two you mean perspective and y? Perspective is (0,90) from the previous behavior -- I think it could go much higher but I didn't want to change behvaior.
|
|
Ah, sorry I wasn't clear. I just expected them all to be -90 to 90 (well 89 I guess). |
|
Ah yes fabulous point. It's so that any real camera.angle set in any way (click and drag, programmatically) is represented properly in the widget. Trig magic means we can have 180 for x and z. |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: |
References and relevant issues
Task in #7587 to expose more canvas settings in the GUI. This exposes zoom and angles of the Camera in the popup widget
Description
A user wanted to reproduce the zoom and angle of the 3D camera between sessions, but with the desire to not use the console. I'd think this is not uncommon. This PR adds multiple widgets to the ndisplay==3 popup to expose these settings, and adds zoom to ndisplay==2. It adds a labeled slider widget to all camera settings. Adding this in the form of a widget also increases accessibility for users with difficulty operating a pointer for spinning. Open to discussion about step size and number of decimals displayed. Because this widget popup cannot be used at the same time as the canvas itself, I don't think too many callbacks need hooked up.
Current behavior is to bound angles with X[-180, 180], Y(-90,90) and Z[-180,180] because values outside (-90,90) for Y result in recalculation of viewer.camera.angles. See this comment chain for discussion of this implementation.
Current PR:
Original Description
This PR adds multiple widgets to the ndisplay==3 popup to expose these settings. It also changes the perspective slider to a spinbox; however, if this is well received it would be nice to update each of these widgets to include a slider (for easy spinning) and the spinbox. Adding this in the form of a widget also increases accessibility for users with difficulty operating a pointer for spinning. Open to discussion about step size and number of decimals displayed. Because this widget popup cannot be used at the same time as the canvas itself, I don't think too many callbacks need hooked up.