Track layer slider max value #7756
Conversation
This fixes the issue that occurs when setting a new tail length that is longer than the current maximum allowed on either the head or tail length slider. It sets the slider to the new max.
Tracks fade out of view completely at early timepoints (alpha tapers off to 0). Clamping alpha to .5 to have "mild" fading only.
for more information, see https://pre-commit.ci
napari/_vispy/filters/tracks.py
Outdated
There was a problem hiding this comment.
Personally, I would take this alpha change out and make a separate PR, but definitely take the advice of the core devs. I partly suggest that because I'm unsure about the change, but also because then each PR will serve a single 'task'.
For me, having the alpha fade completely out is very nice when there are lots of competing tracks in the canvas. Try out the tracks_3d example. With this alpha it's a bit too cluttered for me, even with the normal tail length.
That said I'd be very happy if the alpha was settable with something like fade_alpha-- but that's much more work to hook up😬
Perhaps try a different LUT than turbo. Something like PiYG will never 'fade to black', same with the inverted cmaps like 'I Purple', so may look nicer with long tails for you. What do you think with those?
I totally support and understand why you've changed the alpha regardless!
As an aside ... I'm surprised turbo is the default?
There was a problem hiding this comment.
Good points with various colormaps giving various amounts of "visible" fading (I did set it to turbo myself). I do think that introducing a "fade tracks" checkbox or so would be nice, simply because if, as a user, I demand 1000 frames track tail, I want to see all 1000 frames. Totally see the point of fading and I like the effect, but I think there are pros and cons for both.
There was a problem hiding this comment.
I would take this alpha change out and make a separate PR
Yes, happy to do that (sorry I didn't mention it in my previous reply). Waiting for advice from others too.
There was a problem hiding this comment.
Thanks for the pr @horsto! I agree with Tim that we should split out the alpha change from this PR, and I'm leaning towards including a fade_alpha or fade_opacity checkbox or even slider that allows the user to set the minimum alpha value.
There was a problem hiding this comment.
Great, ok, I deleted the alpha changing commit from this PR.
There was a problem hiding this comment.
And updated the description accordingly.
There was a problem hiding this comment.
Awesome, thanks for that change @horsto 🎉 It would be great if you could open an issue to track the request for setting a min alpha for the opacity fading. I'm approving and I've also changed the milestone to 0.6.0 since this is a small and non-controversial change. Congrats on your first contribution! 🔥
|
Fantastic, thank you! And yes, I will open an issue tomorrow following your suggestion. |
|
Docs fail here is unrelated, see also #7759 failing with same error. We'll look into it! |
|
I just re-ran it. Look like a GitHub hiccup. |
Oh yay, that would be nice. @jni and I re-ran a couple of times and across PRs and saw the same thing but it must've just been gremlins in the fiber |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7756 +/- ##
==========================================
- Coverage 92.97% 92.93% -0.04%
==========================================
Files 633 633
Lines 59245 59271 +26
==========================================
+ Hits 55081 55084 +3
- Misses 4164 4187 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It will be nice to have some simple tests here. |
@horsto there's a few tests for the tracks layer controls in this file. To test the current change you'd want to make a layer and controls the way it's done in the previous tests, then update the layer def test_update_max_tail_length(null_data, properties, qtbot):
"""Check updating of the tail length slider beyond current maximum."""
layer = Tracks(null_data, properties=properties)
qtctrl = QtTracksControls(layer)
qtbot.addWidget(qtctrl)
# verify the max_length argument is initialized correctly
assert qtctrl.tail_length_slider.maximum() == layer._max_length
# update max_length beyond the current value
layer.tail_length = layer._max_length + 200
assert qtctrl.tail_length_slider.maximum() == layer._max_lengthThen you'd want to do the same for the head length. Let me know if you're happy to add the tests yourself, otherwise I can push to this branch 😊 |
I will have a look later today! |
Added test_update_max_tail_length() and test_update_max_head_length() according to suggestion from @DragaDoncila . Unified QtTracksControls(layer) variable naming (controls) across testing functions.
|
Added tests. |
for more information, see https://pre-commit.ci
|
Thanks for adding those tests @horsto 🚀! CI is green so we'll merge this shortly. Congrats on your first contribution 🔥 |
References and relevant issues
Closes #7728
Description
This PR takes care of
twoone thing:.tail_length(or.head_length) on an existing tracks layer that is higher than the standard value of 300, display the track lengths correctly and adjust the new max of the tail or head length sliders to that new valueThe below part was part of the original PR, but I scratched it since developers decided it would be best to separate it into a new project / PR in the future.
I noticed that the shader tapers off alpha value of the earliest visible track points (points at the very beginning of tail or end of head) to 0 (zero). This counteracts the user's input because at a tail length of 500 for example, the earliest 100-200 points are barely visible. I clamped the alpha value to 0.5 instead of 0, which I feel is more reasonable because it leads to a "slide fade" (does not get rid of the effect completely), but maintains some visibility. One could think about enabling or disabling this feature with a checkbox.Below is a direct comparison. Note the change in visibility for blue (early) track points.