Skip to content

Conversation

@IbrahimCSAE
Copy link
Collaborator

@IbrahimCSAE IbrahimCSAE commented Sep 16, 2025

Context

The old code was not determining correctly which extent dimensions to use based on the view plane normal, now its fixed to use:

  • Sagittal (looking along X): uses Y extent for width, Z extent for height
  • Coronal (looking along Y): uses X extent for width, Z extent for height
  • Axial (looking along Z): uses X extent for width, Y extent for height

@IbrahimCSAE IbrahimCSAE requested a review from sedghi September 16, 2025 20:39
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.

Looks reasonable

@IbrahimCSAE IbrahimCSAE changed the title fix(fov): fov was not working correctly for other planes [WIP - DONT MERGE] fix(fov): fov was not working correctly for other planes Sep 17, 2025
@tblock79
Copy link
Contributor

This fix seems to break the stack viewports in many cases, and I am not sure if it would work for volumes with oblique orientation.

Using the legacy FOV calculation method for volume viewports could possibly be a quick fix:

if (imageData && !useLegacyMethod && (this.type !== ViewportType.ORTHOGRAPHIC)) {
    const extent = imageData.getExtent();
    const spacing = imageData.getSpacing();
    widthWorld = (extent[1] - extent[0]) * spacing[0];
    heightWorld = (extent[3] - extent[2]) * spacing[1];
}
else {
    ({ widthWorld, heightWorld } = this._getWorldDistanceViewUpAndViewRight(bounds, viewUp, viewPlaneNormal));
}

@IbrahimCSAE
Copy link
Collaborator Author

This fix seems to break the stack viewports in many cases, and I am not sure if it would work for volumes with oblique orientation.

Using the legacy FOV calculation method for volume viewports could possibly be a quick fix:


if (imageData && !useLegacyMethod && (this.type !== ViewportType.ORTHOGRAPHIC)) {

    const extent = imageData.getExtent();

    const spacing = imageData.getSpacing();

    widthWorld = (extent[1] - extent[0]) * spacing[0];

    heightWorld = (extent[3] - extent[2]) * spacing[1];

}

else {

    ({ widthWorld, heightWorld } = this._getWorldDistanceViewUpAndViewRight(bounds, viewUp, viewPlaneNormal));

}

Yeah this is broken thats why we changed the title to WIP. We could do legacy for Volume viewport but I believe it still doesn't always make the image take the full canvas. I'm leaving it up until we figure out something consistent

@tblock79
Copy link
Contributor

That's definitely true. Using the legacy scaling as fallback might still make sense for the interim time because the current release version is often cropping a significant number of slices in volume viewports (thus, the legacy scaling should be preferable - even if not perfect).

Copy link
Member

sedghi commented Sep 25, 2025

can you send us some screenshots of how much is it missing?

@tblock79
Copy link
Contributor

Sure, here is an example (left is the current release, right is when using the legacy method as fallback for volume viewports). As you can see, a significant number of slices is outside the FOV after a camera reset. The impact might be dataset-dependent but I noticed it quite often.
image

@sedghi
Copy link
Member

sedghi commented Oct 3, 2025

I think this is related issue #2168

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.

4 participants