Skip to content

Deal with WritePointer#3747

Closed
jiapei100 wants to merge 3 commits intoPointCloudLibrary:masterfrom
jiapei100:master
Closed

Deal with WritePointer#3747
jiapei100 wants to merge 3 commits intoPointCloudLibrary:masterfrom
jiapei100:master

Conversation

@jiapei100
Copy link

WritePointer is NOT in VTK 9.0.0 ....
I'm NOT sure if my way to replace this WritePointer is correct or not, it's just following a comment ...

@kunaltyagi
Copy link
Member

it's just following a comment

Which comment are you following?

WritePointer is NOT in VTK 9.0.0 ....

Yeah, we haven't looked at upgrading our dependencies yet.

I'm NOT sure if my way to replace this WritePointer is correct or not

The change needs to be compatible with older VTK versions too. Using macros or somehow encapsulating the modifications inside one or 2 functions (to limit the use of macros) would be the best approach.

PS: Did you test the changes on VTK 9?

@kunaltyagi kunaltyagi added changelog: enhancement Meta-information for changelog generation module: visualization needs: author reply Specify why not closed/merged yet labels Mar 13, 2020
@jiapei100
Copy link
Author

jiapei100 commented Mar 13, 2020

@kunaltyagi

1. Yeah...
The comment is :

WritePointer is NOT in VTK 9.0.0 ....

2. And, I'm updating the 2nd file now: visualization/include/pcl/visualization/pcl_visualizer.h

3. Finally, I believe the following function in file interator_style.h is wrong as well?

/** \brief Set render window. */
        void
        setRenderWindow (const vtkSmartPointer<vtkRenderWindow> &win)
        {
          win_ = win;
        }

How to carry out vtkSmartPointer assignment?

@kunaltyagi kunaltyagi added needs: feedback Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Mar 15, 2020
Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most pressing feedback is, delete all commented code you added.

In terms of the solution you provided, it looks flexible for VTK versions 6.0 up to the current nightly build.

Edit: The nightly build appears to provide an AllocateEstimate and AllocateExact methods which might emulate better the original behavior.

I need to actually dig down and come up with an MVCE to test this solution.

// Create polys from polyMesh.polygons
vtkSmartPointer<vtkCellArray> cell_array = vtkSmartPointer<vtkCellArray>::New ();
vtkIdType *cell = cell_array->WritePointer (vertices.size (), vertices.size () * (max_size_of_polygon + 1));
// vtkIdType *cell = cell_array->WritePointer (vertices.size (), vertices.size () * (max_size_of_polygon + 1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete all commented code you've introduced. Deleting those lines is perfectly ok. Apply this logic to the rest of the changes you did in this file.

Do you see the code from lines 1690-1693? Since max_size_of_polygon is no longer used, that section is also no longer required.

// Update the cells
cells = vtkSmartPointer<vtkCellArray>::New ();
vtkIdType *cell = cells->WritePointer (verts.size (), verts.size () * (max_size_of_polygon + 1));
//vtkIdType *cell = cells->WritePointer (verts.size (), verts.size () * (max_size_of_polygon + 1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as before for lines 1869-1872

#include <pcl/visualization/interactor_style.h>

// VTK includes
class vtkSmartPointerBase;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declaration is not used in this header. What made you add it?

class vtkSmartPointer;
class vtkPolyData;
class vtkTextActor;
class vtkRenderWindow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declaration is used in this header. I'm surprised the CI is not complaining 😕

@kunaltyagi kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: feedback Specify why not closed/merged yet labels Apr 3, 2020
@stale
Copy link

stale bot commented May 23, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label May 23, 2020
@larshg
Copy link
Contributor

larshg commented Feb 19, 2021

Superseded by #4262.

@larshg larshg closed this Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: enhancement Meta-information for changelog generation module: visualization needs: more work Specify why not closed/merged yet status: stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants