This is a request for feedback. TLDR: I am proposing that we allow extending our API to support std::vector<> based convenience methods. This is to make the C++ and Python APIs easier to use.
I am curious to know how you feel about loosening our rules for public API of VTK classes. The Python wrappers have advanced significantly and removing some of our restrictions would greatly improve our Python API (and C++ API!).
Here are two examples:
class vtkContourFilter...
const std::vector<double> GetContours();
void SetContours(const std::vector<double>& ctrs);
PS: These do not replace the existing API, just augment it.
What does this enable? This in Python:
c = vtk.vtkContourFilter(contours=[10,20]);
and this in C++:
vtkNew<vtkContourFilter> c;
c->SetContours({10, 20});
auto contours = c->GetContours();
Another example:
class vtkAlgorithm...
vtkSmartPointer<vtkDataObject> Execute(vtkDataObject* input=nullptr);
The issue that this one fixes is that because the wrapper doesn’t know that Execute() returns a new object (that it doesn’t manage), it adds a Register() call when receiving this object and this leads to the object leaking. Returning a smart pointer instead tell the wrappers to take ownership and prevent the leak.
Neither of these methods would be wrapped in Java since the Java wrapper does not get the same attention that the Python wrapper does. Since we are not replacing the API but rather augmenting it, this should not be an issue. If the VTK/Java community is interested in benefiting from the new API, some work would be necessary to support these methods. I would be interested to know if the VTK/Java community would be interested in putting the effort into this.
I’m all for it personnaly. But keep in mind that certain VTK dependent project have other wrappers that do not support std::vector. eg: Client/Server Wrapping in ParaView
If I understand you correctly, you are proposing the use of std::vector<> and no other containers, and doing this as augmentation to other methods. If that’s the case, I’m okay with it. I don’t think we are ready to start dealing with other container types, but we need to avoid becoming lazy and no longer create this augmentation pattern. However, is there a need to go into the code base and introduce std::vector<> throughout the code, what’s your take on consistency in API?
Because VTK is so array-based, augmenting methods with std::vector<> is consistent with VTK and probably quite useful. Plus, in the spirit of consistency, std::vector.data() provides another GetPointer() method, what could go wrong with that?
Also, are we going to limit the types T in std::vector to just the 13 fundamental types defined as part of the VTK API (in type.h)? And if there are constraints on T, how do we enforce this?
This raises the question as to why stop at std::vector<>? What about std::list<> and std::array<>?
Exposing STL classes is opening a can of worms in my opinion since their design represents a whole lot of public methods that are impractical to implement in wrapper code. Furthermore the currently supported wrapper languages (Java, Python and C#) do not support the concept of C++ const methods, so a VTK method returning any container will be necessarily mutable. I don’t think that is desirable.
I appreciate the syntactic sugar of this but surely it can be achieved via some C++ magic with existing VTK classes like vtkDataArray.
I like the idea of adding std:vector<> . As others said since we are augmenting the existing there shouldn’t be an API compatibility problem. If I remember correctly, the Python wrappers know how to deal with std::vector so the transition should be easy on that front.
Also, it streamlines and makes easier the serialization of an algorithms state.
That’s true but once the VTK classes are updated it would be relatively easy to update the Client/Server wrapping to accept those on top of what it accepts know . Of course this can happen independently in its own pace and only if/when needed.
Maybe we can wrap those in new macros like we do in vtkSet/Get ?
Indeed this is an issue. I suggest that the Getters return a copy in this case so that the method can remain const in C++ and avoid undesired effects. This allows also to keep the internal member variables intact. i.e. we can introduce the new API leaving internally the member as a plain array.
Thanks all. This is a great discussion. In the light of the comments made, I would like narrow my suggestion:
Let’s add support for the use of std::vector<> of primitive types to handle the use case of vector arguments that may vary in size (such as the contour values).
This is a pretty basic use case that should be fairly easy to implement for wrappers and has been a pain point. For example, the ParaView properties that deal with this kind of API have this “repeat” feature where they call n times the Set method with the first argument as the index. If I had the option of using vectors at the time, I would have and would have asked Ken Martin to add support for it in the clientserver streams wrapper.
Furthermore the currently supported wrapper languages (Java, Python and C#) do not support the concept of C++ const methods, so a VTK method returning any container will be necessarily mutable. I don’t think that is desirable.
This is a very good observation. We should only support non-const vectors that are copies. In the prototype that I have, it is indeed a copy (because the underlying data structure is different) so it doesn’t matter that it is mutable. Furthermore, the wrappers themselves make another copy since there is no way of reusing a pointer to create a list or tuple.
I appreciate the syntactic sugar of this but surely it can be achieved via some C++ magic with existing VTK classes like vtkDataArray.
We should move away from C++ magic IMO. Too much of that in VTK. Too many macros. Too many tricks to convince the wrappers to do certain things. It makes the C++ API ugly and hard to learn. We should gradually improve the wrappers and start relying on standard C++ containers and algorithms more.
I think it would be better to standardise on a VTK object for enumeration of containers. It would work for input and output in the vtkContourFilter example.
All the wrapper languages support the concept of enumerable/iterator classes/interfaces and COM provides an example of how for, foreach and while not done loops are handled by multiple .net languages in a consistent manner via the IEnumVARIANT interface.
In VTK it could look something like this
template <typename T>
class vtkEnumerable
{
public:
bool Next(const long number_to_fetch, T items[], long& number_fetched);
bool Skip(const long count);
bool Reset();
vtkEnumerable<T> Clone();
};
C++ implementations could wrap std::vector<>, std::list<>, std::array<> without loss of generality.
The wrapper knows that a method returns a new object, from the VTK_NEWINSTANCE wrapper hint.
I would be happy to see more STL in VTK API, such as std::string instead of char* or std::vector instead of C pointers.
However, vtkContourFilter’s API for setting contour values is not a good motivation for it, as that API is just messed up. To fix it, we don’t need to switch to STL but we could simply add methods to get/set the values as a vtkDoubleArray.
To make getting/setting scalar arrays more Python-friendly, I don’t think we would need STL either. Instead, the wrapper should be able to convert a vtkDoubleArray to/from a Python array. @dgobbi could confirm.
@berk.geveci It might help if you could find a number of good examples that demonstrate the value of using STL containers instead of VTK containers.
I had exactly the same thoughts, but I have convinced myself that std::vector<double> is a better choice.
The biggest problem with vtkDataArray is that it is not pass-by-value. Therefore SetContourValues() is not the only place where the values can be changed: it’s possible to call SetContourValues(array) and then modify the array. If done intentionally, it makes it harder to track the state of the filter, and if done accidentally, it causes bugs.
I prefer that vtkDataArray is used only for data and not for the settings of the filters.
We could add a new lightweight array type to VTK that enforces pass-by-value, but using std::vector<> is simpler.
The wrapper knows that a method returns a new object, from the VTK_NEWINSTANCE wrapper hint.
This is super helpful. Thank you. Much easier than using a smart pointer.
To make getting/setting scalar arrays more Python-friendly, I don’t think we would need STL either. Instead, the wrapper should be able to convert a vtkDoubleArray to/from a Python array. @dgobbi could confirm.
Hmmm. This is a possibility but it doesn’t help on the C++ side at all. If we want to make the C++ API more convenient, vector<> is the way to go. Also, returning a vtkDoubleArray would be an issue unless we wrap that in a way that has a Python container API, which is more intrusive IMO. Currently, I don’t have a better example than the variable size argument use case. I’ll think about it more.
I would be happy to see more STL in VTK API, such as std::string instead of char* or std::vector instead of C pointers.
std::string is supported for both Python & Java wrapping btw. There are some classes that use std:string in their public API. Maybe in VTK 10 (X?), we should do a big backwards compatibility break and clean it all
