InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

ENH: Add a VTK-5 legacy PolyData cell-count regression test (confirm the `- 1` offsets adjustment)

Open
#6,731 0 comments 0 reactions 0 assignees View on GitHub
area:IO type:Testing
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.