Skip to content

Conversation

@sadhana-r
Copy link
Contributor

@sadhana-r sadhana-r commented Dec 30, 2024

Closes #143
Closes #147
Closes #160

This PR should be merged after PR #155.

  • Create SlicerOpenLIFUPhotoscan
  • Add load photoscan button for user to select model or json file
  • Load openlifu photoscan using either json or model/texture filepaths and create SlicerOpenLIFU Photoscan object
  • Have a list of photoscans in the openlifu data module parameter node, and showing the loaded photoscans in the loaded objects view
  • Populate the list of photoscans in the transducer tracking module using these SlicerOpenLIFUPhotoscans
  • When should the texture get applied to the model node? - In the manual workflow, when a photoscan is loaded through the data module, the model and texture files are loaded into the scene and the texture gets displayed on the model node. 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, the photoscans are not loaded into the scene. They will only get loaded when the user presses a button in the transducer tracking module to 'Load Photoscan into Scene' or 'Preview photoscan'.
  • Remove 'Load Photoscan' button from transducer tracking module. This will be replaced in a separate issue with a shortcut button to 'Add Photoscan to Session' - similar in functionality to the one included in the data module.
  • Add a check to 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.
  • TODO: Update open-lifu python version. Depends on Add functionality to load photoscans OpenLIFU-python#186
  • Depends on Add functionality to load photoscan affiliated data from the database OpenLIFU-python#199. Update open-lifu python version

@sadhana-r sadhana-r force-pushed the load_slicer_photoscan branch 2 times, most recently from d7e57d7 to 74080aa Compare December 31, 2024 13:12
@sadhana-r sadhana-r force-pushed the load_slicer_photoscan branch from f0b0508 to 6b11acb Compare January 7, 2025 21:31
@sadhana-r sadhana-r marked this pull request as ready for review January 7, 2025 21:33
@ebrahimebrahim ebrahimebrahim self-requested a review January 22, 2025 15:28
Copy link
Contributor

@ebrahimebrahim ebrahimebrahim left a 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

@sadhana-r sadhana-r force-pushed the load_slicer_photoscan branch 2 times, most recently from 993c1d2 to 13714cd Compare January 30, 2025 18:51
@sadhana-r
Copy link
Contributor Author

sadhana-r commented Jan 30, 2025

I have been considering 3 scenarios for loading a photoscan:

  1. Manual loading of a photoscan.json: This is the simplest case. In this case, we can use openlifu to load an openlifu photoscan into Slicer. The openlifu photoscan includes the relative filepaths to the model and texture files, so we need to provide SlicerOpenLIFUPhotoscan.initialize_from_openlifu_photoscan both the openlifu photoscan and the directory in which the photoscan.json lives. This could be within a Database or a standalone random folder containing a json file along with .obj/.exr files. The initialized SlicerOpenLIFUPhotoscan is added to loaded_photoscans in the Data module's parameter node.
  2. Manual loading of a model and texture filepath: I went back and forth but I think I'll remove all assumptions about database organization, so if the user wants to load a file with any openlifu metadata they have to select the JSON file. Selecting an .obj file from the database will load a photoscan with ID 'vtkMMRMLModelNodeXX' and not the photoscan ID (I have modified volume loading to work the same way). In this case, a SlicerOpenLIFUPhotoscan is created using initialize_from_data_filepaths which takes the absolute model and texture filepaths, and internally creates a dummy openlifu photoscan to keep track of metadata. Once again, the initialized SlicerOpenLIFUPhotoscan is added to loaded_photoscans in the Data module's parameter node.
  3. Loading a photoscan in the transducer tracking module during an active session: When a session is loaded in the Data module, we check the photoscan_ids associated with the session and create a dictionary containing photoscan_id:photoscan_absolute_filepath_info for each photoscan_id. This is done using openlifu database operations. The resulting information gets stored in the loaded SlicerOpenLIFUSession and is used to populate the dropdown menu in the transduer tracking module. In the transducer tracking module, when a user selects a photoscan_id and wants to preview the photoscan, then an openlifu photoscan can be created directory from the stored photoscan_absolute_filepath_info using the openlifu library. photoscan_absolute_filepath_info also includes a new item photoscan_metadata_directory that can be provided to SlicerOpenLIFUPhotoscan.initialize_from_openlifu_photoscan as the parent directory. My logic here is that I can assume that the model and texture filenames in an openlifu photoscan object are relative to the photoscan_metadata_directory, but I cannot assume that the parent directory of for example model_abspath is the same as the photoscan_metadata_directory.

An alternative approach to 3) is When a session is loaded in the Data module, we check the photoscan_ids associated with the session and create a dictionary containing id: openlifu_photoscan for each photoscan_id. Then, in the transducer tracking module, when a user selects a photoscan_id and wants to preview the photoscan, the transducer tracking module accesses the logic class of the data module to interact with the Database instance and calls get_photoscan_absolute_filepath_info to get the absolute data filepaths. In this case, we could initialize SlicerOpenLIFUPhotoscan by calling initialize_from_data_filepaths. For this approach, we would need to add an optional openlifu_photoscan argument to the function, so the user can also provide the photoscan object for metadata and approval status. And the function will only create a 'dummy photoscan' if the user doesn't provide that optional photoscan i.e. in scenario (2).

**I am implementing the methods for (3) in a separate PR since it depends on updated changes to openlifu python **.

@ebrahimebrahim
Copy link
Contributor

I went back and forth but I think I'll remove all assumptions about database organization, so if the user wants to load a file with any openlifu metadata they have to select the JSON file.

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

@ebrahimebrahim
Copy link
Contributor

3. Loading a photoscan in the transducer tracking module during an active session: When a session is loaded in the Data module, we check the photoscan_ids associated with the session and create a dictionary containing photoscan_id:photoscan_absolute_filepath_info for each photoscan_id. This is done using openlifu database operations. The resulting information gets stored in the loaded SlicerOpenLIFUSession and is used to populate the dropdown menu in the transduer tracking module. In the transducer tracking module, when a user selects a photoscan_id and wants to preview the photoscan, then an openlifu photoscan can be created directory from the stored photoscan_absolute_filepath_info using the openlifu library. photoscan_absolute_filepath_info also includes a new item photoscan_metadata_directory that can be provided to SlicerOpenLIFUPhotoscan.initialize_from_openlifu_photoscan as the parent directory. My logic here is that I can assume that the model and texture filenames in an openlifu photoscan object are relative to the photoscan_metadata_directory, but I cannot assume that the parent directory of for example model_abspath is the same as the photoscan_metadata_directory.

An alternative approach to 3) is When a session is loaded in the Data module, we check the photoscan_ids associated with the session and create a dictionary containing id: openlifu_photoscan for each photoscan_id. Then, in the transducer tracking module, when a user selects a photoscan_id and wants to preview the photoscan, the transducer tracking module accesses the logic class of the data module to interact with the Database instance and calls get_photoscan_absolute_filepath_info to get the absolute data filepaths. In this case, we could initialize SlicerOpenLIFUPhotoscan by calling initialize_from_data_filepaths. For this approach, we would need to add an optional openlifu_photoscan argument to the function, so the user can also provide the photoscan object for metadata and approval status. And the function will only create a 'dummy photoscan' if the user doesn't provide that optional photoscan i.e. in scenario (2).

For (3) I am not 100% on either of the two approaches. I think to the extent possible we should be using openlifu to do the loading of photoscans and the obtaining of absolute filepaths. Maybe something like this:

  • User loads a session. The list of photoscan IDs is loaded and stored in the SlicerOpenLIFUSession so that it can be used to populate the photoscan dropdown in the TT module. So that's a List[str] attribute under SlicerOpenLIFUSession.
    • Maybe we also need photoscan names in order to make the dropdown options look prettier -- if that's the case then we use Database.load_photoscan to get the actual Photoscan objects, get the name out of them, and then save the ids and names instead of just the ids. So it'd be a List[Tuple[str,str]] type of attribute under SlicerOpenLIFUSession.
  • When the user just wants to preview a photoscan, we use Database.get_photoscan_info to get the needed model and texture filepaths and we use those just to do a preview -- not loading anything into the slicer scene and also not constructing a SlicerOpenLIFUPhotoscan
    • If the preview window has an approve button and the user toggles it, then the approve status is updated as a complete database read-and-write: read the Photoscan using Database.load_photoscan, make the change, then write it back out using Databse.write_photoscan.
      • (If the photoscan id happens to match one of the loaded SlicerOpenLIFUPhotoscans that are loaded into the slicer scene, then we need to remember to reload it after the database update)
  • Only if we enter the actual transducer tracking manual workflow (Implement end-to-end transducer tracking #149) with a chosen photoscan do we finally load a SlicerOpenLIFUPhotoscan into the scene, and it becomes a loaded openlifu object tracked under the data module's parameter node. I don't know yet what button will do that -- it's whatever button in the TT module is the first one to use to do the first step of the manual transducer tracking workflow.
    • The way the SlicerOpenLIFUPhotoscan is constructed is to use SlicerOpenLIFUPhotoscan.initialize_from_openlifu_photoscan and provide a photoscan that is loaded via Database.load_photoscan. One problem however is that you also have to provide the parent_dir and this doesn't feel right to me. This is something that we can have openlifu manage internally. I am about to make a further comment (*) below -- but basically i think SlicerOpenLIFUPhotoscan.initialize_from_openlifu_photoscan should not have to accept a parent_dir and should be able to work with a Photoscan alone assuming there's an active database connection in the data module.

Am I thinking about the role of SlicerOpenLIFUPhotoscan in the right way? Is this how you intended for it to work?


(*) photoscan.load_data_from_photoscan currently wants a parent_dir and that is the only way to get it to find and load the data. I think this is asking the user to dig too much into the internals of the database. Instead maybe it could accept a Database object itself! Then SlicerOpenLIFUPhotoscan.initialize_from_openlifu_photoscan could finally be free to work off of a Photoscan alone, relying on the Database to find and load the data.


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)

Copy link
Contributor

@ebrahimebrahim ebrahimebrahim left a 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:

image

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

@ebrahimebrahim
Copy link
Contributor

update: also tested in Slicer 5.7.0-2024-07-15 r32947 / 8c6219a -- indeed the model was not appearing textured

@sadhana-r
Copy link
Contributor Author

sadhana-r commented Feb 6, 2025

When I loaded this one using the "load photoscan" button, the texturedidn't show up as being applied to the mesh:

The way I have it structured, the texture only gets applied when the photoscan is visualized (i.e. photoscan.show_texture_on_model). Since that was removed, the model and texture are in the scene as separate nodes and the model doesn't appear textured. To apply the texture to the model node, I call self.model_node.CreateDefaultDisplayNodes() to access the model node's display node, but this toggles 'eyeballs on' for the model node. Is there a way to create/access the display node for a model node without having it rendered in the scene? If not, then as currently implemented, the only way for the user to preview the photoscan with texture, would be to click the 'Preview photoscan' button or visualize it in the scene after TT.

@sadhana-r
Copy link
Contributor Author

If the preview window has an approve button and the user toggles it, then the approve status is updated as a complete database read-and-write: read the Photoscan using Database.load_photoscan, make the change, then write it back out using Databse.write_photoscan.

  • (If the photoscan id happens to match one of the loaded SlicerOpenLIFUPhotoscans that are loaded into the slicer scene, then we need to remember to reload it after the database update)

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 _create_nodes function is a private function within SlicerOpenLIFUPhotoscan but I can make that a public funciton and create the nodes independent of a SlicerOpenLIFUPhotoscan. I'll figure out the specifics in the next PR addressing the preview.

@ebrahimebrahim
Copy link
Contributor

Summarizing our live discussion just now:

  • the issue with texture not showing up is fine -- it's intentional and not an issue. If you want you can bring back the texture and use SetVisibility(False) on the mrml node (or on its display node? not sure)
  • we talked through a way to expose getting the underlying photoscan data from openlifu-python, and then adjusting things here in this PR to not have to provide parent_dir when the database can provide the data instead. So parent_dir would be used only in the manual (file-loading, no database) way of doing things.

@sadhana-r sadhana-r force-pushed the load_slicer_photoscan branch from cdaa7aa to bb25c89 Compare February 7, 2025 20:16
@sadhana-r
Copy link
Contributor Author

sadhana-r commented Feb 7, 2025

I decide to address issue #160 in the current PR so its easier to compare the manual vs session related workflows.
I still kept the two ways of initializing a SlicerOpenLIFUPhotoscan: initialize_from_openlifu_photoscan and initialize_from_data_filepaths. I found that this was cleaner in terms of accounting for the different ways that the metadata is handled.

I now also apply the texture to the model upon loading but SetVisibility(False).

Copy link
Contributor

@ebrahimebrahim ebrahimebrahim 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, 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 😄

Copy link
Contributor

@ebrahimebrahim ebrahimebrahim left a comment

Choose a reason for hiding this comment

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

😄!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants