search

NVIDIA / ai-assisted-annotation-client

Client side integration example source code and libraries for AI-Assisted Annotation SDK
Other
307 stars 64 forks source link

Support deprecated v1 clara version for backward comptability #60

Closed SachidanandAlle closed 4 years ago

SachidanandAlle commented 4 years ago

For more details: https://github.com/NVIDIA/ai-assisted-annotation-client/issues/59#issuecomment-592296387

lassoan commented 4 years ago

Thanks for the quick update. I'll test this now.

lassoan commented 4 years ago

It works very well.

One important issue to fix: Default server name address should not be displayed in the server address combo box and should not be saved into application settings. The module should just replace empty text by the default server address, but not write it back to settings or GUI widget.

You can add this line to setupOptionsFrame method to make this clear to users:

    self.ui.serverComboBox.lineEdit().setPlaceholderText ("enter server address or leave empty to use default")

Another minor thing. Since segmentation tools are independent (they are not meant to be used together, such as starting segmentation with one tool then refine it with another), it would be better to allow opening only one section at a time, by adding something like this to setupOptionsFrame:

self.collapsibleButtonsGroup = qt.QButtonGroup()
        self.collapsibleButtonsGroup.addButton(self.ui.segmentationCollapsibleButton)
        self.collapsibleButtonsGroup.addButton(self.ui.annotationCollapsibleButton)
        self.collapsibleButtonsGroup.addButton(self.ui.deepgrowCollapsibleButton)
        self.ui.segmentationCollapsibleButton.collapsed = False  # show the first one by default; instead we could show the first one that has available models
SachidanandAlle commented 4 years ago

For default server, I guess we have to show it to end user as part of upload agreement? Not true?

https://github.com/NVIDIA/ai-assisted-annotation-client/blob/cd52c00934178a3e5371240ab5ff2b6f4a6496af/slicer-plugin/NvidiaAIAA/SegmentEditorNvidiaAIAALib/SegmentEditorEffect.py#L263-L268

For second point, segmentation & dextr3d are kind of together.. from segmentation, you get the extreme points.. and which will be your initial inputs to dextr3d.. i mean segmentation forwards it's feed to dextra3d

however deepgrow can be independent of segmentation and dextra3d.. that's one reason i tried to collapse at the start.. if we do force collapse (to switch between 1 vs 2) it might create confusion to user...

lassoan commented 4 years ago

Yes, we would still show the server address in the warning popup.

I did not know about potential cooperation between segmentation and dextr3d. I guess I'll have to try it. If the modes are not completely independent then it is probably better to leave them as is (no forcing of showing only one is needed).

SachidanandAlle commented 4 years ago

Added snapshot + steps for deepgrow in the readme.. @lassoan if all ok.. please approve..

hopefully we can publish the plugin with these changes in next nightly build