Rotation minimizing frames #28
Conversation
There was a problem hiding this comment.
Thank you for investigating this. If double reflection method makes any practical difference (significantly more stable, more accurate, much faster than parallel transport) then we should definitely have it. If it is not better (or somewhat worse) then we can still keep it, just so that people who are wondering if parallel transport is the best choice can do some experimentation.
| @@ -36,6 +36,7 @@ vtkParallelTransportFrame::vtkParallelTransportFrame() | |||
| this->SetTangentsArrayName("Tangents"); | |||
| this->SetNormalsArrayName("Normals"); | |||
| this->SetBinormalsArrayName("Binormals"); | |||
| this->SetRotationMinimizingFrames(false); | |||
There was a problem hiding this comment.
We initialize simple types in the .h file. Please do it for this method selection variable, too. (it will be an enum not a bool, see below).
| @@ -119,7 +120,10 @@ int vtkParallelTransportFrame::RequestData( | |||
| int numberOfCells = input->GetNumberOfCells(); | |||
| for (int cellIndex = 0; cellIndex < numberOfCells; cellIndex++) | |||
| { | |||
| this->ComputeAxisDirections(input, cellIndex, tangentsArray, normalsArray, binormalsArray); | |||
| if (this->RotationMinimizingFrames) | |||
| this->ComputeAxisDirections2(input, cellIndex, tangentsArray, normalsArray, binormalsArray); | |||
There was a problem hiding this comment.
According to VTK style, braces {} are always required in branches, even if there is a single instruction in the branch. Please add them.
| @@ -119,7 +120,10 @@ int vtkParallelTransportFrame::RequestData( | |||
| int numberOfCells = input->GetNumberOfCells(); | |||
| for (int cellIndex = 0; cellIndex < numberOfCells; cellIndex++) | |||
| { | |||
| this->ComputeAxisDirections(input, cellIndex, tangentsArray, normalsArray, binormalsArray); | |||
| if (this->RotationMinimizingFrames) | |||
| this->ComputeAxisDirections2(input, cellIndex, tangentsArray, normalsArray, binormalsArray); | |||
There was a problem hiding this comment.
Rename the methods to ComputeAxisDirectionsParallelTransport and ComputeAxisDirectionsDoubleReflection.
| @@ -26,10 +26,12 @@ | |||
| /// | |||
| /// References: | |||
| /// - Parallel transport theory: R. Bishop, "There is more than one way to frame a curve", | |||
| /// American Mathematical Monthly, vol. 82, no. 3, pp. 246�251, 1975 | |||
| /// American Mathematical Monthly, vol. 82, no. 3, pp. 246�251, 1975 | |||
There was a problem hiding this comment.
We only use ASCII characters in the code. Please replace the unicode character by simple hyphen -.
| @@ -36,6 +36,7 @@ vtkParallelTransportFrame::vtkParallelTransportFrame() | |||
| this->SetTangentsArrayName("Tangents"); | |||
| this->SetNormalsArrayName("Normals"); | |||
| this->SetBinormalsArrayName("Binormals"); | |||
| this->SetRotationMinimizingFrames(false); | |||
There was a problem hiding this comment.
Parallel transport, Double reflection method, etc. all minimize the rotation. The method name in the Wang2008 paper is "double reflection method". Please rename this class to vtkRotationMinimizingFrame and add a Method enum property (follow the VTK conventions for Get/Set method naming). Method names will be ParallelTransport and DoubleReflection. Add a deprecated vtkParallelTransfportFrame class, derived from vtkRotationMinimizingFrame (it does not add or remove any feature, just makes the vtkParallelTransfportFrame available for a while for backward compatibility).
|
|
||
| double tangent0[3] = { 0.0, 0.0, 0.0 }; | ||
| vtkIdType pointId0 = polyLine->GetPointId(0); | ||
| double pointPosition0[3]; |
There was a problem hiding this comment.
Always initialize vectors to avoid having different behavior in debug and release mode (I see that this was the original implementation, so change it there, too).
| input->GetPoint(pointId0, pointPosition0); | ||
|
|
||
| // Find tangent by direction vector by moving a minimal distance from the initial point | ||
| for (int pointIndex = 1; pointIndex < numberOfPointsInCell; pointIndex++) |
There was a problem hiding this comment.
This is just too much copy-paste from ComputeAxisDirections. Factor our the common parts into separate methods and call them from both ComputeAxisDirections variants.
| for (int pointIndex = 1; pointIndex < numberOfPointsInCell; pointIndex++) | ||
| { | ||
| vtkIdType pointId1 = polyLine->GetPointId(pointIndex); | ||
| double pointPosition1[3]; |
There was a problem hiding this comment.
Initialize to {0.0, 0.0, 0.0} unless you find that there is a measurable performance penalty.
| /// - Parallel transport implementation: Piccinelli M, Veneziani A, Steinman DA, Remuzzi A, Antiga L. | ||
| /// "A framework for geometric analysis of vascular structures: application to cerebral aneurysms.", | ||
| /// IEEE Trans Med Imaging. 2009 Aug;28(8):1141-55. doi: 10.1109/TMI.2009.2021652. | ||
| /// - Wang, W., J ̈uttler, B., Zheng, D., and Liu, Y. 2008. Computation of rotation minimizing frame. ACM Trans. Graph. 27, 1, Article 2 (March 2008), 18 pages. | ||
| /// DOI = 10.1145/1330511.1330513 http://doi.acm.org/10.1145/1330511.1330513 |
There was a problem hiding this comment.
Add open-access URL as well (https://www.microsoft.com/en-us/research/wp-content/uploads/2016/12/Computation-of-rotation-minimizing-frames.pdf)
| /// - Parallel transport implementation: Piccinelli M, Veneziani A, Steinman DA, Remuzzi A, Antiga L. | ||
| /// "A framework for geometric analysis of vascular structures: application to cerebral aneurysms.", | ||
| /// IEEE Trans Med Imaging. 2009 Aug;28(8):1141-55. doi: 10.1109/TMI.2009.2021652. | ||
| /// - Wang, W., J ̈uttler, B., Zheng, D., and Liu, Y. 2008. Computation of rotation minimizing frame. ACM Trans. Graph. 27, 1, Article 2 (March 2008), 18 pages. |
There was a problem hiding this comment.
Please describe the most important characteristics of the two methods (especially when one should be used vs. the other), do a simple performance profiling (in release mode, compute directions for many large curves and see if the difference is noticeable).
No description provided.