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.
C++ prefers iterator ranges with begin() and end(), so I think I’d rather see that than IEnumerable interface-like things.
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.
So some context
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 any vtkObject*. The subclass restricts this to saying “yes, vtkObject*, but also vtkFooBar*”, 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.
1 Like
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.
vtkAbstractTransform <- vtkHomogeneousTransform <- vtkLinearTransform
^ ^ ^
| | |
vtkGeneralTransform vtkPerspectiveTransform vtkTransform
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.
This is fine. If it works, it actually is a vtkPerspectiveTransform. If not, dynamic_cast will return nullptr.
But the instance is then mutable!
I think the fundamental problem with any wrapper code is that no other languages, AFAIK, support passing const reference parameters.
If you got a vtkHomogeneousTransform*, sure. If you got const vtkHomogeneousTransform*, then you also need const_cast (with its various caveats and footnotes about its use).
The problem isn’t mutability. It’s mutability as the wrong type. A vtkPerspectiveTransform has limits on what it can represent; using it as the more general vtkHomogeneousTransform is just asking to violate those invariants.
Consider this:
class Ellipse {
double minor;
double major;
public:
Ellipse(double major, double minor) : major(major), minor(minor) {}
};
class Circle : public Ellipse {
public:
Circle(double r) : Ellipse(r, r) {}
void set_radius(double r) { major = minor = r; }
};
If Ellipse::set_minor() existed, Circle::set_minor() could be called. This…does not work well unless Ellipse knows to also update major at the same time. However, if I have Ellipse* e, dynamic_cast<Circle*>(e)->set_radius(0.1) is just fine (modulo nullptr checks).
We seem to be talking at cross purposes.
I thought the issue was having add/insert methods defined too low in the class hierarchy which allowed base types into the collection. But surely this can be handled via templating. So the only other issue is when you want to only permit iteration in some parts of the code. If the collection can be cast dynamically it becomes mutable.
Yes, but to an API that properly constrains it (or you get nullptr back on the cast to an improper type). If you get a const vtkXCollection*, no amount of casting (other than the “I’m asking for trouble const_cast” case) will get you a mutable collection.
How do you propose this should be catered for in wrapper code?
Indeed, wrappers do have decisions to make. With Python, shimming in vtkFoo_const classes probably isn’t too hard, just probably very confusing if/when backtraces end up happening. You also have this problem:
class vtkFoo_const(vtkBase):
pass # const methods
class vtkFoo_nonconst(vtkFoo_const):
pass # add in non-const methods
class vtkBar_const(vtkFoo_const):
pass
# How to do this? Mixins? But then how to make a "real" vtkFoo?
class vtkBar_nonconst(vtkBar_const, vtkFoo_nonconst):
pass
Alas, wrapping will always have some mismatches that occur. Python could probably get away with something like a metaclass like vtkConst(vtkFoo) that wraps member access as vtkConst() objects and throws on any mutable method call (const-overloads would need some assistance here).
I don’t know Java well enough to know how it’d work there.
Either way, I think if the C++ API isn’t sound, the wrappers have zero chance of getting it right. If the wrappers have something they can’t handle, then it’s an API not accessible to them. I don’t think they should have absolute veto over something, but they certainly can influence it, but also not to the point of making the C++ API a ball of barbed wire.
You do this with interfaces in Java.
So in the class diagram below the vtkAbstractContainer class would become vtkContainerInterface implemented by both vtkContainer (non-const) and vtkContainerDecorator (const). Then rinse and repeat inheriting from the const and non-const classes and introducing new interfaces.