View Issue Details [ Jump to Notes ] [ Print ]
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 
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
TagsNo tags attached.
ProjectTBD
Typecrash
Attached Filespatch file icon polydata.patch [^] (872 bytes) 2016-07-14 23:31 [Show Content]
zip file icon 16140TestCase.zip [^] (1,334 bytes) 2016-07-18 11:19
zip file icon test2.zip [^] (2,293 bytes) 2016-07-18 22:39
patch file icon polydata2.patch [^] (849 bytes) 2016-07-18 22:39 [Show Content]

 Relationships

  Notes
(0036477)
Haocheng Liu (developer)
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 (reporter)
2016-07-14 23:15

 trying to reopen...
(0036519)
edice (reporter)
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 (reporter)
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 (developer)
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 (developer)
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 (reporter)
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 (developer)
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 (reporter)
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 (developer)
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 (administrator)
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.

 Issue History
Date Modified Username Field Change
2016-05-24 05:48 edice New Issue
2016-06-20 11:36 Utkarsh Ayachit Assigned To => Haocheng Liu
2016-06-20 16:35 Haocheng Liu Status backlog => active development
2016-06-20 17:09 Haocheng Liu Status active development => backlog
2016-07-14 16:48 Haocheng Liu Note Added: 0036477
2016-07-14 16:49 Haocheng Liu Status backlog => closed
2016-07-14 16:49 Haocheng Liu Resolution open => not fixable
2016-07-14 23:15 edice Note Added: 0036517
2016-07-14 23:15 edice Status closed => backlog
2016-07-14 23:15 edice Resolution not fixable => reopened
2016-07-14 23:29 edice Note Added: 0036519
2016-07-14 23:30 edice Note Edited: 0036519
2016-07-14 23:31 edice File Added: polydata.patch
2016-07-14 23:33 edice Note Added: 0036520
2016-07-15 09:39 Haocheng Liu Product Version 6.3.0 => 5.10.1
2016-07-15 12:37 Haocheng Liu Note Added: 0036522
2016-07-18 11:19 Haocheng Liu File Added: 16140TestCase.zip
2016-07-18 11:24 Haocheng Liu Note Added: 0036524
2016-07-18 11:27 Haocheng Liu Note Edited: 0036524
2016-07-18 11:27 Haocheng Liu Note Edited: 0036524
2016-07-18 11:28 Haocheng Liu Resolution reopened => won't fix
2016-07-18 22:39 edice File Added: test2.zip
2016-07-18 22:39 edice File Added: polydata2.patch
2016-07-18 22:49 edice Note Added: 0036529
2016-07-19 10:42 Haocheng Liu Note Added: 0036530
2016-07-19 10:42 Haocheng Liu Status backlog => active development
2016-07-19 10:42 Haocheng Liu Resolution won't fix => open
2016-07-19 19:47 edice Note Added: 0036532
2016-07-20 12:24 Haocheng Liu Note Added: 0036535
2016-08-12 09:55 Kitware Robot Note Added: 0037473
2016-08-12 09:55 Kitware Robot Status active development => closed
2016-08-12 09:55 Kitware Robot Resolution open => moved

Copyright © 2000 - 2018 MantisBT Team