I’ve filed an issue outlining problems with the current set of vtkCollection APIs that are exposed. The issue states the problem and this thread is meant to discuss possible solutions (conclusions will go back to the issue).
What can’t be done
Just to lay some groundwork for what barriers definitely exist, there are some design solutions that are banned due to C++ language design.
Mainly, C++ lacks const-specific subclass casting (e.g., given class T: public U, static_cast<U*>(T*) is always just as valid as static_cast<U const*>(T const*) even though static_cast<U const*>(T*) is the only valid cast given the API), so there’s no inheritance mechanism we can do to factor out common APIs too easily. Mixins can probably help, but the wrappers will need support for that I imagine.
Checking types upon insertion
Insertion into the collection can ensure that the object to be inserted is actually of the collection’s expected type.
Pros
When inserting, the code doing the inserting can get notified (probably via a return flag) if it worked
Fast retrieval
Cons
Slower insertion
Needs a new virtual API to understand what the actual type’s required type is
Checking types upon retrieval
Instead of static_cast, GetNextX() can use vtkX::SafeDownCast to ensure that any invalid items in the collection are returned as a nullptr.
Pros
No new API required
Fast insertion
Cons
Slower retrieval
Ends iteration early due to the nullptr also being a sentinel for the end of the collection
Method name is now unclear (should GetNextActor() iterate until it finds an actor?)
Insertions can “poison the well” from “anywhere” since there’s no checking at that time
New design
vtkCollection<T> could be implemented to indicate that it is a collection over type T. This would likely be handled similarly to the vtkDataArray templates for the wrappers.
Using this, we could have the ability to cast from const vtkCollection<T>* to const vtkCollection<U>* since there is no direct inheritance that C++ would force on us. How to expose this in wrapper APIs is a separate question since there’s no wrapper overloading base on const-ness.
Of course, the type could also try to resolve the const conflation difference between vtkCollection<const T> and const vtkCollection<T> instead of (probably) only acting as if vtkCollection<T> and const vtkCollection<const T> exist.
Pros
Greenfield development, so no breaking APIs required (just a lot of deprecations)
Cons
Wrapping complexities
Need a new name since vtkCollection is already taken
Needs some const-correctness to get utility of being able to access const instances from const collections
If this is done with templates, and if we want to have container inheritance, then the base class can be supplied as a template parameter:
class vtkContainer : public vtkObject
{...};
template <class B, class T>
class vtkContainerTemplate : public B
{...};
class vtkPropContainer : public vtkContainerTemplate<vtkContainer, vtkProp>
{...};
class vtkProp3DContainer : public vtkContainerTemplate<vtkPropContainer, vtkProp3D>
{...};
Just a half-baked idea, I’m not totally sure if it’s implementable. But it would be wrappable.
I don’t think collections should inherit from each other because that exposes the AddItem(Base*) APIs. Unless the intermediate collection class only has access methods and no way to modify the container itself.
If I recall correctly, the way vtkCollections are generally used in VTK is that there is one “owner” that builds the collection, and then one or more consumers that access it.
If there inheritance between the “access only” classes, each of which would also have a subclass that adds mutator methods, then we would at least have some guarantee that “consumers” of the container would not add/remove items, and that only “owners” would have that ability.
Edit: I used a similar approach with the transform classes in VTK. The vtkLinearTransform (derives from vtkHomogeneousTransform) has the accessor methods, while its subclass vtkTransform has the mutator methods.
Of course I kind of screwed things up by adding a DeepCopy to vtkAbstractTransform at the root of the hierarchy… but the concept was sound, regardless of blunders in the implementation.
Alas, vtkPropCollection is used as a mutable argument to GetActors (and associated) methods on vtkProp. Since vtkActorCollection* can be passed here, you end up with a vtkAssembly showing up in a vtkActorCollection which is…not something one can expect to work with the given API. Of course, I would expect GetActors to return a new vtkPropCollection for better composibility, but VTK is not very consistent with that pattern .
I’m confused. Sometimes you want mutable collections which can only accept a descendant class and other times you want the collection to be immutable for consumption?
I’m saying that if we have inheriting collection classes, the base classes cannot expose mutable APIs because subclasses cannot reliably implement them. You can treat a const vtkSubCollection* as a const vtkParentCollection*, but not if it is not a const*. Since C++ does not allow one to overload inheritance-based casting based on const-ness, inheritance is the wrong tool to use here.
Then it seems like a tempated vtkCollection<T> (with no collection inheritance) is the best option. One way to avoid exposing the mutable methods could be to construct and pass around an iterator instance instead. Like the c# IEnumerator interface. I don’t know how well that sits with C++ philosophy.
Then I suppose @dgobbi’s suggestion of a vtkMutableCollection<T> subclass with add/remove methods would be the only way for that to work with wrapping.
When I though about this in the past my feeling is that we should be using std::list
e.g. I look at these questions form the perspective of if we were writing VTK from scratch right now, what would we use to store actors/props/etc and my answer is a std container (list in this case). And then work back from what the solution should be to see how/if we can get VTK there. And maybe std::list wouldn’t make sense even today. But vtkCollection was created long before std::list existed so we didn’t have that option in the past.
Given that I’m wondering what the issue is with using std::list, specifically
what is the problem use case with this? Assuming U* still has the original T* vtable I’m wondering how this will realistically cause us problems. I must be missing something here as upcasting is a core feature of OO that we have relied on for ages.
if the issue is we need to pass a std::list<vtkActor*> to a method taking std::list<vtkProp*> (and assuming c++ doesn’t handle that) then how would a new library handle that case? e.g. is there an accepted design pattern for that (such as make the new methods iterator based). If so maybe we can use that. Or maybe we add methods like std::list<vtkProp*>& getActorsAsProps(); and keep a copy list. Just trying to get some ideas/questions out there.
The problem is that, here, the base class has methods which are not valid for the base class. The base class (vtkCollection) has never been in an is-a relationship with any of its subclasses. vtkContainer has an AddItem can take anyvtkObject*. The subclass restricts this to saying “yes, vtkObject*, but alsovtkFooBar*”, so the base class provides methods which are not valid for the subclass. This is not something C++ supports (through inheritance) and should never have been done in the first place.
You don’t. A list of actors is-not-a list of props, so they’re not compatible. Proof: I can insert a vtkProp::New() in a list of props, but not a list of actors, so a list of actors is-not-a list of props.
This is closer to the proper solution: return a container of what I have; if the caller needs a refinement, std::remove_if is the tool to prune out entries which aren’t suitable (i.e., not instances of vtkActor subclasses).
Also, a std::list<T*> is terrible; a std::vector<T*> will almost always outperform it due to missing an extra pointer indirection. Any resize/reallocation mechanism is probably going to fit into a handful of cachelines and also probably not going to be that much of an issue I imagine.
Wait, a list of actors is definitely a list of props from an OO perspective. I get that a std::list of actors is not a std::list of props from a c++ type perspective and I get slicing etc. But semantically a list of actors is a list of props.
I get what you are saying, that just wasn’t what I was addressing. Assuming we used a std container, why is it a problem was what I was getting at. In my case U was vtkProp and T was vtkActor which is a is-a relationship and the container holds pointers. So my point was what methods in vtkProp are unsuitable for a vtkActor? What methods that take a pair of iterators over vtkProp* would be unsuitable for a pair of vtkActor * iterators?
std::list std::vector etc whatever one fits best based on our usage patterns. I just used list as that matches the current structure in vtkCollection.
If you’re only talking about pairs of iterators, that’s usually fine. But the interface also contains single iterator methods like replacement. Those are not safe.
No. I can insert props into a list of props that I cannot insert into a list of actors. A container is invariant over its contained type. You’re looking for a covariant type of which no STL container (AFAIK) is one.
Semantics, by default, refers to the semantics of the English language. But I digress… when I started with VTK, it had a very strong focus on OO and the fact that it was implemented in C++ seemed to be a secondary consideration. I wouldn’t mind to see VTK go back to its more “abstract” roots. Makes it easier to wrap when it’s not so C++ centric.
Of course I understand that the argument that a vtkActor container can’t be a vtkProp container is true for every OO computer language (that I’m aware of, at least). Unless the containers aren’t mutable.
Yes, it is a tradeoff. Generally I like for VTK’s interface/API to be easy to understand for someone who knows the language (C++ or python or java etc depending on the API they are using) The use of special VTKisms such as vtkAtomic when there is a very widely used std::atomic breaks that so over time we try to move to the standard to make it easier for new developers to get up to speed.
But, if the language doesn’t have a widely accepted solution that new developers would already know, then it gets tricky. There is definitely a lot of value in the “abstract” very uniform approach we used early on. I was hoping that collection was one of those places where it was a solved problem for C++ and we could just adopt that common solution. But I have run into the class hierarchy issue with containers and my solutions didn’t feel great.
I’m repeating something from earlier in this thread, but way back when I was trying for a good OO solution for transforms, I chose to make a set of abstract transform types without mutators (top row) that inherited from each other, plust a set of concrete transforms with mutators (bottom) that don’t inherit from each other.
As far as OO abstraction goes, everything worked out nicely. Implementation-wise, there was some cut-and-paste (no templates).
Interface-wise, it’s kind of nice, since VTK filters use the abstract classes (and everyone likes abstract classes for interfaces), while users instantiate the concrete types when they actually want to build a transform.
A flaw in this is that there’s nothing to prevent vtkHomogeneousTransform from being dynamically cast to vtkPerspectiveTransform in the downstream code.
So here’s an alternative using composition and the decorator pattern where the “internal” methods are virtual. When an immutable const vtkAbstractContainer& parameter is declared the wrapper code can generate a vtkContainerDecorator instead.
.