BICCN / cell-locator

manually align specimens to annotated 3D spaces
https://cell-locator.readthedocs.io
Other
19 stars 7 forks source link

WIP: Update CellLocator to use latest slicer #131

Closed allemangD closed 4 years ago

allemangD commented 4 years ago

Work in progress towards #96.


Changes in slicer are in branch cell-locator-v4.11.0-2020-07-10-b13a49465e. Similarly with branch cell-locator-2020-07-10-f0bb5d78 of CTK.

allemangD commented 4 years ago

A few things I've noticed so far that still need to be addressed:

General

CLI Arguments

Keybinds

Mouse shortcuts

jcfr commented 4 years ago

Annotations have no visual thickness.

The thickness property associated with each annotation should be used to create a model.

To create the model, the following code should be re-used:

Each time an annotation is added to the scene (NodeAddedEvent), the model should be initialized and observation should be added to call the code referenced above when the annotation is updated.

Since we the code already exists in C++, I suggest to partially re-introduce the Splines loadable module and only keep part of widget and logic. The MRML/MRMLDM/Reader part can be dropped as it has been re-implemented

Explore vs Annotate model

image

When annotation mode was toggled from explore to annotate, the annotation:

When annotate mode is enabled, moving the cursor out of the 3D and back ... expect the user to drop a point. This makes sense because the interaction mode is set to persistent (SwitchToPersistentPlaceMode()) which means that the user is expected to add an other point.

jcfr commented 4 years ago

Next, I suggest to fix the toggling Polyline vs Spline.

For reference, the property is available in Slicer here: image

Relevant code in Slicer: https://github.com/Slicer/Slicer/blob/f2c326c4d10f2cc2def302caff05faf35db356bd/Modules/Loadable/Markups/MRML/vtkMRMLMarkupsCurveNode.h#L184-L193

Once we will work on revisiting the explore vs annotate to account for the new behavior.

allemangD commented 4 years ago

A screenshot below shows the new state of the UI. Note the new "plus" button at the top - this is the new "place" mode.

I included some documentation about the semantics for the new modes in CHANGES.md. I'm unsure where a more suitable place would be - perhaps a new section in README.md?

  • The existing "Annotate" interaction mode now for editing existing control points.
  • The new "Place" interaction mode is for adding new control points.
  • The annotation snaps to the slice in both of these modes.

CellLocator-place-mode-screenshot


The spline/polyline toggle is also functioning correctly, and all models update accordingly.


With these features, the only remaining bugs are that some of the keyboard shortcuts listed in the readme are still not functioning correctly. Below are the shortcuts that still need fixing:

General:

Ctrl+W removes everything from 3d view, leaving it blank.

2D view (Annotation):

These should be available only in the new "Annotate" mode.

Delete works only after a point has been moved. Ctrl + LMB does nothing. LMB near annotation line does nothing. RMB near annotation line does nothing.


Everything else seems to be at feature parity from my testing. @jcfr do you have any thoughts?

jcfr commented 4 years ago

Looks great :+1:

After user is done adding an annotation, mode should automatically go from "The new "Place" interaction" mode to "Annotate" mode

removes everything from 3d view, leaving it blank.

It seems to work well for me. The CCF atlas is re-loaded as expected.

perhaps a new section in README.md?

I suggest to add a file called Documentation/UserGuide.md

Shortcuts, description of the modes etc ... would be moved there.

jcfr commented 4 years ago

Here is the seg fault reported on shutdown:

image

I followed this approach to generate the core dump. See https://www.slicer.org/wiki/Documentation/Nightly/Developers/Tutorials/Debug_Instructions#Analyze_a_segmentation_fault

jcfr commented 4 years ago

Updating Slicer doesn't address the crash

jcfr commented 4 years ago

I observed an issue. Here are the steps to reproduce:

  1. draw an annotation with few points
  2. switch to explore mode and move around
  3. go back to annotate

Current behavior: Annotation is snapped to the current location of slice

Expected behavior: slice should be snapped to the location of the annotation so that it can be edited

allemangD commented 4 years ago

I attempted to resolve the CHANGES.md conflict by rebasing on the updated master; however GitHub seems to still indicate there are conflicts. For now I'll focus on addressing the other bugs mentioned above instead.