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

Similarity3DTransform::SetScale should recompute m_Offset #2629

Closed
N-Dekker opened this issue Jul 2, 2021 · 1 comment · Fixed by #2627
Closed

Similarity3DTransform::SetScale should recompute m_Offset #2629

N-Dekker opened this issue Jul 2, 2021 · 1 comment · Fixed by #2627
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances

Comments

@N-Dekker
Copy link
Contributor

N-Dekker commented Jul 2, 2021

Description

When Similarity3DTransform::SetScale is called, it does not compute m_Offset, which appears to cause an incorrect result from similarity3DTransform.TransformPoint when TransformPoint is called directly after SetScale.

template <typename TParametersValueType>
void
Similarity3DTransform<TParametersValueType>::SetScale(ScaleType scale)
{
m_Scale = scale;
this->ComputeMatrix();
}

The 2D version behaves differently: Similarity2DTransform::SetScale does compute m_Offset:

template <typename TParametersValueType>
void
Similarity2DTransform<TParametersValueType>::SetScale(ScaleType scale)
{
m_Scale = scale;
this->ComputeMatrix();
this->ComputeOffset();
}

Steps to Reproduce

The 2D transform:

const auto transform2D = itk::Similarity2DTransform<double>::New();
transform2D->SetCenter(7.0);
transform2D->SetScale(3.0);
std::cout << transform2D->TransformPoint({}) << '\n';

Prints [-14, -14], which looks fine to me. However, the 3D version:

const auto transform3D = itk::Similarity3DTransform<double>::New();
transform3D->SetCenter(7.0);
transform3D->SetScale(3.0);
std::cout << transform3D->TransformPoint({}) << '\n';

Prints [0, 0, 0], which appears wrong!

Expected behavior

In general, PointType(center + (scale * (input - center)). When the input point is entirely zero ({}), center = PointType(7.0) and scale = 3.0, I would expect PointType(7.0 + (3.0 * (0.0 - 7.0)) == PointType(-14.0).

Versions

ITK 5.2, as well as revision 895fc71 (today July 2, 2021 at the master branch)

Additional Information

It looks like this inconsistency between the 2D and the 3D version of the similarity transform has been there for a long time already! Comparing the 2D and 3D version from commit 5d9e2fc by Luis Ibanez (@luisibanez), 29 April 2005:

// Set Scale Part
template <class TScalarType>
void
Similarity2DTransform<TScalarType>
::SetScale( ScaleType scale )
{
m_Scale = scale;
this->ComputeMatrixAndOffset();
}

// Set the scale factor
template <class TScalarType>
void
Similarity3DTransform<TScalarType>
::SetScale( ScaleType scale )
{
m_Scale = scale;
this->ComputeMatrix();
}

@N-Dekker N-Dekker added the type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances label Jul 2, 2021
@dzenanz
Copy link
Member

dzenanz commented Jul 5, 2021

Oh, I guess #2627 is testing this.

N-Dekker added a commit to N-Dekker/ITK that referenced this issue Jul 8, 2021
Addressed issue InsightSoftwareConsortium#2629
"Similarity3DTransform::SetScale should recompute m_Offset", which said:

> When `Similarity3DTransform::SetScale` is called, it does not compute
> `m_Offset`, which appears to cause an incorrect result from
> `similarity3DTransform.TransformPoint` when `TransformPoint` is called
> directly after `SetScale`.

Included a unit test (GoogleTest) to ensure that the issue is fixed.
dzenanz pushed a commit that referenced this issue Jul 8, 2021
Addressed issue #2629
"Similarity3DTransform::SetScale should recompute m_Offset", which said:

> When `Similarity3DTransform::SetScale` is called, it does not compute
> `m_Offset`, which appears to cause an incorrect result from
> `similarity3DTransform.TransformPoint` when `TransformPoint` is called
> directly after `SetScale`.

Included a unit test (GoogleTest) to ensure that the issue is fixed.
thewtex pushed a commit that referenced this issue Aug 11, 2021
Addressed issue #2629
"Similarity3DTransform::SetScale should recompute m_Offset", which said:

> When `Similarity3DTransform::SetScale` is called, it does not compute
> `m_Offset`, which appears to cause an incorrect result from
> `similarity3DTransform.TransformPoint` when `TransformPoint` is called
> directly after `SetScale`.

Included a unit test (GoogleTest) to ensure that the issue is fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants