Looks nice, when the dust settles, it would be nice to have an article and guidelines as to the use of vtkNew/vtkSmartPointer/vtkWeakPointer since this is now a bit dated: A Tour of VTK Pointer Classes also I attempted to introduce a section in the VTK TextBook, section: 3.11.2 VTK Pointer Classes. The original text book was written well before smart pointers so I rewrote most of the code in the PDF version of the textbook to mainly use vtkNew, sometimes vtkWeakPointer (e.g. Fig 3-25) reserving vtkSmartPointer for uses like in std::vector.
I like this comment, this is what I mean about clarifying the roles of vtkNew and vtkSmartPointer.
Agreed. Updating the docs / textbook and making a new “Best Practices” blog post is definitely in order as we iron out the remaining issues.
Python support for vtkNew is absent right now, but it would be easy to add. The wrappers just have to capture the returned vtkNew object, Get() the pointer, construct a Python object, and then let the vtkNew go out of scope. This would require around 20 new lines of code at most.
Nice discussion and work.
Are there any concrete plans to work through the code base and make everything consistent / uniform ? We seem to be good at identifying solutions but often they are only partially implemented 
which can lead to frustrated users (and developers for that matter).
1 Like
The current implementation of vtkNew and vtkSmartPointer is already complete. By “making everything consistent / uniform”, do you mean making smart pointers replace raw pointers everywhere? If so, I’ve started another discussion to go over the specific issue of using smart pointers in VTK’s API:
In a perfect world, it’d be good to start using smart pointers in our APIs. But realistically, I don’t see that happening as it would break virtual APIs and/or introduce hidden overload issues for projects that extend VTK’s classes. If we wanted to do that, we might as well rewrite VTK from the ground up and use internal shared_ptrs to manage object lifetimes, like VTK-m does.
With that effort vs. benefit ratio, I more or less expect the end result to be “leave things as they are”, but wanted to start the discussion, gauge interest, and see what possible solutions the community comes up with.
To start, rework the examples and the inner workings of classes. Many users/developers learn from these, and copy code fragments from these, and we could do better at setting a good example. The API will have to evolve if we decide to move in that direction.
Will,
The examples in the VTK Examples allude either vtkNew or vtkSmartPointer. I notice that most users posting examples do the same.
Bill
Excellent on the Examples that’s a credit to you, Andrew and many others who are keeping these polished. Grepping the internals of classes *.cxx for ::New() and doing a quick word count I get over 8500 cases and I suspect I am responsible for many :-). Some I assume may be necessary but there is certainly room for improvement.
@will.schroeder This is interesting, using something like:
find . -name *.cxx -exec grep -Hn '>::New()' {} + | wc
or
find . -name *.cxx -exec grep -Hn 'vtkNew<' {} + | wc
I get for the VTKExamples:
- vtkSmartPointer : 10256
- vtkNew : 283
- vtkNew/vtkSmartPointer: 0.03
For VTK:
- vtkSmartPointer : 6285
- vtkNew : 8150
- vtkNew/vtkSmartPointer: 1.30
A lot of the VTK examples were written before vtkNew was introduced . @lorensen and I also converted many of the older ones to using vtkSmartPointer. This explains why the ratio is much lower. However, if you look at the examples, the majority can use vtkNew in place of vtkSmartPointer.
A quick look at VTK shows that thare are still areas where vtkNew or vtkSMartPointer can replace the old
vtkObject *Obj = vtkObject::New();`
//
Obj->Delete();
pattern. These will most likely change in the future as the code evolves.
I should add that the VTK Examples are keeping up with the VTK master changes as I do a build on Windows and Linux on a mostly daily basis.
Also see my thread about refactoring examples to use more vtkNew in June 2017: https://vtk.org/pipermail/vtk-developers/2017-June/035145.html
(That was right before vtkNew got the implicit conversion to raw pointer, so my suggestion was met with some scepticism because of all the .Get() it would add)
I use a mix of both forms, for different purposes.
For “normal” use, managing a pointer:
auto mapper = vtkSmartPointer<vtkPolyDataMapper>::New();
But also use without a managing pointer:
vtkSmartPointer<vtkRenderer> renderer;
...
if (some_condition)
{
renderer = vtkSmartPointer<vtkRenderer>::New();
syncRenderers->SetRenderer(renderer);
}
This allows some processes to have a VTK renderer. The ones without have a null pointer, but in all cases I don’t have to fiddle with a Delete anywhere, since the vtkSmartPointer takes care of that for me.
/mark
I’ve started using auto in new examples:
auto actor = vtkSmartPointer::New();
Personal preference.
I prefer vtkNew when it is a local declaration as even with auto it is more compact, and more clearly conveys what I am doing:
vtkNew<vtkChartXY> chart;
versus
auto chart = vtkSmartPointer<vtkChartXY>::New()
I will admit to some bias, but think that it is both shorter and more expressive to use vtkNew in that context. If I am storing an instance I get, or working with it from a factory etc I would prefer vtkSmartPointer, i.e.
vtkSmartPointer<vtkChartXY> chart = chartFactory->createLineChart();
That is where I wonder if the vtkNew name is its downfall when using move semantics. We named it quite narrowly, and calling
vtkNew<vtkChartXY> chart = chartFactory->createLineChart();
Seems a little muddy, whereas vtkUniquePointer would be clearer in intent. I guess people can learn the nuance of what happened with vtkNew, but before it becomes too widespread it is worth considering whether the addition of something like vtkUniquePointer as a preferred factory method would be preferable to generalizing vtkNew.
When I proposed it and added it I wanted a simple equivalent to declaring a local instance.
vtkChartXY chart;
vtkNew<vtkChartXY> chart;
It was not named in the best way, or intended to fill the emerging use of unique_ptr with move semantics, factory method creation etc.
@allison.vacanti is it possible that some casts work on Windows while throw an error on Linux/Mac?
For example,
vtkWeakPointer<...> b;
…
vtkSmartPointer<… > a = b;
Seems to work fine on Windows but fails to build on Linux/Mac (https://github.com/Slicer/Slicer/commit/8fdb84d8ca10fee270688ec3a7aa10407c02b418#commitcomment-33060893). Either behavior would be OK, but it should be consistent across major compiler versions.
I’m on leave for the next 6 weeks recovering from surgery and don’t have access to my dev machine, but from what I remember, all of the smart pointers are convertible from vtkNew, but other smart pointer types need an explicit Get() to convert e.g. weak to smart. I can double check this when I get back. I’m not opposed to allowing those conversions, IIRC it was tricky to get the dependencies right while avoiding circular includes so I held off on that feature until I had more time to dig in.
As for the platform-specific behavior, I don’t think we have a lot of control over keeping things consistent across compilers for now, it sounds like some are just more permissive than others when it comes to allowing conversions.
As you pointed out, this only works in Windows MSVC:
auto pd = vtkSmartPointer<vtkPolyData>::New();
vtkWeakPointer<vtkPolyData> wppd = pd;
std::cout << wppd->GetReferenceCount() << std::endl;
vtkSmartPointer<vtkPolyData> pd1 = wppd;
std::cout << wppd->GetReferenceCount() << std::endl;
This works in Linux and MSVC:
auto pd = vtkSmartPointer<vtkPolyData>::New();
vtkWeakPointer<vtkPolyData> wppd(pd);
std::cout << wppd->GetReferenceCount() << std::endl;
vtkSmartPointer<vtkPolyData> pd1(wppd);
std::cout << wppd->GetReferenceCount() << std::endl;
A quick scan of the tests for vtkWeakPointer also shows that a constructor not assignment is used.
I am new user in VTK i have readed your discussion,it is helpful! but i can`t understand all of them .
So in brief ,if i want to create a smart pointer , use the vtknew or vtkSmartPoiner to create is the same thing?
There’s at least in most case no big difference between them ？
Use vtkNew<> whenever you create a new object.
If you need to keep a pointer to an existing object then you can use:
-
vtkSmartPointer<>- it prevents automatic deletion of the referenced object -
vtkWeakPointer<>- it does not prevent automatic deletion, but when the referenced object is deleted then the smart pointer is set to nullptr - simple object pointer - it does not prevent automatic deletion and when the referenced object is deleted then it will point to invalid memory area and crashes the application if dereferenced, so it has to be avoided (except special highly performance-critical parts of your code)
2 Likes