InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
ENH: Add a VTK-5 legacy PolyData cell-count regression test (confirm the `- 1` offsets adjustment)
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
`itkVTKPolyDataMeshIO` counts VTK-5 legacy PolyData cells as `count - 1` (offsets are N+1 for N cells). The evidence says this is correct, but it has never been confirmed against a real version-5 fixture, and ITK appears to have no v5-format cell-count regression test. A change dropping the `- 1` was proposed during #6329 and deliberately excluded as a likely off-by-one regression.
The proposed-but-excluded change
During the `itkMeshFileReadWriteTestField` arm64 fix (PR #6329 — the actual bug there was `ReadCellDataBufferAsBINARY` scanning `POINT_DATA` instead of `CELL_DATA`), a secondary suggestion was raised against the version-5 branch of `ReadMeshInformation()`:
```diff
- this->m_NumberOfCells += numberOfVertices - 1; // and numberOfLines, numberOfPolygons
+ this->m_NumberOfCells += numberOfVertices;
```
It was **not applied**, and the exclusion was noted in the #6329 PR body.
Evidence the `- 1` is correct (verified against current main)
`Modules/IO/MeshVTK/src/itkVTKPolyDataMeshIO.cxx`, `m_ReadMeshVersionMajor >= 5` branch:
```cpp
// L398
this->m_NumberOfCells += numberOfVertices - 1;
// L400
EncapsulateMetaData(metaDic, "numberOfVertexOffsets", numberOfVertices);
```
The parsed value is stored under the name **`numberOfVertexOffsets`** — i.e. the code itself declares it to be an *offsets* count. In the VTK 5+ legacy format, `VERTICES n m` is followed by an `OFFSETS` array of length **N+1 for N cells**, so `numberOfVertices - 1` = N is the correct cell count. The later cell-reconstruction loops iterate `offsets.size() - 1`, consistent with the same N+1 semantics.
The pre-5 branch has no `OFFSETS` array and correctly uses the raw count with no `- 1` (L405, L451, L497) — the two branches are mutually consistent.
Same pattern at all three sites:
| Section | `- 1` site | metadata name site |
|---|---|---|
| `VERTICES` | L398 | L400 `numberOfVertexOffsets` |
| `LINES` | L444 | L446 `numberOfLinesOffsets` |
| `POLYGONS` | L490 | L492 `numberOfPolygonsOffsets` |
Suggested approach
1. Obtain or author a **VTK version-5** legacy PolyData fixture (`# vtk DataFile Version 5.x`) with small, known cell counts for `VERTICES`, `LINES`, and `POLYGONS` — e.g. 3 polygons, so `OFFSETS` has 4 entries.
2. Read it via `itkMeshFileReadWriteTest` (or a new GTest) and assert `GetNumberOfCells()` equals the *known cell count*, not the offsets count.
3. Round-trip read → write → re-read and assert cell count and connectivity are preserved.
4. Most likely outcome: close as "no change needed — the suggestion misread the offsets count as a cell count." Only if the test proves `- 1` undercounts should the code change be revisited.
5. Land the version-5 fixture as permanent regression coverage regardless of outcome; ITK currently appears to lack a v5-format cell-count test.
Related
- PR #6329 — "BUG: Fix VTKPolyDataMeshIO binary cell-data section scan (itkMeshFileReadWriteTestField)", merged 2026-05-22. The `.cxx` cell-count change was explicitly excluded there.
- FIELD-support origin commit: `e5afbec391` (2024-09-23).
Contributor guide
Research direction
Start with Modules/IO/MeshVTK/src/itkVTKPolyDataMeshIO.cxx, especially the version-5 branch and its offsets metadata, then inspect itkMeshFileReadWriteTest or the existing MeshVTK GTests. Add a small VTK version-5 legacy PolyData fixture covering known VERTICES, LINES, and POLYGONS counts, read it and round-trip it, and verify cell counts and connectivity remain correct.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- data, testing
- Issue type
- Feature
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 74/100