elrnv / vtkio

Visualization ToolKit (VTK) file parser and writer
Apache License 2.0
55 stars 12 forks source link

Add check for empty polydata topology #46

Closed Siimeloni closed 1 month ago

Siimeloni commented 1 month ago

When parsing a .vtp file using vtkio the program crashed with this error message:

The application panicked (crashed).
Message:  index out of bounds: the len is 0 but the index is 0
Location: /home/maik/Rust/vtkio/src/xml.rs:1569

The problem was that the data vector is always used, even if the vector is empty but still Some. This happened for my files when the different types of topology are specified in the file but don't contain any data. E.g. like this:

      <Polys>
        <DataArray type="Int64" Name="connectivity" format="ascii" RangeMin="0" RangeMax="95">
        </DataArray>
        <DataArray type="Int64" Name="offsets" format="ascii" RangeMin="28" RangeMax="96">
        </DataArray>
      </Polys>

So I added a check that also returns None if the Vectors of offset or connectivity are empty.

elrnv commented 1 month ago

Thank you for finding this issue and submitting a PR! Unfortunately this is not the right solution here. We should indeed parse the empty DataArray as an empty DataArray element. The VTKFile type is ought to represent what is in the xml as closely as possible to avoid confusion and inconsistencies. The right solution is indeed to fix the conversion to the model version, where in xml.rs at line 1569 we indeed should avoid assuming that there is at least one element and instead return an empty buf if this happens. It is also arguably a simpler solution. Please let me know if you have any questions about this change. It would also be amazing to have a test for this included in the PR.

Thanks again!

Siimeloni commented 1 month ago

Okay, thanks for the feedback. I will look into it again!

Siimeloni commented 1 month ago

So, I added that the string variable contains an empty String if data is empty. I also added a test that creates two DataArray objects. One of them is empty, the other contains data. For me the test works.

I hope I got what you meant with empty buf. Please let me know if that was not the right solution or something doesn't work.

elrnv commented 1 month ago

Yes this is perfect, thank you!