COMP: Fix cast filter proxy pixel writes

[nilason]

Handle output iterators that return pixel proxies instead of lvalue references by copying the pixel, updating its components, and assigning it back through the iterator.

Addresses compilation error (attempting to build OrfeoToolBox v10 against ITK 6.0 beta2):

In file included from /dev/OTB/Modules/StereoProcessing/Applications/app/otbStereoRectificationGridGenerator.cxx:28:
In file included from /opt/local/include/ITK-6.0/itkCastImageFilter.h:155:
/opt/local/include/ITK-6.0/itkCastImageFilter.hxx:167:84: error: non-const lvalue reference to type 'itk::Vector<double, 2>' cannot bind to an initializer list temporary
  167 |     std::conditional_t<isVariableLengthVector, OutputPixelType, OutputPixelType &> outputPixel{ *outputIt };
      |                                                                                    ^          ~~~~~~~~~~~~~

Note: this is created with assistance of Codex, as I'm not familiar with ITK code. This solves the compilation error, but needs to be reviewed with that in mind.

PR Checklist

• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)
• Added test (or behavior not changed)
• Updated API documentation (or API not changed)
• Added license to new files (if any)
• Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
• Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

[greptile-apps]

Confidence Score: 4/5

Safe to merge; adding a focused test would protect the new proxy-output behavior from regressions.

Findings

1.  Cover proxy output path ▶

Summary

The update adds support for proxy-backed output pixels in CastImageFilter. One non-blocking coverage gap remains: the new copy-modify-assign path has no direct regression test.

_{Reviews (1) · Last reviewed commit: "COMP: Fix cast filter proxy pixel writes"}

[greptile-apps]

Cover proxy output path

The new copy-modify-assign path is not exercised by the direct CastImageFilter tests. Ordinary Image outputs dereference to references, and VectorImage uses VariableLengthVector, so both select the earlier branch instead. Add an adaptor-backed, non-VariableLengthVector proxy-output test that runs the filter and verifies every converted component. This is non-blocking, but without it a regression can reintroduce a proxy-write compilation or conversion failure without detection.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Artifacts

Proxy branch evidence script

• The executed script searches the relevant tests and records iterator facts and unavailable test tools, providing a repeatable verification command.

Direct cast test coverage inspection

• Executed repository inspection shows the changed copy-modify-assign branch, direct tests limited to VectorImage and ordinary Image outputs, and no direct CastImageFilter/ImageAdaptor instantiation; the branch lacks an exact test.

Iterator branch and environment inspection

• Executed inspection proves VectorImage is a proxy but VariableLengthVector case and records absent CMake, Pixi, CTest, and test driver; no runtime result can be inferred.

_{Ran code and verified through T-Rex}

[greptile-apps]

Comments Outside Diff

These findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.

•  No existing test covers CastImageFilter's non-VariableLengthVector proxy-output branch ▶

  • Bug
    • Modules/Filtering/ImageFilterBase/include/itkCastImageFilter.hxx:175-184 is selected only when decltype(*outputIt) is not a reference and OutputPixelType is not VariableLengthVector. Existing direct tests cover ordinary Image outputs (reference dereference) and VectorImage outputs (non-reference proxy, but VariableLengthVector pixel type), both of which select the line-166 arm instead. No direct CastImageFilter/ImageAdaptor test instantiation was found.
  • Cause
    • The test matrix has no output image type that produces a writable proxy while exposing a non-VariableLengthVector pixel type, such as a suitable ImageAdaptor output.
  • Fix
    • Add a focused CastImageFilter test using an adaptor-backed output with a fixed-size/vector-like external pixel type. Include compile-time assertions that decltype(*ImageRegionRange<OutputImageType>::iterator) is not a reference and OutputImageType::PixelType is not VariableLengthVector; run the filter and verify every converted component, thereby exercising the copy-modify-assign path.

[N-Dekker]

Looks promising, thanks @nilason, but we would really need to have this case added to our unit tests!

[N-Dekker]

It looks like this statement in the else clause is the only fundamental difference between the if and the else clause. (Right?) If that is indeed the case, I think it would be better to merge the proposed if and the else clauses, and have a single if, saying something like:

if constexpr (<this problematic case>)
{
  *outputIt = outputPixel;
}

[nilason]

There are differences also before the (repetitive) for-loop. But an if-else-clause on either side would also work.

[nilason]

A possible rewrite might look like:

    if constexpr (std::is_reference_v<OutputPixelReferenceType> || isVariableLengthVector)
    {
      std::conditional_t<isVariableLengthVector, OutputPixelType, OutputPixelType &> outputPixel{ *outputIt };
    }
    else
    {
      OutputPixelType outputPixel{ *outputIt };
    }

    for (unsigned int k = 0; k < componentsPerPixel; ++k)
    {
      outputPixel[k] = static_cast<OutputPixelValueType>(inputPixel[k]);
    }

    if constexpr (!std::is_reference_v<OutputPixelReferenceType> && !isVariableLengthVector))
    {
      *outputIt = outputPixel;
    }

[hjmjohnson]

Agreed the duplication is worth removing — I looked at merging into a single if constexpr and the clean way to do it changes the control flow more than a one-line fixup should (the two branches produce a reference vs. a value with different lifetimes, so a shared declaration needs its own follow-up). Left as-is in #6901 to keep that PR to the fix plus the test; happy to open a separate cleanup PR if you'd still like it done here.

[blowekamp]

What are the template parameters to itk::CastImageFilter that cased the issue?

[nilason]

Looks promising, thanks @nilason, but we would really need to have this case added to our unit tests!

I'm too unfamiliar with ITK do implement tests, but feel free to add test to this PR or separately (in which case I can rebase this).

[nilason]

What are the template parameters to itk::CastImageFilter that cased the issue?

It is a simple #include "itkCastImageFilter.h" that is the immediate cause (built with Apple clang version 21.0.0), see PR description above.

[nilason]

What are the template parameters to itk::CastImageFilter that cased the issue?

It is a simple #include "itkCastImageFilter.h" that is the immediate cause (built with Apple clang version 21.0.0), see PR description above.

The file is https://gitlab.orfeo-toolbox.org/orfeotoolbox/otb-modules/stereoprocessing/-/blob/main/Applications/app/otbStereoRectificationGridGenerator.cxx

[hjmjohnson]

Thank you for tracking this down and reporting it, @nilason — the reproduction steps and OTB source pointer made this easy to root-cause. Since it needed a test and I'm not sure how comfortable you are writing one, I've opened #6901 with your fix plus a regression test and the root-cause answer to @blowekamp's question, crediting you as co-author. Your branch here is untouched.

Happy to keep discussing here if anything about the approach in #6901 needs adjusting.

[hjmjohnson]

itk::CastImageFilter<itk::VectorImage<float,2>, otb::Image<itk::Vector<double,2>,2>> — matching the otb::Image<DisplacementType> (DisplacementType = itk::Vector<double,2>) cast from FloatVectorImageType in otbStereoRectificationGridGenerator.cxx.

The general trigger is any subclass of itk::Image used as the output, with a fixed-size (non-VariableLengthVector) pixel type: itk::Image declares AccessorFunctorType = DefaultPixelAccessorFunctor<Self> with Self fixed to Image<TPixel,VDim> itself, so a subclass that doesn't redeclare it inherits the base's version. ImageBufferRange's direct-pixel-access check then fails for the subclass, and *outputIt returns a proxy value instead of a reference. otb::Image is exactly such a subclass.

Detail and a reproducer with no OTB dependency in #6901, which also adds the test @N-Dekker asked for.

[nilason]

Thank you all for digging into this! As maintainer you could probably have continued with this PR, pushing commits to my PR branch (the "Allow edits and access to secrets by maintainers" checkbox is ticked) and I wouldn't have mind. Please continue with #6901, I'm glad to help if I can. I can always test with OTB again.