ENH: Adding itk::PolyLineCell

[PranjalSahu]

This PR adds itk::PolyLineCell.

• Implementation is similar to LineCell, except that the pointIds is a vector to support for a variable number of points.
• I am not sure about some features such as CellDimension and CellBoundaryFeature. Need comments to update it.
• Added test for PolyLineCell in the itkCellInterfaceTest.
• Also added additional test (hollow.vtk) to see if both LINE and POLYLINE Cell can come in the same mesh in any order and the reading/writing should be correct. Two straight lines are the LINE cell and the circular lines are the POLYLINE cell in the following image.
• 
• When reading from a VTK file we decide the type of cell to be inserted based on the number of points. If there are two points in a Line then LineCell is used else PolyLineCell is used.
• Older code where the polyline was splitted to create multiple (2 point) line cells is removed as that is not needed anymore. A LINE cell will only be created for 2 points cell in the vtk file.
• Comments are welcome to test different behaviors and corner cases where this can break.
• Currently tested it with a single PolyLineCell and it worked well with mesh to poly and back.

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.

[dzenanz]

Mostly looks good.

[jhlegarreta]

Thanks for having added a test. I added a few comments (I apologize if these were planned to be added later as the PR is still a draft).

[dzenanz]

Looks good so far.

[thewtex]

🥇

[jhlegarreta]

@PranjalSahu as metioned in commit a837b20, methods need to be documented in header files. Probably this is a side effect of other CellInterface-templated classes being documented in the wrong place as well.

Also, virtually all child methods seem to have the exact same documentation (have checked only a few methods), so if I'm not mistaken, if a overridden method is not documented, the parent class documentation is used for the child class. The latter should be verified, but if it is the case (should be the case for e.g. PrintSelf methods, although there are some extraneous cases #3401), and if the reimplementation does not require any custom documentation, then all method documentation should be removed. That would ease maintenance as well.

You may want to check and fix this. Thanks.

[PranjalSahu]

ok I will move them to header files.

[jhlegarreta]

While trying to have a look at why the const end iterator is not covered by tests:
https://open.cdash.org/viewCoverageFile.php?buildid=7884087&fileid=51209739

I saw that the empty cell point tests may not be working correctly, and I got a segfault for the PolyLine. When executing Modules/Core/Mesh/test/itkCellInterfaceTest.cxx, I got this in the standard output (Win 10, MSVC 2019):

-------- Vertex (VertexCell)
    Iterator test: PointIds for empty cell: 18446744073709551615,
    ConstIterator test: PointIds for empty cell: 18446744073709551615,
-------- Line (LineCell)
    Iterator test: PointIds for empty cell: 18446744073709551615,
    ConstIterator test: PointIds for empty cell: 18446744073709551615,
-------- PolyLine (PolyLineCell)

I am not sure whether having a random (or max trait for some type) is the intended behavior when there are no cells for the iterator to iterate on.

PolyLine segfaults with "vector script out of range" in itkPolyLineCell.cxx:186.

Does this deserve an issue?

[PranjalSahu]

Polyline cell can have a variable number of points. So we can't have a max trait like other cells that have a fixed number of points (ex. Vertex, Line).

And I checked the test logs for the windows test and it looks fine to me.

2086: -------- PolyLine (PolyLineCell)
2086:     Type: 10
2086:     Dimension: 1
2086:     NumberOfPoints: 0
2086:     NumberOfBoundaryFeatures:
2086:       0: 0
2086:     Iterator test: PointIds for empty cell: 
2086:     ConstIterator test: PointIds for empty cell: 
2086:     SetPointIds
2086:     ConstIterator test: PointIds for populated cell: 
2086:     Iterator test: PointIds for populated cell: 
2086:     PointIds for copied cell: 
2086: -------- PolyLineCellType with 7 vertices (PolyLineCell)
2086:     Type: 10
2086:     Dimension: 1
2086:     NumberOfPoints: 7
2086:     NumberOfBoundaryFeatures:
2086:       0: 7
2086:     Iterator test: PointIds for empty cell: 18446744073709551615, 18446744073709551615, 18446744073709551615, 18446744073709551615, 18446744073709551615, 18446744073709551615, 18446744073709551615, 
2086:     ConstIterator test: PointIds for empty cell: 18446744073709551615, 18446744073709551615, 18446744073709551615, 18446744073709551615, 18446744073709551615, 18446744073709551615, 18446744073709551615, 
2086:     SetPointIds
2086:     ConstIterator test: PointIds for populated cell: 100, 1, 2, 3, 4, 5, 6, 
2086:     Iterator test: PointIds for populated cell: 7, 8, 9, 10, 11, 12, 13, 
2086:     PointIds for copied cell: 7, 8, 9, 10, 11, 12, 13, 

But yes this could be an issue as in the code we have return &m_PointIds[0];.
I will check it.

[PranjalSahu]

In the default constructor, I will use 2 points instead of empty, because at least 2 points should be present in a PolyLine Cell.

[jhlegarreta]

In the default constructor, I will use 2 points instead of empty, because at least 2 points should be present in a PolyLine Cell.

I am fine testing with 0 points: in fact. as it happens to be an interesting case to test I'd keep it unless I'm missing something. The thing is that the rest of the *Cell classes do not segfault at that point and itk::PolyLineCell does segfault, at least in my local build.

In fact, if you happen to have a Win 10, MSVC 2019 machine at hand, it would be interesting to try to reproduce the segfault. Also, as you see, the output you print for the rest of the *Cell classes is different, so there is more going on here. Thanks.

[PranjalSahu]

Yes that is because other cells have a minimum number of points so that vector is never empty.
But in Polyline case it can be empty when constructing.

I have to check why it passes the tests in CI and in my machine.
And IMO having atleast 2 points in a PolyLine is the right approach.

Constructor for VertexCell where NumberOfPoints is constant.

VertexCell()
  {
    for (PointIdentifier i = 0; i < Self::NumberOfPoints; ++i)
    {
      m_PointIds[i] = NumericTraits<PointIdentifier>::max();
    }
  }

[jhlegarreta]

Yes that is because other cells have a minimum number of points so that vector is never empty.
But in Polyline case it can be empty when constructing.

Then the iterators (at least) in itk::PolyLineCell should protect against accessing empty lists.

[PranjalSahu]

@jhlegarreta Please check PR #3409
Inserting two points (minimum needed for a polyline cell) in the PolyLine default constructor.
Will solve the segfault problem also.

[jhlegarreta]

@jhlegarreta Please check PR #3409
Inserting two points (minimum needed for a polyline cell) in the PolyLine default constructor.
Will solve the segfault problem also.

Looks good. Thanks for doing this 💯 !

[jhlegarreta]

@PranjalSahu If the list of point ids can be cleared (have not checked), there should probably exist an Initialize method that is called both from the constructor and the clearing method where the block that you added should dwell (for the sake of code re-use).

Minor comment concerning the PR:

• Using the class name (i.e. without the empty space before Cell) might help in searches.
• Adding a message body (besides the subject line) is always helpful for additional explanations/reviewing/reading commit history.