STYLE: Change vector to point when possible#841
Conversation
SimonRit
left a comment
SimonRit
left a comment
There was a problem hiding this comment.
We should also add a note to the transition page
| Corner2[2] = this->GetInput(0)->GetOrigin()[2] + | ||
| this->GetInput(0)->GetLargestPossibleRegion().GetSize()[2] * this->GetInput(0)->GetSpacing()[2]; | ||
| itk::Point<double, 3> Corner1, Corner2; | ||
| Corner1 = itk::MakePoint( |
|
@acoussat Can you please check that this does not break your wrapping of |
|
According to the CI, |
|
I guess not, thanks! |
|
We should document the change of API in migration guide. |
axel-grc
force-pushed
the
PointType
branch
2 times, most recently
from
ca3c6df to
498fc59
Compare
| { | ||
| typename InputRegionIterator::PointType pixelPosition = itIn->GetPixelPosition(); | ||
| typename InputRegionIterator::PointType dirVox = -itIn->GetSourceToPixel(); | ||
| stepMM.Fill(0.); |
There was a problem hiding this comment.
This seems to fix the failing test that occurred because of pixelPosition that was corrected to stepMM line 422
There was a problem hiding this comment.
This change and the one line 422 should be in a separate PR. pixelPosition is used 4 times line 422, why is that? Initializing stepMM to 0 does not seem to be the correct solution to me.
axel-grc
force-pushed
the
PointType
branch
2 times, most recently
from
7300a03 to
bdd5a48
Compare
SimonRit
left a comment
SimonRit
left a comment
There was a problem hiding this comment.
Thanks. Minor suggestions except the stepMM change which shouldn't be in this PR IMO.
| cylinder->SetJ(-1.); | ||
| rtk::QuadricShape::ScalarType infDist, supDist; | ||
| auto rayOrigin = itk::MakeVector(52.43581919641932, -0.5, 998.6243011589495); | ||
| rtk::QuadricShape::ScalarType nearDist, farDist; |
There was a problem hiding this comment.
These names should not change, most likely an erroneous rebase on main.
|
|
||
| rei = REIType::New(); | ||
| REIType::VectorType semiprincipalaxis, center; | ||
| REIType::VectorType semiprincipalaxis; |
There was a problem hiding this comment.
These two can now be initialized with MakePoint and MakeVector instead of Fill
| } | ||
| q->Rotate(rot); | ||
| q->Translate(m_Center + rot * spatialOffset); | ||
| VectorType translation = m_Center.GetVectorFromOrigin() + rot * spatialOffset.GetVectorFromOrigin(); |
There was a problem hiding this comment.
I think it makes sense to change the type of spatialOffset to Vector here? Then I think you can remove the two .GetVectorFromOrigin().
| m_BoxMin -= img->GetDirection() * img->GetSpacing() * 0.5; | ||
|
|
||
| // BoxShape corner 2 | ||
| VectorType max; |
There was a problem hiding this comment.
Do you really an extra variable here?
| { | ||
| typename InputRegionIterator::PointType pixelPosition = itIn->GetPixelPosition(); | ||
| typename InputRegionIterator::PointType dirVox = -itIn->GetSourceToPixel(); | ||
| stepMM.Fill(0.); |
There was a problem hiding this comment.
This change and the one line 422 should be in a separate PR. pixelPosition is used 4 times line 422, why is that? Initializing stepMM to 0 does not seem to be the correct solution to me.
| // orthogonal to the plane defined by the detector source direction | ||
| // and the secon detector vector | ||
| PointType v; | ||
| VectorType v; |
There was a problem hiding this comment.
Maybe use MakeVector here?
| } | ||
| auto q = QuadricShape::New(); | ||
| q->SetEllipsoid(VectorType(0.), axes); | ||
| q->SetEllipsoid(itk::MakePoint(0., 0., 0.), axes); |
There was a problem hiding this comment.
Does {} work here? If yes, apply to all 0 init please.
|
Interestingly, I found a commit from 15 years ago going in the other direction. I still think it's better to stick to ITK's semantic and I believe |
Replace variables that were declared then initialized with .Fill(0) by using uniform initialization ({}) where safe.
3 participants
Close #820