-
Notifications
You must be signed in to change notification settings - Fork 236
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] FEATURE: Convenience functions for volume rendering transfer functions #178
base: master
Are you sure you want to change the base?
[WIP] FEATURE: Convenience functions for volume rendering transfer functions #178
Conversation
Travis CI build is currently failing due to the issues with unpkg (mjackson/unpkg#134) |
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.
Excellent work, this is also what I had in mind but never got to code up cleanly. Some comments on the code, maybe you always want to add some unittests?
ipyvolume/transferfunction.py
Outdated
@@ -164,3 +165,141 @@ def control(self, max_opacity=0.2): | |||
return ipywidgets.VBox( | |||
[ipywidgets.HBox([ipywidgets.Label(value="levels:"), l1, l2, l3]), ipywidgets.HBox([ipywidgets.Label(value="opacities:"), o1, o2, o3])] | |||
) | |||
|
|||
|
|||
def linear_transfer_function(rgb_values, |
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.
Instead of rgb_color, we can just name it color, and let matplotlib deal with it:
https://github.com/glue-viz/glue-jupyter/blob/7254451a2a671cdea07296861abd3f0cf50c6409/glue_jupyter/ipyvolume/volume.py#L43
It seems to accept named colors like 'red'
, hex values '#f0e6a4'
, and float tuples.
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.
That's much more elegant, thanks. Also means we can probably use this in conjunction with the color picker ipywidget, too! (Let me check that last part, but that would be great).
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.
It would be great if we do it on the frontend, and indeed use the color picker!
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.
What I've done in a previous project, was to put all the colormaps of matplotlib in 1 texture, of #texture x 1024 pixels, and then the index selected the row of the texture.
ipyvolume/transferfunction.py
Outdated
return transfer_function | ||
|
||
|
||
def load_transfer_functions(include_rgb_linear=True, |
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 would not call it load (get?), and I'd make it 'singleton' like, so it will always return the same widgets, what do you think? (it will basically cache the widgets).
My guess is this will be used as part of a gui to select a particular colormap?
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.
Let's call it 'predefined_transfer_functions'.
I'm not sure I understand correctly: do you want this to stay a function but always return the same dictionary, or have me make a singleton class for it?
I have no major plans for a colormap gui. But I do want to set future efforts at a GUI up for success as much as possible, so if you have advice on things I'm doing (or not doing here I'm happy to hear it.
What I want in the short term is a bunch of transfer function widgets so I can easily switch between them programmatically for the multivolume rendering I'm working on now.
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.
What I would like to see, is when a transfer function is created with the same set of parameters (same key for the dictionary), that it returns the same transfer function. That will limit the number of widgets created, what do you think?
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'm not sure how best to implement the idea, but I think it's a good strategy/goal.
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.
sth like this:
tf_cache = {}
def get_transfer_functions(...)
tf = tf_cache.get(key)
if not tf:
tf = ....
tf_cache[key] = tf
...
I should also mention @vidartf 's work on ipyscales: |
Thanks for the link, I didn't know that about @vidartf. So far I've thought exactly zero about how to display colorbars next to figures, but that will be a pretty necessary next step. |
@maartenbreddels can you explain the difference between the four different transfer function classes in python? Those python classes are:
Found in And on the javascript side, there's Found in |
e861e87
to
6dead3f
Compare
I propose adding convenience functions for volume rendering transfer functions.
Example use
Transfer function from single RGB value with linear opacity ramp
Transfer functions based on matplotlib colormaps
Load all predefined transfer functions to a dictionary
And use them like so:
Discussion
Now that it's possible to have multiple volumes displayed in the same figure, we need something like this to increase the useability (and it looks like this has been suggested before):
I've also seen this issue play out too, since the default choice is very non-standard for my field:
Let me know what your thoughts are, happy to discuss etc.