ColorMap and ValueMap can have multiple sinks

[azeno]

Summary

The PR lifts the restriction that nodes feeding values or resources into the shader graph could only be connected to one sink.

Details

My initial approach where the sources kept a weak reference on the parameter collections of the sink didn't work because on disconnect the sink was still alive and therefor the source would still write into it.

The approach now is to provide a subscription object (a CompositeDisposable) while generating the shader graph (through the ShaderGeneratorContext.Tags property) where leaves can tie their value subscription to the lifetime of the entire shader graph.

Example

The two links from the ColorMap node work as expected:

Related issues

vvvv/VL.StandardLibs#270

[tebjan]

this looks quite good, my only concern is whether we should merge such a change into the stable branch. I think it makes more sense to add it to the preview-2021.4 branch, which most users actively test.

and this change should also be integrated into the ShaderFX node factory, which doesn't exist in the 2021.3 branch.

[azeno]

I rebased on 2021.4 branch and added the changes to the ShaderFX nodes as well. However my only test was with the TextureFX fader overview patch connecting all effects to the same fader InputValue node. That works until you disconnect in which case the effect doesn't seem to get updated and continues to execute the previous connected InputValue instead going to its default value. But maybe that issue is completely unrelated?

And what would be a good patch to test the key generation thing you mentioned in the chat?

[azeno]

Another update which I believe is cleaner as it doesn't rely on weak references anymore and the lifetime of the value subscriptions per sink is clearly defined. The overview patch works nicely already. However didn't find the time to cleanup so consider this a WIP.

[tebjan]

why do you think that a weak reference is worse compared to a manual approach? the new approach seems to force every source of an effect or material to implement this mechanism. so even the stride serializer would need to know about this, or not?

this reminds me also of the parent managers in the entities that now force every entity that wants to enter the scene graph to have one of them. I think that was a design decision that forces work on everyone and it requires knowledge that is hidden. in this case, it is less visible to the user, but i would prefer something that stays confined to the area of shader fx and no other system needs to know about it.

[azeno]

Problem was that the "connection" was still there even after deleting a link. So the source was still writing to the sink. Relying on the garbage collector doesn't work. The sink is still alive after all. This was also the problem I described above when disconnecting. My initial thought that it's about mutating shader source state wasn't entirely correct.

Stride serializer: In that scenario I'd load a material from disk right? Well then there're no nodes of ours involved which would feed values into the material.

[azeno]

No longer using subjects. Added link to this PR in case one needs to understand the topic in another lifetime. Would like to merge it back.

[tebjan]

great work, just one thing from my side: as I wrote in the comment of the parameter setter, it is now optimized for a somewhat rare special case. the default case in the update loop per pin should/could have its own code path, IMO.

[azeno]

Just pushed another commit optimizing for the single parameter collection case. Will keep it as a separate commit to make it easier to see the changes.

[tebjan]

major PR and ready to merge from my side. definitely, the better UX compared to a warning. should there be an issue after the merge, I would consider it a normal bug.

also, thanks for the detour into the multiple sink problem, I am sure we will have this problem again in the future and we will be able to learn from this solution. as in vvvv/VL.StandardLibs#310