InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
Investigate adopting factoryless instance creation for BoundingBox, VectorContainer and ScalableAffineTransform (carried downstream by MITK)
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
MITK ships a private ITK branch consisting of upstream `v5.4.6` plus **one commit** that switches `itk::BoundingBox`, `itk::VectorContainer` and `itk::ScalableAffineTransform` from `itkNewMacro` to `itkFactorylessNewMacro`. It has never been proposed here. Should ITK adopt it?
To be clear up front: this is **not** a report of an ignored contribution — no PR, issue, or Discourse topic for it exists. It is a downstream-held patch that looks upstreamable, and this issue is to decide whether it is.
Provenance
MITK's `CMakeExternals/ITK.cmake` pins:
```
GIT_REPOSITORY https://github.com/MITK/ITK.git
GIT_TAG 2d98b3bdbe663a03f335939696f7ec70e21603b1 # branch: v5.4.6-patched
```
`git rev-list --count v5.4.6..2d98b3bd` on that fork is **1**:
- "Use factoryless instance creation for frequently used basic types"
- Stefan Dinkelacker (DKFZ / MITK), 2025-05-10
- 4 insertions, 3 deletions, 3 files
The diff in full:
```diff
Modules/Core/Common/include/itkBoundingBox.h
- itkNewMacro(Self);
+ itkFactorylessNewMacro(Self);
Modules/Core/Common/include/itkVectorContainer.h
- itkNewMacro(Self);
+ itkFactorylessNewMacro(Self);
Modules/Core/Transform/include/itkScalableAffineTransform.h
- itkNewMacro(Self);
+ itkFactorylessNewMacro(Self);
+ itkCloneMacro(Self);
```
As of `main` (2026-07-29) all three still use `itkNewMacro(Self)`.
Why it looks upstreamable
**Established pattern.** 64 headers under `Modules/**/include/` already use `itkFactorylessNewMacro`, including `itkMetaDataObject.h` in Core/Common.
**No override conflicts.** Searching `Modules/**` for factory overrides of these three types returns zero for all three.
**Transform IO is unaffected** — the obvious objection for `ScalableAffineTransform`, which *is* registered (`Modules/IO/TransformFactory/src/itkTransformFactoryBaseRegister.hxx:148`). `TransformFactory::RegisterTransform()` keys on `T::New()->GetTransformTypeAsString()`, i.e. `"ScalableAffineTransform_double_3_3"`, whereas `itkNewMacro`'s `New()` looks up the bare class name. Different keys; making `New()` factoryless does not disturb the reader path.
Related prior work in this area: #4453, #4481.
Open questions for reviewers
1. **Benchmark.** No measured benefit has been published. What is the actual gain, and on what workload? `VectorContainer` and `BoundingBox` are plausible hot-path allocations; a number should precede a merge.
2. **Behavior change.** This removes the `ObjectFactory` substitution hook for three public classes. An external consumer who registered an override for them would silently stop receiving it. Is that acceptable under ITK's compatibility policy for `main`, and does it warrant a release note?
3. **Scope.** The `itkCloneMacro(Self)` addition to `ScalableAffineTransform` is an unrelated feature (adds `Clone()`), and should probably be a separate commit if this is pursued.
4. **Coverage.** If the pattern is right for these three, are there other frequently instantiated basic types that warrant the same treatment? A principled sweep may beat a three-file patch.
5. **Targeting.** If accepted, `main` seems the right target rather than a `release-5.4.x` backport, since it is an optimization with a behavior change rather than a bug fix. Worth confirming.
How this surfaced
Found while adding MITK as a headless downstream consumer to a local ITK build-matrix testbed (MITK's `PythonWheel` build configuration, configured against a locally built ITK via `ITK_DIR`). Inspecting MITK's ITK pin surfaced the one-commit delta.
Credit for the patch belongs to Stefan Dinkelacker and the MITK team; I have no connection to it beyond noticing it. If there is interest here, they would be the right people to ask about motivation and measurements.
Contributor guide
Research direction
Start with itkBoundingBox.h, itkVectorContainer.h, and itkScalableAffineTransform.h, comparing their current itkNewMacro usage with existing itkFactorylessNewMacro examples such as itkMetaDataObject.h. Review itkTransformFactoryBaseRegister.hxx and the related issues #4453 and #4481, then benchmark the affected allocations and assess the ObjectFactory compatibility change. Done means a documented decision on scope, targeting, compatibility, and whether a focused upstream change is warranted.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100