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

PipelineBrush and effects refactoring #3298

Merged

Conversation

Sergio0694
Copy link
Member

Follow up to #3112

PR Type

What kind of change does this PR introduce?

  • Feature
  • Refactoring (no functional changes, no api changes)

What is the current behavior?

Effects are not extensible for the user, and there's no build-time check to ensure effects in a PipelineBrush instance are set in a valid order. Failing to do so will result in a runtime crash.

What is the new behavior?

  • The PipelineBrush now has two separate properties: Input and Effects. There are two different interfaces to ensure there are build-time checks on effects assigned to either property.
  • Since the initialization code has been moved away from the PipelineBrush and into each concrete effect, users are now free to also create their own custom effects and plug them into a PipelineBrush. They can do so by inheriting from either IPipelineInput or IPipelineNode.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented May 23, 2020

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost assigned michael-hawker May 23, 2020
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely like how this improves the usability of the system with the input/effects and the stacking made clearer. And the fact that then it's more easily extensible for others to add in their own. Had a few quick initial thoughts from my initial glance.

@michael-hawker michael-hawker added this to the 6.1 milestone May 26, 2020
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Just need a small tweak to the sample for Acyclic and I think we're good to go. This definitely resolves the need for the 19041 workaround! Thanks!

Also, question on the Placement property on the BlendEffect? For the Unicorn Image Source, setting it to Background places it in-front of the brush in the 'Foreground', but setting it to Foreground places it behind the rest of the pipeline in the 'Background'... should these be reversed or am I looking at this from the wrong perspective?

@ghost
Copy link

ghost commented May 27, 2020

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@Sergio0694
Copy link
Member Author

@michael-hawker So, regarding the blend placement, it refers to the "current" pipeline, which is the main one from the brush in that case. Using Background means that that's put in the background, which is why the unicorn ends up being on top, and vice versa.

This of course is just a matter of naming, I can revert them if you think that'll be more intuitive!

@michael-hawker
Copy link
Member

@Sergio0694 let's discuss with some community folks and then we can create a separate PR inverting the property or changing the names to be clearer on what the behavior is around merging pipelines (or maybe they'll say the current makes sense).

@ghost ghost removed the needs attention 👋 label May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants