Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
marcelluethi opened this issue Jun 8, 2020 · 7 comments
Open

Comments

@marcelluethi
Copy link
Contributor

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
Copy link
Member

Andreas-Forster commented Jun 9, 2020

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
Copy link
Member

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
Copy link
Member

Andreas-Forster commented Jun 9, 2020

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
Copy link
Member

clangguth commented Jun 9, 2020

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
Copy link
Member

Andreas-Forster commented Jun 9, 2020

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants