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

Only load enabled channels #173

Merged
merged 4 commits into from
Jan 8, 2024
Merged

Conversation

frasercl
Copy link
Collaborator

@frasercl frasercl commented Jan 8, 2024

Addresses #158: don't load disabled channels.

To verify:

  1. Open test app.
  2. Switch volume to "OME Zarr (Nucmorph)"
  3. Disable either channel.
  4. Step through time. Each time point should load faster than normal.
  5. Re-enable the disabled channel. The channel should load into its current timepoint.

What this PR does NOT do:

This is a relatively small change to address the "letter" of the linked issue, which falls just a bit short of the "spirit" of the issue for the following reasons:

  • Channels are initialized as enabled and only disabled after loading has begun, so this feature gives no benefit to the initial load of a volume. As a consequence, because the test viewer does not have a JSON volume that includes both multiple time points and enough channels to require more than one atlas image, I had trouble testing how the JSON atlas loader behaves when loading a subset of channels.
  • Though I spent a long time trying to clean up how channel enabled/disabled state gets updated to fix a bug, which turned out to be caused by something unrelated and silly, I didn't manage to find a change that I could reasonably earmark onto this PR. Until channel state is a bit more reliable, this feature may be a bit fiddly, though the point above is the only behavior I've seen that I'd describe as a "bug."
  • This PR doesn't include the optimization of only loading newly-enabled channels when possible. I'd rather address the subtleties that come with that optimization (described in the linked issue) when state updates are a bit more coherent. (Caching may make the benefits of this optimization minimal anyways.)

@frasercl frasercl requested a review from a team as a code owner January 8, 2024 20:11
@frasercl frasercl requested review from toloudis, meganrm, blairlyons, interim17, ShrimpCryptid and ascibisz and removed request for a team January 8, 2024 20:11
@@ -513,6 +518,7 @@ export default class VolumeDrawable {
// Set the color for a channel
// @param {Array.<number>} colorrgb [r,g,b]
updateChannelColor(channelIndex: number, colorrgb: [number, number, number]): void {
console.log("update color", channelIndex, colorrgb);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentionally left in?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup looks like this could be removed

Copy link
Contributor

@ShrimpCryptid ShrimpCryptid left a comment

Choose a reason for hiding this comment

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

LGTM!

@frasercl frasercl merged commit 0b6e1c7 into main Jan 8, 2024
3 checks passed
@frasercl frasercl deleted the feature/load-enabled-channels-only branch January 8, 2024 22:35
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.

3 participants