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

Add MultiChannelImageVisual #2156

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

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Jul 25, 2021

This is something I've had in my work vispy-based project (SIFT) for a while, but only recent extracted all of the SIFT-specific stuff so that it could be included in vispy. This is something I've talked to @tlambert03 in other PRs about possible using as a based for some more complex ImageVisual logic. This Visual is an ImageVisual for RGB images, but each channel is its own texture (pro: full 32-bit float precision per-channel) and a clim and gamma for each channel as well. This is a very common use case for the field I'm in where people want to take remote sensing satellite imagery data and combine the individual wavelength "bands" into RGB images. You typically see recipes with brightness temperature limits (clims) and gamma corrections.

This visual also allows you to only fill in some of the bands. So maybe you only have the R channel filled in, but G and B are set to None (small array of NaNs in-texture).

I could add more tests to this and/or an example, but I wanted to get any feedback from people before I go too deep. @kmuehlbauer @tlambert03 @almarklein Thoughts?

@djhoese
Copy link
Member Author

djhoese commented Jul 25, 2021

CC @rayg-ssec who is the other dev on the SIFT project and has been curious about pushing SIFT features to vispy for a while.

@tlambert03
Copy link
Member

tlambert03 commented Jul 25, 2021

looks promising! I could definitely build off of this for complex-numbered image visuals.
My immediate thought is that it would be nice not to hard-code on three channels, but rather provide a hook for converting n-channels into a vec4 color. I like how your _create_textures is already creating n textures based on the number of channels... so it seems like you're half-way to an arbitrary number of channels?
For me, I'd just need a way to hook into _build_color_transform

I realize the hard part of arbitrary channels is the shader itself, which mostly comes down the the $get_data and $color_transform functions ... could something like this work for get_data?

def _nchannel_texture_lookup(n):
    from textwrap import indent, dedent
    body = [f"val[{i}] = texture2D($texture_{i}, texcoord).r;" for i in range(n)]

    out = """
    float[{n}] texture_lookup(vec2 texcoord) {{
        if(texcoord.x < 0.0 || texcoord.x > 1.0 ||
        texcoord.y < 0.0 || texcoord.y > 1.0) {{
            discard;
        }}
        float[{n}] val;        
{body}
        return val;
    }}""".format(n=n, body=indent('\n'.join(body), '        '))
    return dedent(out)


def build_interpolation(self):
    self._data_lookup_fn = Function(_nchannel_texture_lookup(self.num_channels))
    self.shared_program.frag["get_data"] = self._data_lookup_fn
    if self._texture.interpolation != texture_interpolation:
        self._texture.interpolation = texture_interpolation

    for n, tex in self._texture.textures:
        self._data_lookup_fn[f"texture_{n}"] = tex

and then color_transform would have to accept a float[n_channels] and return a vec4?

@djhoese
Copy link
Member Author

djhoese commented Jul 25, 2021

Yes, we could definitely make more string-format-y. I think I just decided to cut off the amount of effort I put into this PR before getting feedback so it definitely paid off since you suggested exactly what needs to be done to make this more useful. I'll look at this this week.

@djhoese
Copy link
Member Author

djhoese commented Jul 26, 2021

@tlambert03 I just had a thought, should this visual be limited to 2 bands, 3 bands, and 4 bands? There isn't really a need for 1 band as that would just be the ImageVisual and there isn't really a default behavior we can provide for more than 4 bands and at that point I'm not sure clim and gamma make much sense. At that point we'd probably be better redesigning how users provide and specify their data.

@tlambert03
Copy link
Member

that would probably meet 90%+ of use cases (including complex stuff as long as I can somehow provide the color lookup for the vec4 returned by get_data.). Fine with me if you want to leave it there :)

@almarklein
Copy link
Member

I like @tlambert03 's idea to generalize the coloring. I fact, maybe it should be possible to have 6 channels that map to e.g. yellow, red, pink, blue, cyan, green?

Another thought is to look into a very different approach to solving this use-case: using grayscale images (with colormaps) and using additive blending. Though I kind like the idea that (eventually) we render things as expected (alpha blending etc) and users should not touch blending parameters.

FWIW we have rgbaf32 too, right?

Comment on lines 51 to 52
def set_clim(self, clim):
if isinstance(clim, str) or len(clim) == 2:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: call this clims like you do in the other clim setter makes this a bit easier to read.

@djhoese
Copy link
Member Author

djhoese commented Jul 26, 2021

FWIW we have rgbaf32 too, right?

Yes, but as far as I could tell OpenGL doesn't support setting one channel of those at a time. That's what this visual would require (ex. set red, later set green, etc).

@djhoese
Copy link
Member Author

djhoese commented Jul 27, 2021

FYI I am working on this but am getting really weird results depending on what I set the alpha channel to. I think it has to do with setting a 32-bit float on the alpha channel when GPUs probably don't provide a full 32-bit precision on the .a of a vec4. I decided to implement it differently than you suggested @tlambert03 in order to keep relatively the same structure of everything, meaning a texture lookup that returns a vec4, but that might be my problem here. I'll see if I can figure it out.

@djhoese
Copy link
Member Author

djhoese commented Jul 28, 2021

@tlambert03 I'm going to write some more tests for the RGBA case, but this seems to work as I expect for 2 input arrays. How else can I refactor this to make it easier to customize?

As for the issues I was having before, the main problems were:

  1. I had the .size property backwards. I was forgetting to do [::-1].
  2. Alpha needs special handling if it is NaN or if the clims include infinity. It was ending up with a value of 0 in these cases which isn't super useful.

@djhoese
Copy link
Member Author

djhoese commented Jul 28, 2021

I had to xfail two of my new tests because they assume NaNs are detected properly which they aren't on CI. This is the same as what I had to do for some of the ImageVisual tests that use NaNs.

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

Successfully merging this pull request may close these issues.

None yet

3 participants