search

unibas-gravis / scalismo-ui

Visualization for Statistical Shape Models and Medical Images based on Scalismo.
GNU General Public License v3.0
22 stars 15 forks source link

Adding group node action setting all childrens visibility #29

Closed Andreas-Forster closed 5 years ago

Andreas-Forster commented 6 years ago

This PR addresses #28 with a new action for group nodes. This action changes the visibility of all renderable children in a group. The behavior is the same as for renderable objects. This also works for a selection of groups.

A remaining issue will be the simultaneous change of groups and renderable objects visibility at the same time.

clangguth commented 6 years ago

Looks good, just two minor nitpicks:

First, the naming is inconsistent and not really correct IMO:

https://github.com/unibas-gravis/scalismo-ui/blob/fc0f345e8a74032ebe8052e7ebb0b9853920ac50/src/main/scala/scalismo/ui/view/action/popup/ChildVisibilityAction.scala#L49

and

https://github.com/unibas-gravis/scalismo-ui/blob/fc0f345e8a74032ebe8052e7ebb0b9853920ac50/src/main/scala/scalismo/ui/view/action/popup/ChildVisibilityAction.scala#L74

should read: "Children visible in", and "Children visible". You can stick with your wording if you want, but in any case, it's always about (all) children. In the first case, there are multiple viewports, in the second case, there's a single viewport.

Second, the spacing constants should be scaled: https://github.com/unibas-gravis/scalismo-ui/blob/fc0f345e8a74032ebe8052e7ebb0b9853920ac50/src/main/scala/scalismo/ui/view/action/popup/ChildVisibilityAction.scala#L88

should be

val tb = 2.scaled
val lr = 12.scaled

(you'll need to import scalismo.ui.view.util.ScalableUI.implicits._ ).

This is an omission that's also present here, and I suggest to fix that too: https://github.com/unibas-gravis/scalismo-ui/blob/fc0f345e8a74032ebe8052e7ebb0b9853920ac50/src/main/scala/scalismo/ui/view/action/popup/VisibilityAction.scala#L89

(there might be other places somewhere in the code where I forgot to scale things)

Andreas-Forster commented 6 years ago

Fixed naming to be consistent and introduced proper scaling of the spacing (also for the VisibilityAction).

@clangguth Thank you for your fast and precise review.

clangguth commented 5 years ago

@Ghazi-Bouabene Nice catch, but I'm not sure about that bugfix. It technically solves the problem by working around it. But in fact, the problem is somewhere else: the action should not even appear for nodes where it does not make sense. IMO, the correct way would be to filter the affected nodes (throwing out empty ones), and if nothing remains, not show the action at all. That would be here: https://github.com/unibas-gravis/scalismo-ui/blob/fc0f345e8a74032ebe8052e7ebb0b9853920ac50/src/main/scala/scalismo/ui/view/action/popup/ChildVisibilityAction.scala#L34