Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialise connections to children of ScenePlug/ImagePlug #3986

Closed
johnhaddon opened this issue Oct 29, 2020 · 3 comments · Fixed by #5624
Closed

Serialise connections to children of ScenePlug/ImagePlug #3986

johnhaddon opened this issue Oct 29, 2020 · 3 comments · Fixed by #5624

Comments

@johnhaddon
Copy link
Member

johnhaddon commented Oct 29, 2020

Summary

Some advanced users would benefit from being able to manage connections to the individual children of ScenePlug and ImagePlug. This is trivially achieved when writing a Python or C++ subclass of SceneProcessor/ImageProcessor as the connections can be made in the constructor. But if the connections are made manually in a regular node graph, they are not serialised because the Serialisable flag has been turned off for the children.

User story

A common use is to use an expression to drive the globals or metadata, while passing everything else through via pass-through connections.

Implementation notes

This is trivially achieved by modifying the ScenePlug and ImagePlug constructors to keep the Serialisable flag for the children. But doing this leads to test failures because we end up serialising internal connections for regular SceneProcessor/ImageProcessor subclasses. I think ideally we should make SceneProcessor.out and ImageProcessor.out non-serialisable by default as it has continually tripped us up, but this is a breaking change and we don't know the consequences for custom nodes in the wild. Perhaps it is sufficient just to document the change and say "turn serialisation back on in your constructor if you need it"?

@johnhaddon
Copy link
Member Author

The more I think about it, the more I think we should disable serialisation of inputs for *Processor.out. Serialising them leaks internal implementation details and can cause problems when the implementation needs to change. We've disabled it manually on a node-by-node basis as we discover the problems caused, but haven't been brave enough just to make the change across the board. I think now is the time.

@danieldresser-ie
Copy link
Contributor

Disabling serialization of *Processor.out definitely sounds good. It would be an extremely special case where you would actually want to serialize that.

I don't entirely understand why it relates to this though ... I would hope that we would only be serializing the child connections in the case where they aren't already implied by the parent connection, like what happens currently with shader color connections? And I would think that the internal connections to "out" would be whole plug connections anyway, so I don't understand why this would affect them?

But standardizing no serialization for the out plug sounds good anyway.

@johnhaddon
Copy link
Member Author

I would think that the internal connections to "out" would be whole plug connections anyway, so I don't understand why this would affect them?

It's really common to pass through some children directly via connections and then implement compute for the remaining unconnected outputs.

johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Jan 12, 2024
johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Jan 24, 2024
johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Jan 24, 2024
This means we also need to disable serialisation for `ImageProcessor.out` to avoid unwanted serialisation of internal connections. This _could_ have been achieved by removing the Serialisable flag, but that caused problems for things like `Dot::setup()`, which _do_ force the added plug to be serialisable, but _not_ its children. We could have modified all those locations to set flags recursively, but since we want to remove flags entirely in future it makes more sense to use a custom serialiser instead. The Dot setup problem demonstrates the problem with flags in the first place - they get copied from place to place, but they may only have made sense on the node they originated from.

In conjunction with the previous commit, this fixes GafferHQ#3986.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants