PERF: Let SpatialObject directly access m_ObjectToWorldTransformInverse

[N-Dekker]

Replaced this->GetObjectToWorldTransformInverse() calls with direct access to m_ObjectToWorldTransformInverse. A performance improvement greater than 6% was observed, from more than 0.74 sec. down to less than 0.69 sec, when calling IsInsideInWorldSpace 2^25 times (as tested on VS2019 Release).

[aylward]

Not ideal.

1. You are not guaranteed to have a valid inverse value unless you check the mtimes.
2. ITK's design pattern is not to directly access parent member vars for this reason. So, the pattern used here isn't consistent with the rest of ITK.

How about instead still creating a function call to access it, but eliminate the test for mtimes in that function. The function name should make it obvious that the variable is being assumed to be valid. Perhaps

GetLastValidObjectToWorldTransformInverse()

or

GetObjectToWorldTransformInverseWithoutVerification()

or

such.

[aylward]

@dzenanz - what is the recommended pattern for this situation?

[aylward]

Ah - I see this is all within the base class - not derived. So my comment on derived class isn't valid, but it still holds that the inverse might not be valid without the mtime test.

s

[N-Dekker]

Thanks for your feedback, @aylward m_ObjectToWorldTransformInverse is kept up-to-date with each SetObjectToWorldTransform call:

ITK/Modules/Core/SpatialObjects/include/itkSpatialObject.hxx

Lines 525 to 530 in d4d7419

	SpatialObject<TDimension>::SetObjectToWorldTransform(const TransformType * transform)
	{
	if (!transform->GetInverse(m_ObjectToWorldTransformInverse))
	{
	itkExceptionMacro("Transform must be invertible.");
	}

As well as by ProtectedComputeObjectToWorldTransform() (which is called by SpatialObject::Update()):

ITK/Modules/Core/SpatialObjects/include/itkSpatialObject.hxx

Lines 500 to 509 in d4d7419

	SpatialObject<TDimension>::ProtectedComputeObjectToWorldTransform()
	{
	m_ObjectToWorldTransform->SetFixedParameters(this->GetObjectToParentTransform()->GetFixedParameters());
	m_ObjectToWorldTransform->SetParameters(this->GetObjectToParentTransform()->GetParameters());
	if (this->HasParent())
	{
	m_ObjectToWorldTransform->Compose(this->GetParent()->GetObjectToWorldTransform(), false);
	}
	if (!m_ObjectToWorldTransform->GetInverse(m_ObjectToWorldTransformInverse))

That's why I think m_ObjectToWorldTransformInverse is kept up-to-date well enough 🤷

[N-Dekker]

My previous pull request proposes a different approach:

• WIP: Add SpatialObject::WorldSpaceContext class, a faster alternative to SpatialObject::IsInsideInWorldSpace #4434

One of the ideas behind PR #4434 is to take those relatively expensive GetObjectToWorldTransformInverse() calls out of the pixel-by-pixel loop, and store a pointer to that inverse transform in a local instance of the proposed WorldSpaceContext class, before going into the loop.

So this PR (#4435) kind-of competes with my other PR, #4434 😃 If IsInsideInWorldSpace would no longer call GetObjectToWorldTransformInverse(), the proposed WorldSpaceContext class would become less interesting. What do you think?

[aylward]

This is outstanding!

I like the approach in #4434 - very nice!

Perhaps just add a comment to those worldspace functions that they assume that the inverse is up to date. Then we'll cross our fingers and hope some third party hasn't done something that breaks. As you said, it should be up to date.

Thanks!

[N-Dekker]

@aylward Thanks! If this PR passes the CI, I'll add a note to each of the four adjusted member functions (DerivativeAtInWorldSpace, IsInsideInWorldSpace, IsEvaluableAtInWorldSpace, ValueAtInWorldSpace) saying something like:

This member function assumes that the internal ObjectToWorldTransformInverse transform is up-to-date. This transform may be updated explicitly by calling GetObjectToWorldTransformInverse(), Update(), or SetObjectToWorldTransform(transform)

OK?

[aylward]

Perfect!