Skip to content

BUG: Backport ImageAdaptor::ComputeOffset for ImageRegionIterator#6066

Open
hjmjohnson wants to merge 1 commit intoInsightSoftwareConsortium:release-5.4from
hjmjohnson:backport-image-adaptor-offset
Open

BUG: Backport ImageAdaptor::ComputeOffset for ImageRegionIterator#6066
hjmjohnson wants to merge 1 commit intoInsightSoftwareConsortium:release-5.4from
hjmjohnson:backport-image-adaptor-offset

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Cherry-pick of 1e3a883 from main (N-Dekker). Adds ComputeOffset to ImageAdaptor, fixing ImageRegionIterator support with image adaptors.

Test adapted for release-5.4: replaced itk::MakeIndexRange with ImageRegionConstIteratorWithIndex and added explicit template arguments (no CTAD on release-5.4).

Backport for #6051 (Tier 2). Locally verified: 3/3 GTests pass.

@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Apr 15, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 15, 2026

Greptile Summary

This PR backports the addition of ImageAdaptor::ComputeOffset (cherry-picked from commit 1e3a8833 on main) to the release-5.4 branch. The fix makes ImageAdaptor delegate ComputeOffset to its internal m_Image, consistent with the existing delegation pattern for ComputeIndex and GetOffsetTable. Without this method, ImageRegionConstIterator<AdaptorType>::Increment() (which calls this->m_Image->ComputeOffset(ind) via the adaptor pointer) would fall through to ImageBase::ComputeOffset, which uses a separate offset table from the adaptor's superclass instead of the internal image's. Two new GTests cover the fix, and the adaptation from main (replacing itk::MakeIndexRange with ImageRegionConstIteratorWithIndex and adding explicit template arguments) is correct for the release-5.4 toolchain constraints.

Confidence Score: 5/5

Safe to merge — correct minimal fix with adequate test coverage.

The implementation is correct, follows the existing delegation pattern in ImageAdaptor, and is verified by two new GTests. The only finding is a P2 comment typo that doesn't affect behavior.

No files require special attention.

Important Files Changed

Filename Overview
Modules/Core/ImageAdaptors/include/itkImageAdaptor.h Adds ComputeOffset inline method that delegates to m_Image->ComputeOffset, consistent with the existing delegation of ComputeIndex and GetOffsetTable — correct and minimal change.
Modules/Core/ImageAdaptors/test/itkImageAdaptorGTest.cxx Adds two GTests: one verifying ComputeOffset returns identical values to the internal image, and one verifying end-to-end ImageRegionIterator support. Contains a minor comment typo ("intenal").

Sequence Diagram

sequenceDiagram
    participant Iter as ImageRegionIterator<AdaptorType>
    participant Adaptor as ImageAdaptor
    participant Image as m_Image (TImage)

    Note over Iter,Image: Increment() — wrapping at end of row
    Iter->>Adaptor: ComputeIndex(offset)
    Adaptor->>Image: m_Image->ComputeIndex(offset)
    Image-->>Adaptor: IndexType
    Adaptor-->>Iter: IndexType

    Note over Iter,Adaptor: (before this PR) ComputeOffset fell through to ImageBase::ComputeOffset
    Iter->>Adaptor: ComputeOffset(ind)
    Adaptor->>Image: m_Image->ComputeOffset(ind)
    Image-->>Adaptor: OffsetValueType
    Adaptor-->>Iter: OffsetValueType
    Note over Iter: m_Offset, m_SpanEndOffset, m_SpanBeginOffset updated correctly
Loading

Reviews (1): Last reviewed commit: "BUG: Add ImageAdaptor::ComputeOffset, to..." | Re-trigger Greptile

Comment thread Modules/Core/ImageAdaptors/test/itkImageAdaptorGTest.cxx Outdated
@dzenanz dzenanz requested a review from N-Dekker April 15, 2026 15:27
@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Apr 15, 2026

@N-Dekker was ComputeIndex method already added to 5.4 branch? If not, we should consider doing that. In the future we will want to write code which calls it, it would be good if it compiled on 5.4 too.

@hjmjohnson hjmjohnson force-pushed the backport-image-adaptor-offset branch from 10801b6 to bcab3dc Compare April 15, 2026 17:04
@thewtex thewtex added this to the ITK 5.4.6 milestone Apr 15, 2026
Comment thread Modules/Core/ImageAdaptors/test/itkImageAdaptorGTest.cxx Outdated
@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker was ComputeIndex method already added to 5.4 branch? If not, we should consider doing that. In the future we will want to write code which calls it, it would be good if it compiled on 5.4 too.

Just for the sake of clarity, the ComputeIndex() method is really independent of this PR, which is about ComputeOffset.

ImageConstIterator::ComputeIndex() was introduced by commit 3e5a75a, so it isn't yet included with ITK 5.4. 🤷

Fixed issue InsightSoftwareConsortium#5870,
"ImageAdaptor should have its own ComputeOffset, ImageRegionIterator support
broken"

`ImageRegionConstIterator<AdapterType>::Increment()` depends on
`AdapterType::ComputeOffset`.
@hjmjohnson hjmjohnson force-pushed the backport-image-adaptor-offset branch from bcab3dc to 5ca44c9 Compare April 16, 2026 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants