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

fix logic for flip, remove useless member variable ( panCache ) #258

Closed
wants to merge 4 commits into from

Conversation

bionoone
Copy link
Contributor

No description provided.

@netlify
Copy link

netlify bot commented Oct 24, 2022

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit cb8997d
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/63595b2f8349d400079e5e64
😎 Deploy Preview https://deploy-preview-258--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -252,20 +252,38 @@ class Viewport implements IViewport {
vec3.scaleAndAdd(center, center, jVector, (size[1] / 2.0) * spacing[1]);
vec3.scaleAndAdd(center, center, kVector, (size[2] / 2.0) * spacing[2]);

const imageToWorld: mat4 = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not imageToWorld (since it does not have Origin in the 4th column). Maybe call it directionMatrix?

Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I see what is fixed, the demos work like before, but I wanted the flip to be based on direction which your PR does too

@bionoone
Copy link
Contributor Author

Original Unflip :

original

Original – horizontal flip:
original-hflip

Original vertical flip:

original-vflip

@sedghi
Copy link
Member

sedghi commented Oct 26, 2022

so i tried our own example, it kind of works, but not as expected

cd packages/core
yarn run example volumeAPI

When I try flippingH on Sagittal it flips but then I can't flip back. I confirm that there was a bug that you couldn't flipH on Sagittal at all, so it is progress I guess?

Axial (acquisition)

  • FlipH and FlipV works (both ways)S

Sagittal

  • FlipH works just once
  • FlipV works

Coronal

  • FlipV works just once
  • FlipH works

Note: you can try scrolling to a slice other than the middle slice of the volume to see effects of the flip better, you can use

cornerstoneTools.utilities.scroll(viewport, {delta: 30})

@sedghi
Copy link
Member

sedghi commented Oct 27, 2022

I did some digging into this, I guess the correct implementation should do it based on viewRight and viewUp, and not the direction of the image itself, since that is basically what we mean by flipHorizontal and flipVertical. I tried it

 const { focalPoint, viewPlaneNormal, viewUp } = this.getCamera();

    const viewRight = vec3.create();
    vec3.cross(viewRight, viewPlaneNormal, viewUp);

    const center = focalPoint;
    let flipHTx, flipVTx;

    const transformToOriginTx = vtkMatrixBuilder
      .buildFromRadian()
      .identity()
      .translate(-center[0], -center[1], -center[2])
      .rotateFromDirections(viewRight, [1, 0, 0])
      .rotateFromDirections(viewUp, [0, 1, 0]);

    const transformBackFromOriginTx = vtkMatrixBuilder
      .buildFromRadian()
      .identity()
      .rotateFromDirections([0, 1, 0], viewUp)
      .rotateFromDirections([1, 0, 0], viewRight)
      .translate(center[0], center[1], center[2]);

    if (flipH) {
      this.flipHorizontal = flipHorizontal;
      flipHTx = vtkMatrixBuilder
        .buildFromRadian()
        .multiply(transformToOriginTx.getMatrix())
        .scale(-1, 1, 1)
        .multiply(transformBackFromOriginTx.getMatrix());
    }

    if (flipV) {
      this.flipVertical = flipVertical;
      flipVTx = vtkMatrixBuilder
        .buildFromRadian()
        .multiply(transformToOriginTx.getMatrix())
        .scale(1, -1, 1)
        .multiply(transformBackFromOriginTx.getMatrix());
    }

It works for flipHorizontal for all views (except oblique) but not for vertical.

I guess the main issue is that flipping happens at the actor matrix level, and actor matrix is agnostic to the camera view, so that is why some orientations it works and some don't (?)

I can dig more later

@bionoone
Copy link
Contributor Author

bionoone commented Oct 27, 2022

there is a feature definition issue : the flip is about flipping the original orientation.
typical use case is for XRay where depending on what you want to do, you may want to see left & right hand in the same orientation or opposite. ( for CT / MR, the use case, is the patient is in supine/prone position. but this resolve using the rotate tool - all acquisition I ever seen are head first. no feet first ).

this is the current implementation. so if your acquisition is AXIAL, and the view is SAGITAL, horizontal flip, exchanging left and right has no effect on the (sagital) view.

Using viewup + normal would be required if you want to achieve an engineer feature : flip view in any orientation. but in this case, there is a need for a large refactor to handle it correctly. for instance you flip image, then using the crosshair you tilt the view, and then if you flip, it's along another axis plane. so in the end, you cannot have a "flip" indicator.

@sedghi
Copy link
Member

sedghi commented Oct 28, 2022

#271 draft

@sedghi
Copy link
Member

sedghi commented Nov 4, 2022

can we close this in favor of recent merged PRs?

@sedghi
Copy link
Member

sedghi commented Nov 24, 2022

I'm closing this, please re-open if needed

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

Successfully merging this pull request may close these issues.

None yet

2 participants