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

WIP: add thumbnails to dicom browser #979

Closed
wants to merge 1 commit into from

Conversation

pieper
Copy link
Member

@pieper pieper commented Jun 29, 2021

@pieper pieper marked this pull request as draft June 29, 2021 00:34
@pieper
Copy link
Member Author

pieper commented Jun 29, 2021

Example usage here: https://youtu.be/JsKRAi9E8U0

@Punzo
Copy link
Contributor

Punzo commented Jan 19, 2023

Hi @pieper what is the status of this PR?

I think it could be very useful. For example the ctkDICOMThumbnailGenerator method which returns a QImage removes a performance bottleneck when rendering > 10 thumbnails at once. (Saving to png in some cases could take from 0.01 to 0.1 sec for each thumbnail).

Please let me know if I can help with anything in integrating this PR.

@pieper
Copy link
Member Author

pieper commented Jan 19, 2023

@Punzo thanks for looking at this. I think it's ready to go if we like the current functionality but we could always improve things I guess. I'd be fine with merging it now and then adding improvements in a future PR.

@Punzo
Copy link
Contributor

Punzo commented Jan 19, 2023

@Punzo thanks for looking at this. I think it's ready to go if we like the current functionality but we could always improve things I guess. I'd be fine with merging it now and then adding improvements in a future PR.

Perfect, I'll wait the merge then ! Thanks!

@pieper pieper requested review from cpinter and lassoan January 19, 2023 18:47
@pieper
Copy link
Member Author

pieper commented Jan 19, 2023

@lassoan @cpinter Can one of you have a quick look and comment if we should merge as-is to do some more tweaks?

@jcfr
Copy link
Member

jcfr commented Jan 19, 2023

Thanks @Punzo and @pieper

I will have a look tonight or tomorrow.

Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Looks great, I would just suggest a few improvements (that I implemented and tested already):

  • Make the layout horizontal (place the image on the right instead of below) as the list tends to be narrow and the tall.
  • Add an expand (double-array) button to allow show/hide of the thumbnail.
  • Performance improvement: Only update the thumbnail if it is visible. I've also experimented with delayed update of the thumbnail (only updates the thumbnail when the slider does not move for 10ms), but it made it harder to find frames with browsing, so I did not end up adding this feature.

image

@lassoan
Copy link
Member

lassoan commented Jan 20, 2023

@pieper I did not have right to push to your fork, so I've pushed it to my branch: https://github.com/lassoan/CTK/tree/browser-thumbnails

Probably you want to cherry-pick my commits from there. I've also implemented a few small issues.

@pieper
Copy link
Member Author

pieper commented Jan 20, 2023

These look great @lassoan 👍

Mine was just a WIP, so it's just as easy to make a new PR from your branch and work from here. Then I can retire mine.

@lassoan lassoan mentioned this pull request Jan 20, 2023
@lassoan
Copy link
Member

lassoan commented Jan 20, 2023

I've rebased and cleaned up the branch and submitted the new pull request: #1063, so I'm closing this one now.

@lassoan lassoan closed this Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants