-
Notifications
You must be signed in to change notification settings - Fork 29
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
Ensure visualizer operator override #1889
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks good, new tests catch intentionally regressing either issue as appropriate. Had some comments on the tests and new test infrastructure.
I didn't verify in-editor because lazy I assume Bruno will double-check it solves things on his end 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working as intended on my end! Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation in #1751 introduced a regression whereby
Visualizer
operators found themselves overruled by the new propagation semantics forSink
. This happened becauseVisualizer
inherits its internalProcess
method from theSink
base class, and therefore the following check succeeded for bothSink
andVisualizer
sinceDeclaringType
would beSink
in both cases:bonsai/Bonsai.Core/Expressions/InspectBuilder.cs
Line 215 in 130de6f
This PR introduces a new internal virtual method to allow the
Process
method to be redeclared in derived classes. This allowsVisualizer
to "override" the method with a new static function which just calls the same base class implementation but which now will cause a change in theDeclaringType
of the method. The above check will now fail forVisualizer
types, which restores the behavior of the operator.Due to the subtle nature of visualizer propagation semantics and the likelihood of further internal implementation reviews, we also added regression tests for the issue, together with a refactoring of the testing framework towards a more flexible combinator-based API for building workflow graphs (this could almost go into its own PR as it can in principle be used for everything, but I prefer to leave it close to its original motivation and we can see where it goes from there).
Fixes #1860