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.

Sign up for GitHub

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

Jump to bottom

fix(ImageResliceMapper): Axis orthogonality, Image size and texture coordinates #3064

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(ImageResliceMapper): Axis orthogonality, Image size and texture coordinates #3064

wants to merge 4 commits into from

Conversation

yarous224
Copy link
Contributor

const w2io = mat3.fromValues(image?.getDirection());

mat3.fromValuesneeds expanded array, the old version would output a matrix full of NaN here.

Context

Results

Changes

  • Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Testing

  • This change adds or fixes unit tests
  • Tested environment:
    • vtk.js:
    • OS:
    • Browser:

@finetjul finetjul requested a review from sankhesh April 29, 2024 06:01
@yarous224 yarous224 closed this May 28, 2024
@yarous224
Copy link
Contributor Author

Image Size

The polygons generated by ImageResliceMapper do not take into account the size of the pixels themselves.

This is a 512*512 spacing 0.782 image. It length is 400.384mm.

Before Change:

bb54c760cd380cfb238e11fbe86a197

After Change:

55aae1f4723109b03716f057b2a90da

@yarous224 yarous224 reopened this May 28, 2024
@yarous224 yarous224 changed the title fix(ImageResliceMapper): Axis orthogonality cannot be correctly identified when using slicePlane fix(ImageResliceMapper): Axis orthogonality, Image size and texture coordinates May 28, 2024
@yarous224
Copy link
Contributor Author

Texture coordinates

ImageResliceMapper does not account for the 0.5 offset of the texture matrix when calculating the matrix from world coordinates to texture coordinates, resulting in behavior that is inconsistent with other software (e.g., Radiant).

Before Change
image
After Change
image
Radiant
image

@floryst
Copy link
Collaborator

floryst commented May 30, 2024

@sankhesh can you take a look?

sankhesh
sankhesh requested changes May 30, 2024
View reviewed changes
Sources/Rendering/OpenGL/ImageResliceMapper/index.js
@@ -1217,7 +1221,14 @@ function vtkOpenGLImageResliceMapper(publicAPI, model) {
);
const otherAxes = [(orthoAxis + 1) % 3, (orthoAxis + 2) % 3].sort();
const dim = image.getDimensions();
const ext = [0, dim[0] - 1, 0, dim[1] - 1, 0, dim[2] - 1];
const ext = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use image.getSpatialExtent here.

Sources/Rendering/OpenGL/ImageResliceMapper/index.js
@@ -1133,17 +1138,16 @@ function vtkOpenGLImageResliceMapper(publicAPI, model) {
resGeomString = resGeomString.concat(`Plane${slicePlane.getMTime()}`);
if (image) {
resGeomString = resGeomString.concat(`Image${image.getMTime()}`);
// Check to see if we can bypass oblique slicing related bounds computation
Copy link
Collaborator

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 follow the reason for this change. Firstly, this function will not be called unless there is an input(

vtk-js/Sources/Rendering/OpenGL/ImageResliceMapper/index.js

Lines 121 to 126 in a8b7a5c

if (!model.currentInput) {
vtkErrorMacro('No input!');
return;
}
publicAPI.updateResliceGeometry();
). Secondly, even if execution gets here without an input, an xy plane should still be detected as orthonormal.

@sankhesh sankhesh mentioned this pull request Jul 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sankhesh sankhesh sankhesh requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yarous224 @floryst @sankhesh