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

Max channels allowed #687

Open
vadzimvashchanka opened this issue Jul 18, 2023 · 14 comments
Open

Max channels allowed #687

vadzimvashchanka opened this issue Jul 18, 2023 · 14 comments

Comments

@vadzimvashchanka
Copy link

Hey! I'm using IF slides with 8 IF channels. Is there a way do display them all at once? Are there any cons against increasing the limit here? https://github.com/hms-dbmi/viv/blob/master/packages/constants/src/index.ts

@vadzimvashchanka
Copy link
Author

@ilan-gold @manzt anyone?

@manzt
Copy link
Member

manzt commented Jul 23, 2023

Hi there. IIRC the limit is primarily set due to our shader implementation (which requires a fixed number of channels) and to account for the challenges in human color perception. While technically feasible to display more channels simultaneously, the benefit is increasingly limited due to the difficulty in distinguishing a high number of colors at once.

That being said, I'm open to understanding your use case and happy to be convinced otherwise!

@ilan-gold
Copy link
Collaborator

I would agree with Trevor here. I would add that if someone (ideally not us) were willing to write the necessary code to make the number arbitrary, I would be interested in that. It would be mostly just string manipulation/shader injection. @manzt would be curious to hear your thoughts.

@manzt
Copy link
Member

manzt commented Jul 23, 2023

Right. We have an open discussion on this topic as well. Supporting such a feature would indeed require manipulating shaders at runtime, but iirc, recompiling shaders often depending on the number of channels might not be desireable perf wise.

@ilan-gold
Copy link
Collaborator

I think this could be mitigated with a maxAllowableChannels (or similar) prop. Default could be recompilation but people could set the prop if they don't want it to be dynamic

@vadzimvashchanka
Copy link
Author

Hi guys! Thank you for your response. So the context is the following. I'm a dev who takes part in integration of your package into the portal to visualize IF slides. Usually we have from 3 to 6 channels. But recently the users complained that they can't see some slides. There was an error in console about max number of channels allowed (6) for slides which have 8 channels. So as I see there is no way to avoid this error except forcing the app to display max 6 channels at once. Anyways the user is able to switch the channels and display a preferable list so it's not a big issue.
But it would be great to have this value configurable ideally though props as @ilan-gold suggested so the package users could decide if they want to display more slides channels (prolly with some sacrifice for performance) or less.
image

@ilan-gold
Copy link
Collaborator

Let's see what Trevor has to say. Others (@Nanguage and @YammyHuang) have the same request. I would be open to one of you implementing a solution, although want to see what Trevor's take is.

@vadzimvashchanka
Copy link
Author

@manzt what do you think?

@manzt
Copy link
Member

manzt commented Aug 7, 2023

Yes, I am for supporting an implementation! I am not sure I'll have the time to implement myself but would be happy to provide a review.

I think this could be mitigated with a maxAllowableChannels (or similar) prop. Default could be recompilation but people could set the prop if they don't want it to be dynamic.

I think ideally we should avoid exposing more props. Leaky abstraction. We just want to avoid recompiling shaders on every render (e.g., when we adjust contrast limits or change colors), but I think recompiling shaders if channels are added or removed should probably be OK. Not a super frequent interaction, and the UI for triggering adding/removing channels is typically some kind of dropdown with clicks.

@ilan-gold
Copy link
Collaborator

Ok @manzt I could see there being low-cost to that and/or a way to avoid it. So if anyone wants to implement this, we would take it. I may also have time to do this next month but am also not sure I will have the time.

@tymiao1220
Copy link

Thank you for this nice application. We have the same request for multiple channels visualization. Currently, it can handle 6 channels as expected when we try to add more, the entire image disappears. So we now only add to max 6 channels and then switch between each channel to avoid that.

@tymiao1220
Copy link

@ilan-gold @manzt Do you have any plan of this?

@manzt
Copy link
Member

manzt commented Oct 19, 2023

As @ilan-gold mentioned, we are in support of and would be willing to review a PR implementing this feature, but do not have a plan to implement ourselves.

@xinaesthete
Copy link
Contributor

I might consider making a PR for this if I have time - maybe relatively soon.

I wouldn't be concerned about the performance cost of recompiling shaders at runtime as the number of channels changes; the shaders are relatively simple.

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

No branches or pull requests

5 participants