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

Slicing through small images does not have the right resolution #50

Open marcelluethi opened 4 years ago

marcelluethi commented 4 years ago

Problem

When a small image (i.e. one whose extend is only a few mm) is shown with ScalismoUI, slicing does not work properly. With the position slider only few slices can be shown. My guess is that this is because the position slider in the ViewportPanel2D takes only integer values, which results in that only slices which are a mm apart are shown. For small images, this is not the right behavior.

Expected behaviour:

The number of slices that can be shown should always be large enough to explore the image, irrespective of the size of the image. One possibility would be to always show at least 100 slices (and more if the extend of the sliced scene is larger than 100 mm).

Workaround

Using shift-leftclick on the orthogonal planes correctly sets the slicing position. Holding shift clicked and moving the mouse can therefore be used to move navigate through the slices.

Andreas-Forster commented 4 years ago

I had a quick look at the problem. To me, it looks at the moment, that the slider here does not care about the spacing of a volume but just uses integer values. Those are then directly translated into positions without a fractional component. No scaling or so is done when setting the location, hence the change is always at least plus or minus 1 when using the slider.

For me, it is not immediately clear how to change that. The current implementation is generally applicable. While one will find quick solutions for a displayed volume, it gets more difficult if there are two volumes displayed. What if they have different spacings, or if their slices are just minimally shifted one to the other?

As I stumbled also a few times over this issue, I would be willing to help to fix it. But I do not see an as general solution as it is implemented right now. Not even sure that there will be one. So we might even change this from 'bug' to 'behavior description'.

clangguth commented 4 years ago

Hi folks,

here's a potential quick fix for the issue, please test if this would work as you want it to. Sorry for not creating a complete pull request, instead I'll just paste the entire code below.

Note that this will also require to change the signature of the x,y,z setters in SlicingPosition.scala from Float to Double (the IDE will tell you where). And, if this works, it would make sense to implement something similar in the global slicing position as well.

class ViewportPanel2D(frame: ScalismoFrame, val axis: Axis) extends ViewportPanel(frame) {
  override def name: String = axis.toString

  private val SubDivisions: Double = 100.0

  private lazy val positionSlider = new Slider {
    peer.setOrientation(SwingConstants.VERTICAL)
  }

  lazy val positionPlusButton = new Button(new Action("+") {
    override def apply(): Unit = {
      if (positionSlider.value < positionSlider.max) {
        positionSlider.value = Math.min(positionSlider.value + SubDivisions, positionSlider.max).toInt
      }
    }
  })

  lazy val positionMinusButton = new Button(new Action("-") {
    override def apply(): Unit = {
      if (positionSlider.value > positionSlider.min) {
        positionSlider.value = Math.max(positionSlider.value - SubDivisions, positionSlider.min).toInt
      }
    }
  })

  private lazy val sliderPanel = new BorderPanel {
    layout(positionPlusButton) = BorderPanel.Position.North
    layout(positionSlider) = BorderPanel.Position.Center
    layout(positionMinusButton) = BorderPanel.Position.South
  }

  override def setupLayout(): Unit = {
    super.setupLayout()
    layout(sliderPanel) = BorderPanel.Position.East
  }

  // constructor
  border match {
    case titled: TitledBorder => titled.setTitleColor(AxisColor.forAxis(axis).darker())
    case _ => // unexpected, can't handle
  }

  rendererPanel.border = BorderFactory.createLineBorder(AxisColor.forAxis(axis), ScalableUI.scale(3))

  listenTo(frame.sceneControl.slicingPosition, positionSlider)

  def updateSliderValue(p: scalismo.geometry.Point3D): Unit = {
    val v = axis match {
      case Axis.X => p.x
      case Axis.Y => p.y
      case Axis.Z => p.z
    }
    deafTo(positionSlider)
    positionSlider.value = Math.round(v * SubDivisions).toInt
    listenTo(positionSlider)
  }

  def updateSliderMinMax(b: BoundingBox): Unit = {
    val (min, max) = axis match {
      case Axis.X => (b.xMin, b.xMax)
      case Axis.Y => (b.yMin, b.yMax)
      case Axis.Z => (b.zMin, b.zMax)
    }
    deafTo(positionSlider)
    positionSlider.min = Math.floor(min * SubDivisions).toInt
    positionSlider.max = Math.ceil(max * SubDivisions).toInt
    listenTo(positionSlider)
  }

  def sliderValueChanged(): Unit = {
    val pos = frame.sceneControl.slicingPosition
    axis match {
      case Axis.X => pos.x = positionSlider.value / SubDivisions
      case Axis.Y => pos.y = positionSlider.value / SubDivisions
      case Axis.Z => pos.z = positionSlider.value / SubDivisions
    }
  }

  reactions += {
    case SlicingPosition.event.PointChanged(_, _, current) => updateSliderValue(current)
    case SlicingPosition.event.BoundingBoxChanged(s) => updateSliderMinMax(s.boundingBox)
    case SlicingPosition.event.PerspectiveChanged(s) =>
      updateSliderMinMax(s.boundingBox)
      updateSliderValue(s.point)
    case ValueChanged(s) if s eq positionSlider => sliderValueChanged()
  }
}
Andreas-Forster commented 4 years ago

Thank you for your fast input.

I agree with you. At the very least, this is a quick fix that will last for most cases. Only the increments with the keyboard or mouse-wheel are then still limited to 1mm if I understand your solution right. But if one moves the slider, it should work most of the time until we go to data from microscopes or with very large volumes and very high resolution.

clangguth commented 4 years ago

On second thought, it might be better to have the + and - buttons actually increment/decrement in steps of 1 instead of SubDivisions, in order to allow for very fine-grained movement which might be difficult with the slider. I doubt that many people usually use those buttons anyway :-) - and of course, feel free to experiment with other "granularity" values, this was just a quick shot.

I haven't checked how the movement using keyboard and mouse is done...

Andreas-Forster commented 4 years ago

I think +-1 does not make sense. you would need to hit the keyboard/button 100times to change the position by 1mm. When you want to step through the volume of a long bone like the femur this is so slow, that I think it would be impractical. EDIT: OK, for a faster scroll through you could then use the sliders.

One question: As I am not yet familiar with the complete code of the UI, what do you mean by " implement something similar in the global slicing position as well"? The global SlicingPosition stores the position as Point[_3D] and hence use doubles. There the problem should not occur. Is there another interaction that uses integer-based values dealing with these slices?

clangguth commented 4 years ago

I was talking of the "global" SlicingPositionPanel that you see when you select the top-most "Scene" node in the tree. It has essentially the same sliders as the viewports, except for all 3 dimensions at once. If we use this solution (or something similar) it might make sense to factor out the common logic into a separate class - if possible.

Andreas-Forster commented 4 years ago

Thank you for the quick clarification. I just found the SlicingPositionPanel too right now.