-
Notifications
You must be signed in to change notification settings - Fork 7
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
WIP: Pygfx histogram #76
Conversation
Woohoo! |
thanks @gselzer, couple thoughts after pulling and running:
I guess, all in all, it's nice to see something working, but the additional imgui dependency, very different interface, and lack of flexibility with bar plots do make me wonder what it would take to go just a little lower and use pygfx directly (I know that makes axes harder ... ) |
however ... it is better than nothing, and as long as we can keep it working with pygfx without the dependency on fastplotlib and imgui, and gracefully fallback (i.e. not showing a histogram button if the additional stuff isn't there), then it's fine with me as an experimental feature |
src/ndv/views/_pygfx/_histogram.py
Outdated
class PanZoom1DController(PanZoomController): | ||
"""A PanZoomController that locks one axis.""" | ||
|
||
_zeros = np.zeros(3) | ||
|
||
def _update_pan(self, delta: tuple, *, vecx: Any, vecy: Any) -> None: | ||
super()._update_pan(delta, vecx=vecx, vecy=self._zeros) | ||
|
||
def _update_zoom(self, delta: tuple, *, vecx: Any, vecy: Any) -> None: | ||
super()._update_zoom(delta, vecx=vecx, vecy=self._zeros) |
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.
Take a look at this, and there are a few examples in the docs too which avoids having to make a subclass to do this: pygfx/pygfx#778
src/ndv/views/_pygfx/_histogram.py
Outdated
axis="x", | ||
edge_thickness=8, | ||
resizable=True, | ||
parent=self, |
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.
the parent graphic here would be the line.
Cool to see you guys doing more on this!
Yes we do not have a release with axes yet. We're in alpha (the entire WGPU ecosystem is still new and evolving and we're built on that). Axes are especially still quite wonky, we're really waiting for pygfx/pygfx#495 before they can work nicely (for an example of weirdness see fastplotlib/fastplotlib#613). Nonetheless it's nice seeing you try things to get more examples of use cases! We're planning for a new release in early January. We're catching up with upstream changes from
It might be:
If you change the zoom level of the controller that should be sufficient.
imgui is intended to be completely optional, if you are building a Qt app you probably don't need imgui. fastplotlib@main currently imports imgui but this PR will fix that.
I think it should be easy to add this to the shader? Not sure the best place to add it but if you post an issue they can probably help you.
The issue with thinner lines is that you have to be more accurate with your mouse movements to grab onto them, you could make a thinner inner line and a transparent thicker outer line and use the |
14199f8
to
fa0a5d0
Compare
@tlambert03 made some progress here on a pure-pygfx version: I'm still seeing a nice framerate, ~45FPS on my machine. Much better than the ~10FPS I was seeing with vispy. Currently, I've got the gamma curve, but without a handle. The main reason I left it in is because it helps for clim identification - i.e. it's harder to see the clims without the gamma curve. It likely also provides some utility without the ability to edit. If you like this direction better, there are some further bugs to squash:
|
great work as always! i like the general look and feel a lot better!
by the way, even if pygfx doesn't have shader-side gamma, there's no reason we can't apply it on the CPU. vispy also didn't have shader side gamma until i added it in vispy/vispy#1844 ... and we also weren't using it here until #77 ... so CPU-side gamma is just fine as a fallback (that would be a different PR though)
that's great! I still want to understand why thought 😂 ... particularly given that I didn't see it on my windows machine. also doesn't need to happen here, but some profiling to narrow it down to a specific culprit would be super interesting. (i.e. is it really actually vulkan vs opengl on your gpu? or something more mundane)
I actually think that's great, and could very much live with just that for a while
do you mean you think it's YAGNI to pan/zoom on the histogram? that's surprising to me... you mean you think it's fine to look at it full scale all the time? or just that it can be controlled programmatically rather than with mouse wheel? Also not sure how to reconcile that comment with the next video that seems to show pan/zoom? I think i'm not following :)
do they settle into the actual values after a very short delay though? if so, that also doesn't bother me too much |
Well, it does have a shader-side gamma, but it applies to the entire scene being rendered, which is unfortunate. But, more importantly, it's probably a separate issue that really pertains to the
I agree that figuring out why would be interesting, but until it affects someone else I feel like I could spend my time better elsewhere...
Great!
Well, I didn't push that code with the pan/zoom - I just added it back in to create the GIF 😅 I will push it shortly, though, because it does work. I do still have a sense of YAGNI, in that full scale all the time might be okay. My concerns are more with how pan/zoom gets reset. Right now, changing the data always resets the view, and there is no dedicated UI control for resetting the view on the entire dataset. We may want a dedicated button for that. But I don't feel too strongly about it all.
No, the values never settle. Consider the fact that the maximum value (shown by the y-axis tick) isn't really shown by the histogram mesh. Depending on how far you pan, you might see some bins close to that value but I never actually see one at that value. Would be interested to see if others can replicate this behavior. |
Many things still don't work. But some things do!
Was previously using the bounding box maximum which was the height of the clims :(
6c1a6c2
to
9067b6c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2-mvc #76 +/- ##
=========================================
Coverage ? 67.07%
=========================================
Files ? 42
Lines ? 4367
Branches ? 0
=========================================
Hits ? 2929
Misses ? 1438
Partials ? 0 ☔ View full report in Codecov by Sentry. |
I zoom all the time with 16 bit data (in micromanager and elements). With full scale you normally don't have the sensitivity you need when adjusting the clim handles for data with low intensity values. So I don't see this one as yagni |
This PR throws together a WIP PyGFX histogram widget using fastplotlib. The goal would be parity with the VisPy widget, but the current progress is quite far from that goal. Eyes and discussion encouraged.
TODO:
HistogramCanvas
methodspip install git+https://github.com/fastplotlib/fastplotlib.git@f0f80c6a4deabb0941154584c854f2004b0cc316
. I could not find an earlier version of fastplotlib with axes.Points of discussion:
imgui-bundle
, which is ~30MB. Granted, it's an optional dependency (i.e. you could use vispy instead).LinearRegionSelector
to add in a gamma curve.