# Solutions for vtkCollection API issues

**URL:** https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787
**Category:** Development
**Created:** [October 8, 2021, 2:16pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787 "2021-10-08T14:16:42Z")
**Posts on this page:** 20
**Page:** 1

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [October 8, 2021, 2:16pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/1 "2021-10-08T14:16:42Z")

</div>

Hi,

I’ve filed [an issue](https://gitlab.kitware.com/vtk/vtk/-/issues/18337) 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

Cc: @dgobbi

---

<div class="post-metadata">

### Author: ![dgobbi](https://discourse.vtk.org/user_avatar/discourse.vtk.org/dgobbi/32/18_2.png) [@dgobbi](https://discourse.vtk.org/u/dgobbi)
#### Post date: [October 8, 2021, 3:45pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/2 "2021-10-08T15:45:38Z")

</div>

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:

```auto
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.

---

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [October 8, 2021, 8:26pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/3 "2021-10-08T20:26:03Z")

</div>

> [@dgobbi](#):
>
> if we want to have container inheritance

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.

---

<div class="post-metadata">

### Author: ![dgobbi](https://discourse.vtk.org/user_avatar/discourse.vtk.org/dgobbi/32/18_2.png) [@dgobbi](https://discourse.vtk.org/u/dgobbi)
#### Post date: [October 8, 2021, 8:55pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/4 "2021-10-08T20:55:45Z")

</div>

> [@ben.boeckel](#):
>
> Unless the intermediate collection class _only_ has access methods

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](https://vtk.org/doc/nightly/html/classvtkAbstractTransform.html)… but the concept was sound, regardless of blunders in the implementation.

---

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [October 10, 2021, 3:29pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/5 "2021-10-10T15:29:19Z")

</div>

> [@dgobbi](#):
>
> 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.

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 😕 .

---

<div class="post-metadata">

### Author: ![toddy](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/t/edb3f5/32.png) [@toddy](https://discourse.vtk.org/u/toddy)
#### Post date: [October 11, 2021, 3:07am UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/6 "2021-10-11T03:07:50Z")

</div>

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?

---

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [October 11, 2021, 12:00pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/7 "2021-10-11T12:00:40Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![toddy](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/t/edb3f5/32.png) [@toddy](https://discourse.vtk.org/u/toddy)
#### Post date: [October 11, 2021, 12:41pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/8 "2021-10-11T12:41:44Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [October 11, 2021, 2:51pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/9 "2021-10-11T14:51:46Z")

</div>

C++ prefers iterator ranges with `begin()` and `end()`, so I think I’d rather see that than `IEnumerable` interface-like things.

---

<div class="post-metadata">

### Author: ![toddy](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/t/edb3f5/32.png) [@toddy](https://discourse.vtk.org/u/toddy)
#### Post date: [October 11, 2021, 10:26pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/10 "2021-10-11T22:26:18Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![ken-martin](https://discourse.vtk.org/user_avatar/discourse.vtk.org/ken-martin/32/884_2.png) [@ken-martin](https://discourse.vtk.org/u/ken-martin)
#### Post date: [October 14, 2021, 2:23pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/11 "2021-10-14T14:23:26Z")

</div>

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

> [@ben.boeckel](#):
>
> static\_cast\<U\*\>(T\*)

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.

---

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [October 14, 2021, 3:40pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/12 "2021-10-14T15:40:35Z")

</div>

> [@ken-martin](#):
>
> Assuming U\* still has the original T\* vtable I’m wondering how this will realistically cause us problems.

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.

> [@ken-martin](#):
>
> 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?

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.

> [@ken-martin](#):
>
> 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.

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).

---

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [October 14, 2021, 3:48pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/13 "2021-10-14T15:48:09Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![ken-martin](https://discourse.vtk.org/user_avatar/discourse.vtk.org/ken-martin/32/884_2.png) [@ken-martin](https://discourse.vtk.org/u/ken-martin)
#### Post date: [October 14, 2021, 4:07pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/14 "2021-10-14T16:07:21Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [October 14, 2021, 4:39pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/15 "2021-10-14T16:39:44Z")

</div>

> [@ken-martin](#):
>
> What methods that take a pair of iterators over vtkProp\* would be unsuitable for a pair of vtkActor \* iterators?

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.

> [@ken-martin](#):
>
> But semantically a list of actors is a list of props.

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.

---

<div class="post-metadata">

### Author: ![dgobbi](https://discourse.vtk.org/user_avatar/discourse.vtk.org/dgobbi/32/18_2.png) [@dgobbi](https://discourse.vtk.org/u/dgobbi)
#### Post date: [October 14, 2021, 4:55pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/16 "2021-10-14T16:55:32Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![ken-martin](https://discourse.vtk.org/user_avatar/discourse.vtk.org/ken-martin/32/884_2.png) [@ken-martin](https://discourse.vtk.org/u/ken-martin)
#### Post date: [October 14, 2021, 5:07pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/17 "2021-10-14T17:07:25Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![dgobbi](https://discourse.vtk.org/user_avatar/discourse.vtk.org/dgobbi/32/18_2.png) [@dgobbi](https://discourse.vtk.org/u/dgobbi)
#### Post date: [October 14, 2021, 5:23pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/18 "2021-10-14T17:23:20Z")

</div>

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.

---

<div class="post-metadata">

### Author: ![toddy](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/t/edb3f5/32.png) [@toddy](https://discourse.vtk.org/u/toddy)
#### Post date: [October 15, 2021, 1:28am UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/19 "2021-10-15T01:28:27Z")

</div>

> [@dgobbi](#):
>
> ```auto
> vtkAbstractTransform <- vtkHomogeneousTransform <- vtkLinearTransform
> ^ ^ ^
> | | |
> vtkGeneralTransform vtkPerspectiveTransform vtkTransform
> 
> ```

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.

 ![image](https://discourse.vtk.org/uploads/default/original/2X/f/fd3f9ec7c8640015821c21f5d0e64277edf3f5f1.png)

---

<div class="post-metadata">

### Author: ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 15, 2021, 9:00pm UTC](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787/20 "2021-11-15T21:00:46Z")

</div>

> [@toddy](#):
>
> A flaw in this is that there’s nothing to prevent `vtkHomogeneousTransform` from being dynamically cast to `vtkPerspectiveTransform` in the downstream code.

This is fine. If it works, it actually is a `vtkPerspectiveTransform`. If not, `dynamic_cast` will return `nullptr`.

[Next page](https://discourse.vtk.org/t/solutions-for-vtkcollection-api-issues/6787.md?page=2)