MantisBT - VTK
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0016140VTK(No Category)public2016-05-24 05:482016-08-12 09:55
Reporteredice 
Assigned ToHaocheng Liu 
PrioritynormalSeverityminorReproducibilityhave not tried
StatusclosedResolutionmoved 
PlatformOSOS Version
Product Version5.10.1 
Target VersionFixed in Version 
ProjectTBD
Typecrash
Summary0016140: vtkPolyData::BuildCells() should drop 1-point lines
DescriptionHi,

If a one-line line is added to vtkPolyData,
::BuildCells() will add it as a VTK_LINE cell.

This causes downstream problems, such as in
vtkBoxClipDataSet.cxx, which loads each cell into a vtkGenericCell.

The act of setting the type to VTK_LINE automatically allocates space for 2 points in the cell,
but,
only one location is loaded into the cell, since its a single-point line.

The result is a line that goes from (x,y,z) --> (0,0,0).

The points are then loaded into a Locator, which is created with the dataset bounds... and the point (0,0,0) lays outside the bounds.
This asserts() within Locator in the debug version,
and causes segfaults in the release version.

So, I figure either single-point line cells should be ignored in BuildCells(),
or ... ?

Thoughts?

cheers,
Paul
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
Attached Filespatch polydata.patch (872) 2016-07-14 23:31
https://www.vtk.org/Bug/file/10144/polydata.patch
zip 16140TestCase.zip (1,334) 2016-07-18 11:19
https://www.vtk.org/Bug/file/10146/16140TestCase.zip
zip test2.zip (2,293) 2016-07-18 22:39
https://www.vtk.org/Bug/file/10147/test2.zip
patch polydata2.patch (849) 2016-07-18 22:39
https://www.vtk.org/Bug/file/10148/polydata2.patch
Issue History
Date ModifiedUsernameFieldChange
2016-05-24 05:48ediceNew Issue
2016-06-20 11:36Utkarsh AyachitAssigned To => Haocheng Liu
2016-06-20 16:35Haocheng LiuStatusbacklog => active development
2016-06-20 17:09Haocheng LiuStatusactive development => backlog
2016-07-14 16:48Haocheng LiuNote Added: 0036477
2016-07-14 16:49Haocheng LiuStatusbacklog => closed
2016-07-14 16:49Haocheng LiuResolutionopen => not fixable
2016-07-14 23:15ediceNote Added: 0036517
2016-07-14 23:15ediceStatusclosed => backlog
2016-07-14 23:15ediceResolutionnot fixable => reopened
2016-07-14 23:29ediceNote Added: 0036519
2016-07-14 23:30ediceNote Edited: 0036519bug_revision_view_page.php?bugnote_id=36519#r1661
2016-07-14 23:31ediceFile Added: polydata.patch
2016-07-14 23:33ediceNote Added: 0036520
2016-07-15 09:39Haocheng LiuProduct Version6.3.0 => 5.10.1
2016-07-15 12:37Haocheng LiuNote Added: 0036522
2016-07-18 11:19Haocheng LiuFile Added: 16140TestCase.zip
2016-07-18 11:24Haocheng LiuNote Added: 0036524
2016-07-18 11:27Haocheng LiuNote Edited: 0036524bug_revision_view_page.php?bugnote_id=36524#r1663
2016-07-18 11:27Haocheng LiuNote Edited: 0036524bug_revision_view_page.php?bugnote_id=36524#r1664
2016-07-18 11:28Haocheng LiuResolutionreopened => won't fix
2016-07-18 22:39ediceFile Added: test2.zip
2016-07-18 22:39ediceFile Added: polydata2.patch
2016-07-18 22:49ediceNote Added: 0036529
2016-07-19 10:42Haocheng LiuNote Added: 0036530
2016-07-19 10:42Haocheng LiuStatusbacklog => active development
2016-07-19 10:42Haocheng LiuResolutionwon't fix => open
2016-07-19 19:47ediceNote Added: 0036532
2016-07-20 12:24Haocheng LiuNote Added: 0036535
2016-08-12 09:55Kitware RobotNote Added: 0037473
2016-08-12 09:55Kitware RobotStatusactive development => closed
2016-08-12 09:55Kitware RobotResolutionopen => moved

Notes
(0036477)
Haocheng Liu   
2016-07-14 16:48   
If I understand it correctly, actually in vtk&paraview you cannot create a one-line line. Can you provide some test cases or elaborate the problem more specific? Or maybe it's a problem in your file?

Currently I would just close the bug report.
(0036517)
edice   
2016-07-14 23:15   
trying to reopen...
(0036519)
edice   
2016-07-14 23:29   
(edited on: 2016-07-14 23:30)
I reported this due to a problem in the field, so yes its very easy to hit this problem.

You can make a line from any number of points.
In my case, it came from an external file, and was loaded with code that loads a batch of lines. Some of those happened to be 1-point lines.

You can build a polydata with a single-point line, something like this:

vtkIdTypeArray elements = vtkIdTypeArray::New();
vtkIdType * el = elements->WritePointer(0, 1);
el[0] = 0; // id of the single point

vtkPoints * points = vtkPoints::New();
points->SetNumberOfPoints(1);
double * pt = (double*)points->GetVoidPointer(0);
pt[0] = pt[1] = pt[2] = 10.0;

vtkPolyData * data = vtkPolyData::New();
data->SetLines(elements);
data->SetPoints(points);
data->BuildCells();

At this point, "data" now contains polydata that can cause a segmentation fault in release builds, when its run through vtkLocator which assumes 2-point lines.


Very simple to make a 1 point line by hand. Nothing in the interface prevents that sort of data from being created.

This data could be loaded in bulk, with memcpy() and other such methods. The ideal time to test is during the BuildCells() step, when it has to iterate through the cells anyway.


It would not be a problem if BuildCells does a simple check - see attached patch. Instead, BuildCells() takes data and makes it worse.
(0036520)
edice   
2016-07-14 23:33   
Note, the patch throws a logic_error so it fails fast, unwinding up the stack to be caught and reported nicely to users and developers.
This may not be VTK's way, but it works nicely for me.

Also note you will have to adjust the patch to apply to the current git head.

Thanks for looking!
(0036522)
Haocheng Liu   
2016-07-15 12:37   
Hi edice, I got your point we should throw out a warning message when reading a one-point line which is not defined in VTK. And I'm trying to reproduce the problem. However it seems that by creating the one-point line in vtkLineSource would not work because the constructor would automatically set the initial two points to [0,0,0], which would obsolete the numpts-checking in vtkPolyData::BuildCell. Can you kindly provide a test file include one-point line for me to read? Maybe reading a file would review the problem. Any idea is welcome.

Thanks and have a good weekend.
(0036524)
Haocheng Liu   
2016-07-18 11:24   
(edited on: 2016-07-18 11:27)
An easy patch for this problem is just adding a checking condition in vtkPolyData::BuildCells function, however it would greatly affect the speed. A proper solution would be adding a checking cell function in the vtkPolyData class for users to call when needed which requires further funding support. This bug is logged, but currently we would not fix it due to lack of funding.



The testCases I created is attached to the bug log.
(0036529)
edice   
2016-07-18 22:49   
Hi Lui,

I appreciate your time looking into this, I'm sorry to hear about the lack of funding.

I have attached an updated test case that shows how I improve performance by loading in bulk data with memcpy. Far faster than going through the usual interface.

In addition, I uploaded "polydata2.patch", a patch that applies to git's master branch as of today.

I see the latest code has unrolled the first iteration of the for loop, so the test for incorrect numCellPts goes into 2 places.

I used the vtkErrorMacro() for the error report, but I was looking for an error report that would stop the whole program and not allow it to continue... I am not sure which macro/thing to use.
We could also wrap the test in a #ifdef NDEBUG kind of wrapper, you'd have to advice what is the VTK equivalent of that.


I care about performance, but, I would like to see a benchmark that points to the performance problems caused by this additional test.

My intuition is that there would be minimal impact, because:

* Its checking numCellPts which is being already tested in the line
pTypes[0] = numCellPts > 2 ? VTK_POLY_LINE : VTK_LINE;
So its value would already be in the CPU L1 cache / registers.

* Its a very rare branch, so the CPU branch predictor would do very well to hide this test.

I can't see how this simple extension to an existing test would have a measurable impact ...

cheers,
Paul
(0036530)
Haocheng Liu   
2016-07-19 10:42   
Hi Edice,

I agree with your polydata2.patch, actually I was doing the same stuff when I was testing yesterday.

From my understanding, vtkErrorMacro() and vtkWarningMacro() would not stop the whole program (and they shouldn't since it would crash the program). If you would like to stop it, my suggestion is to use the exit() in your code.

The CPU L1 cache/register sounds valid. I totally agree with your point that the new if statement would be minimal impact and I would create a bug fix merge request, seeing what the other people think about.

Thanks!
Haocheng
(0036532)
edice   
2016-07-19 19:47   
Hi Haocheng,

Re exit, I wanted to follow whatever vtk policy it is when the program should halt.
A better solution may be to skip the line if an invalid number of points is detected (with an error that can be turned off) . I didn't do that in the patch just to keep it simple,but shouldn't be too hard.

While we are at it, may as well add a check for the triangle as well.not sure if triangle strip needs a check too.

Your thoughts?
Cheers!
Paul
(0036535)
Haocheng Liu   
2016-07-20 12:24   
Hi Paul,

VTK did use exit to stop the program, and there are many wild examples for reference if you type the "git grep exit" in VTK_SOURCE_DIR.

I don't think we should skip the line if an invalid number of points is detected because it would silence the problem and might make the debugging extremely hard. Personally I think it's better to warn the user and let the user decide what to do with these troublesome inputs.

I agree warnings should also be added to triangles. And for triangle strips judging from my test result, it becomes tricky because there is no clear connection with the number of points and triangles.... I would vote for just leaving it there.

Best regards
Haocheng
(0037473)
Kitware Robot   
2016-08-12 09:55   
Resolving issue as `moved`.

This issue tracker is no longer used. Further discussion of this issue may take place in the current VTK Issues page linked in the banner at the top of this page.