-
Notifications
You must be signed in to change notification settings - Fork 5
Create SlicerOpenLIFUPhotoscan and implement manual workflow #158
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
Conversation
d7e57d7 to
74080aa
Compare
f0b0508 to
6b11acb
Compare
ebrahimebrahim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big effort and thank you!
The major difficulty I think is the way SlicerOpenLIFU messes around with the openlifu database internals when it should really be relying on openlifu for that.
So there's some reorganization needed on that front I think.
A few other items listed below.
In the treatment workflow, the dropdown bar shows all the photoscans that are in the 'loaded_photoscans' parameter node in a manual workflow, or all the photoscans associated with a session, if there is an active session
When there is an active session that has photoscans I am not seeing all the photoscans associated with the session. For example, after loading the mannequin session, I still get "No Photoscan objects" in the transducer tracking module photoscan dropdown menu.
I think it will be commit OpenwaterHealth/OpenLIFU-python@9a1590d that you add to the python requirements, just noting here since that's what I have been testing with.
Also remember to adapt to the changes that were made after the review of OpenwaterHealth/OpenLIFU-python#186
E.g. the import of photoscan is now different, etc.
Before the next review you can rebase to main
993c1d2 to
13714cd
Compare
|
I have been considering 3 scenarios for loading a photoscan:
An alternative approach to 3) is When a session is loaded in the Data module, we check the **I am implementing the methods for (3) in a separate PR since it depends on updated changes to openlifu python **. |
I am glad you chose this approach for (2)! It makes a lot of sense and the implementation looks really clean now I am thinking about (3) still... rest of review coming soon |
For (3) I am not 100% on either of the two approaches. I think to the extent possible we should be using
Am I thinking about the role of (*) It's very possible I have gotten myself confused and I'm making it worse so please let me know 😁 (Actual final review coming soon when I finish testing things out) |
ebrahimebrahim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super nice and much cleaner now!
The things I tested worked really well.
There's still just this uncertainty I have around SlicerOpenLIFUPhotoscan messing around with a parent_dir, from my long comment just above. Can you let me know what you think on that? If there's a way to get out of having to do that, perhaps by first exposing some additional thing from the OpenLIFU-python side, then I think we should do that before wrapping up this PR.
When I loaded this one using the "load photoscan" button, the texturedidn't show up as being applied to the mesh:
This is on slicer 5.9.0-2025-02-04 r33461 / 0ceb67e which I recently upgraded to so maybe it was working before and stopped -- not sure
|
update: also tested in Slicer 5.7.0-2024-07-15 r32947 / 8c6219a -- indeed the model was not appearing textured |
The way I have it structured, the texture only gets applied when the photoscan is visualized (i.e. |
This makes sense. The role of the SlicerOpenLIFUPhotoscan is essentially to group together an openlifu photoscan object with the corresponing texture and model nodes. Assuming that I can preview a model/texture node in a separate window without loading them into the Slicer scene (or maybe by making them hidden nodes?), then I can preview photoscans without actually creating a SlicerOpenLIFUPhotoscan object. Currently the |
|
Summarizing our live discussion just now:
|
cdaa7aa to
bb25c89
Compare
|
I decide to address issue #160 in the current PR so its easier to compare the manual vs session related workflows. I now also apply the texture to the model upon loading but |
ebrahimebrahim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you for all the work on this!
Only left minor suggestions as I was reading through and testing.
For the next small update please add commits on top without rebasing to help me finish the last review quickly 😄
ebrahimebrahim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄!

Closes #143
Closes #147
Closes #160
This PR should be merged after PR #155.
OnNodeAboutToBeRemoved. When the model or texture node associated with a SlicerOpenLIFUPhotoscan object is deleted, the associated photoscan object should be removed. Similar to how transducers work.