-
Notifications
You must be signed in to change notification settings - Fork 46
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
Comments
@ilan-gold @manzt anyone? |
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! |
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. |
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. |
I think this could be mitigated with a |
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. |
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. |
@manzt what do you think? |
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 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. |
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. |
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. |
@ilan-gold @manzt Do you have any plan of this? |
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. |
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. |
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
The text was updated successfully, but these errors were encountered: