ArrayPlug improvements #6015
Merged
ArrayPlug improvements #6015
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fix error when resizing removed a plug with an input connection. In this case, the "resize when inputs change" behaviour was kicking in during the outer resize and messing everything up. - Fix failure to resize output plugs, due to attempting to add an input plug as a child.
A minimum size of 0 is not currently allowed, and will be silently clamped to 1. But we're about to allow it, and for backwards compatibility we need to keep `Switch` creating arrays of the same size.
Also remove unused imports from CreateViewsTest.
4 tasks
We achieve this by explicitly storing a prototype for the elements rather than using the first child as the prototype. At the same time, we require all clients to pass the prototype to the constructor, so that we are always in a valid state following construction. The serialisation now also correctly passes the prototype on construction, and uses `resize()` to add any required children instead of using a series of `addChild()` calls as before. This reflects how we want people to use the API, and makes for a more compact serialisation too. The one wrinkle in this is support for legacy serialisations, which didn't provide an element prototype to the constructor, and used `addChild()` instead of `resize()`. We continue to support these with some special cases internally, whereby `m_elementPrototype` isn't initialised until the first child is added.
`ArrayPlugSerialiser::childNeedsConstruction()` return false now, so the existence of the Dynamic flag on its children is irrelevant. We're one step closer to eventually getting rid of the flag entirely.
Our `startup/Gaffer/arrayPlugCompatibility.py` shim was keeping these working, but we want to provide a good example.
Passing `nullptr` is no longer allowed except to support the loading of legacy files. Unfortunately, all client code that is adding views is doing it via `addChild( NameValuePlug( ... ) )` rather than `resize()` or `next()`, meaning the elements are not forced to match the prototype exactly. And all existing serialisations are doing the same. And because `NameValuePlug( "left", ... )` produces a plug with a default value of "left", the array elements being created are not homogeneous. That breaks the new ArrayPlug serialisation, which assumes that a simple `resize()` is sufficient to recreate all children. We deal with this using a compatibility shim that detects the bad child additions and resets the default value while maintaining the current value.
And outline a possible path forwards. In practice, we're currently getting away with the violations because the serialisations for these nodes all call `addQuery()` to resize the arrays upfront, so that the arrays already have the required size before the `resize()` call is executed.
johnhaddon
force-pushed
the
arrayPlugElements
branch
from
9cbde14
to
cd70463
Compare
|
I've looked through this code fairly carefully, and it all looks good to me. The main concern with this sort of thing is always weird backwards compatibility issues ... but I can't think of anything you haven't handled. Seems like it's probably good. Oh, and sorry for the mess with CreateViews, that one definitely looks like it was my fault. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This improves the ArrayPlug API so that we can create zero-length arrays which are still aware of the type of their elements, and can automatically resize to create new such elements. It is motivated by #5954, which has just that need.
This isn't without its wrinkles, since it turns out we have several ArrayPlug clients that are violating the constraint that all elements by identical. But I think it's a big step forward for the ArrayPlug itself, so I've fixed one client (CreateViews) and documented a path forwards for the others.