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

Support overriding the current image #548

Merged
merged 2 commits into from
Feb 6, 2024
Merged

Conversation

floryst
Copy link
Collaborator

@floryst floryst commented Feb 6, 2024

The CurrentImageProvider component can now override the image returned from useCurrentImage() for a given subtree. The fallback behavior is to use the current global selected image.

This revives #410 with the fallback behavior.

Copy link

netlify bot commented Feb 6, 2024

Deploy Preview for volview-dev ready!

Name Link
🔨 Latest commit c57abd7
🔍 Latest deploy log https://app.netlify.com/sites/volview-dev/deploys/65c261cf4d9c7c00081b7b9f
😎 Deploy Preview https://deploy-preview-548--volview-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@floryst floryst force-pushed the current-image-provider branch from d16292b to edd9ac7 Compare February 6, 2024 16:30
@floryst floryst force-pushed the current-image-provider branch from edd9ac7 to c57abd7 Compare February 6, 2024 16:43
@floryst floryst added this pull request to the merge queue Feb 6, 2024
Merged via the queue into main with commit 69ddc06 Feb 6, 2024
7 checks passed
@floryst floryst deleted the current-image-provider branch March 21, 2024 16:26
@lchauvin
Copy link
Contributor

Hello,
I'm interested in using this feature, but I don't understand how it works ? How do I specify which images should be loaded by which viewer ?

Thank you.

@floryst
Copy link
Collaborator Author

floryst commented Oct 21, 2024

You can use the CurrentImageProvider as outlined below. Note that this is only most relevant for making custom changes to the app, and it does make it possible for us to support viewing multiple datasets at the same time in the future.

<current-image-provider :image-id="myImageId">
  <vtk-slice-view ...>
</current-image-provider>

@lchauvin
Copy link
Contributor

lchauvin commented Oct 23, 2024

Actually, that's what I'm trying to do (view multiple dataset). Given that the current-image-provider already exists, what is missing to be able to view multiple datasets ?

I actually tried what you mentioned, but I don't see how to change image for this vtk-slice-view then.

@PaulHax
Copy link
Collaborator

PaulHax commented Oct 23, 2024

You could write some code (pinia store?) where given a "view id" it gives you a imageId (which you feed to CurrentImageProvider of course). This repo does not have anything like that yet.

@lchauvin
Copy link
Contributor

I'm gonna look at this soon, because it's something I would need :)

This was referenced Oct 24, 2024
@lchauvin
Copy link
Contributor

lchauvin commented Nov 6, 2024

Hello,

I'm working on specifying an image for each view, so we could display multiple images at once (like to compare images, etc...).
However, I'm wondering what UI would you prefer ? Like a list on top of each view (like Slicer), or drag'n'drop images from the list to the view ? Or something else ?

@PaulHax
Copy link
Collaborator

PaulHax commented Nov 7, 2024

I want it all. =)

Methinks every user action does need some visual space holding button/select-dropdown/whatever so the user knows the action is possible. Buzzword -> discoverablity. Then we add acclerators like drag+drop and keyboard shortcuts.

@lchauvin
Copy link
Contributor

lchauvin commented Nov 7, 2024

@floryst I saw in an other post that you want to unify image list (DICOM + other images). I think that would be useful here, so we can select from all images which image to display on a specific view. Do you know if this is something that would be available soon ?

@lchauvin
Copy link
Contributor

Ok, I was trying to implement this 'multiple images' feature, but I ran into an architecture issue. If each view can have a different image, then we need to keep a mapping of which view display which images. I was working on that, but I realized this is somehow incompatible with the current architecture, where only 1 image is active at a time. So, now I'm not sure what would return useCurrentImage function, as we don't have a single primaryImageID, it depends on the view.

My idea would be to change useCurrentImage to take a view as argument and return the current image in this view (based on the mapping). But this is a significant change in the architecture, and to be honest, I don't know if I have the knowledge to change something that deep in the code.

Is it a path you are willing to go ? Otherwise, can you think about an alternative way ?

Thank you.

@floryst
Copy link
Collaborator Author

floryst commented Dec 2, 2024

This is indeed a harder problem to solve, since it affects UX and internals. I would like to go down that route, but I haven't had the time to do so. Here are some of my thoughts.

  • Having useCurrentImage accept a view ID is a starting point, since that will resolve the mapping between the view and the dataset. useCurrentImage also works within a CurrentImageProvider context, but that is less explicit than providing a view ID.
  • A lot of the sidebar expects a single dataset (e.g. the volume panel and the annotations). We could have it change based on a particular active view (e.g. clicking/highlighting the view will make it "active"), but we'll have to be careful to ensure that the tools operate on data through the view ID, and not the data directly. In any case, there are UX considerations to be made to support this.

I almost expect most of the effort to be on making the UI and UX operate smoothly for views with different datasets. I suppose my second point above would be the easiest approach to doing so, but that's from a high-level perspective.

@lchauvin
Copy link
Contributor

lchauvin commented Dec 3, 2024

Unfortunately, I don't have a deep enough understanding of everything in VolView to figure this out (not on my own at least). How should we proceed ?

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