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

COMP: fix incorrect casts from scalar or vector to points #479

Merged
merged 1 commit into from
Mar 1, 2022
Merged

COMP: fix incorrect casts from scalar or vector to points #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/rtkOraGeometryReader.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ OraGeometryReader::GenerateData()
MetaDataDoubleType * zvecMeta =
dynamic_cast<MetaDataDoubleType *>(dic["zdistancebaseunitcs2imagingcs_cm"].GetPointer());
double zvec = zvecMeta->GetMetaDataObjectValue();
tiltTransform->SetCenter(itk::MakeVector(0., -10. * yvec, -10. * zvec));
tiltTransform->SetCenter(itk::MakePoint(0., -10. * yvec, -10. * zvec));
Copy link

Choose a reason for hiding this comment

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

Thank you @SimonRit I hope you agree that InsightSoftwareConsortium/ITK#3237, declaring those Point(v) constructors explicit does really help to make ITK user code better. 😃 In this particular case, calling itk::MakePoint certainly looks more appropriate than calling itk::MakeVector 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly, it's not obvious to me why an itk::Point is not an itk::Vector. I have often fought against itk when using vectors, matrices and points and I'm pretty sure that there are many more places where RTK code could be improved... As far as I know, this is not well documented, but I might have missed a good explanation.

Copy link

Choose a reason for hiding this comment

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

@SimonRit Well, I guess that itk::Point and itk::Vector are just like in https://www.scratchapixel.com/lessons/mathematics-physics-for-computer-graphics/geometry/points-vectors-and-normals (but then in an N-dimensional space):

Points and Vectors
...
Here, a point is a position in a three-dimensional space. A vector, on the other hand, usually means a direction (and some corresponding magnitude, or size) in three-dimensional space.

Right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but from the same source Three-dimensional points and vectors are of course similar in that they are both represented by the aforementioned tuple notation. Which is why I would not have used two different types.

Copy link

Choose a reason for hiding this comment

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

Points and vectors are similar, but they are not the same. At least, I find it helpful to distinguish between them, by using different types for them. Even though I did not invent itk::Point and itk::Vector myself 😃


sp = tiltTransform->TransformPoint(sp);
dp = tiltTransform->TransformPoint(dp);
Expand Down
3 changes: 1 addition & 2 deletions test/rtkoratest.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@ main(int argc, char * argv[])
spacing[1] = 2.;
spacing[2] = 1.;
reader->SetSpacing(spacing);
ReaderType::OutputImagePointType origin(0.);
Copy link

Choose a reason for hiding this comment

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

@SimonRit This particular line of code still compiles with ITK_LEGACY_REMOVE=ON, right? The intention of InsightSoftwareConsortium/ITK#3237 was to avoid implicit conversion from a single value to a Point (as well as from a Vector to a Point). It should still allow an initialization of the form PointType origin(0.).

Of course, if the line of code isn't useful anymore, it's fine to me to have it removed anyway. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, line below was failing but not this one.

reader->SetOrigin(0.);
reader->SetOrigin(itk::MakePoint(0., 0., 0.));
Copy link

Choose a reason for hiding this comment

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

For the record, the following should also work fine, assuming the right PointType definition (double 3D, I guess):

reader->SetOrigin(PointType());

The coordinates of a point are properly zero-initialized by the expression PointType().

But of course, reader->SetOrigin(itk::MakePoint(0., 0., 0.)) is also OK! 😃

ReaderType::OutputImageDirectionType direction;
direction.SetIdentity();
reader->SetDirection(direction);
Expand Down