Skip to content
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

Complex volume - Continued #2539

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Complex volume - Continued #2539

wants to merge 27 commits into from

Conversation

brisvag
Copy link
Collaborator

@brisvag brisvag commented Oct 23, 2023

Picking up the work in #2314; refer to that for previous discussion.

@brisvag brisvag mentioned this pull request Oct 23, 2023
@brisvag
Copy link
Collaborator Author

brisvag commented Oct 24, 2023

I think that either we merge this as it is (after adding tests), or we modify both Image and Volume to account for ComplexImage, dropping the subclass approach. I kinda prefer the second... In that case, we can also deprecate ComplexImage by simply pointing to Image itself 🤷 @djhoese @jni

@djhoese
Copy link
Member

djhoese commented Oct 24, 2023

It'd be easier if ImageVisual wasn't as complex (not the data type) as it is. I think maintaining either implementation has pros and cons so whoever wants to do the work gets to choose.

@aganders3
Copy link
Contributor

aganders3 commented Nov 3, 2023

This is great to see. I am interested if there is a way to unify this with some experiments I have done with 2D transfer functions for volume rendering. The idea basically being that we could move a lot of this logic into something like a 2D colormap with a glsl snippet, instead of being part of the visual itself.

For reference here is what I mean regarding a 2D transfer function. On the bottom you see a 2D histogram showing scalar data value (x) and gradient magnitude (y) with the transparent 2D transfer function overlaid. I can envision using the same method but with real/imag as the inputs to the transfer function.

Screenshot 2023-11-03 at 2 06 33 PM

@brisvag
Copy link
Collaborator Author

brisvag commented Nov 6, 2023

Are you suggesting to basically have the color2scalar2 snippet settable? Or more something more sphisticated?

@aganders3
Copy link
Contributor

Close, but I think I'm suggesting something a little more sophisticated. It's more like it would skip colorToScalar and instead pass more parameters into applyColormap (or applyTransferFunction). So I guess I am proposing something like this, which would be scriptable similar to the way colormaps are scriptable:

vec4 applyTransferFunction(vec4 data, vec3 loc, vec3 step)

Then for a complex image you might supply a transfer function like this to set the opacity by the magnitude, and the color by the phase. If I'm reading the code correctly I don't think this would be possible with the code proposed in this PR.

class PhaseColorMagAlpha(BaseTransferFunction):
    glsl_map = """
    vec4 transferFn(vec4 data, vec3 loc, vec3 step) {
        return applyColormap(atan(data.g, data.r)) * vec4(1.0, 1.0, 1.0, length(vec2(data)))
    }
    """

loc and step would be useful if you wanted to also calculate gradient magnitude of the data, as in my example above. (see https://core.ac.uk/download/pdf/276284248.pdf for motivation)

There might be more elegant APIs, but the simplest way to do this without breaking anything may be to just add a new rendering mode (Translucent2D). I suppose that leaves a place for selecting mag/phase/real/imag for complex data in one of the existing rendering modes.

@brisvag
Copy link
Collaborator Author

brisvag commented Nov 6, 2023

Interesting... Sounds pretty cool! But this maybe should be handled as a followup PR in case? I don't think we're necessarily making it easier or harder with this PR?

@aganders3
Copy link
Contributor

Yeah that's fair enough - sorry for the noise in that case, I just got excited because it seemed related! What I described might cover the behavior from this PR, but I can appreciate the value in making it simple to do common things (users shouldn't have to write GLSL to just show the magnitude of some complex data). I will polish up what I have to get it ready for a separate PR.

@aganders3
Copy link
Contributor

aganders3 commented Nov 6, 2023

In the meantime I'll add a couple comments on this PR :).

Note colorToScalar is doing the same work as colorToVal. I prefer the name colorToScalar so I suggest you remove colorToVal and replace its usage.

float colorToVal(vec4 color1)
{
return color1.r; // todo: why did I have this abstraction in visvis?
}

I also prefer dropping the Complex* subclasses as you suggested.

Finally a tiny nit - I would add a way to toggle the complex_mode in the example.

@brisvag
Copy link
Collaborator Author

brisvag commented Nov 6, 2023

No worries, it's good to make sure that we don't mess up possible future/better ideas :P

All great suggestions, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants