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

Rewrite internals of EffectDrawBase #310

Merged
merged 6 commits into from
Oct 17, 2024
Merged

Conversation

sirdoombox
Copy link
Collaborator

@sirdoombox sirdoombox commented Oct 16, 2024

Changes

  • Internally, EffectDrawBase now uses CompositionCustomVisualHandler.
  • Fix SukiTransitioningContentControl.
    • Transition animations now play correctly.
    • Backbuffer is cleared out when it's done playing it's animation and assigning null to the content is now valid which should make detaching content from the visual tree work as intended.

Outstanding Issues

  • The Loading control - even when invalidating only the rect for it - still has very high GPU utilisation and I'm not quite sure why.
  • The ShaderToyRenderer in the demo also suffers from this same issue - though it seems to use less GPU for reasons beyond my understanding.
  • We don't really have a docs page covering SukiWindow at the moment.

I wanted to ideally have at least one or two other people test this to make sure the GPU/CPU issues are in fact under control and everything renders correctly, before this gets merged as it's a pretty major internal change.

@kikipoulet
Copy link
Owner

No problem on my side, behave like expected with great performance improvements

@sirdoombox
Copy link
Collaborator Author

I annoyingly discovered a bug with SukiTransitioningContentControl whilst I was testing this out in another app, the first time content was set IsHitTestVisible was being set to false and I'm not fully sure why, struggling to actually get it to enable correctly.

Whilst trying to fix this bug I also discovered that the core functionality was completely disabled and broken in #259 for some reason. I think the bug this was supposed to fix needs a solution, hopefully I can come up with one that retains the functionality of the transitioning content control.

@sirdoombox
Copy link
Collaborator Author

I'm not 100% certain I've fixed the issue that #259 was supposed to fix, but it should do the job, now the back buffer is cleared out and you can assign null to the Content property which should cause the content to be cleared out correctly. It also re-enables the transition animations and such.

@sirdoombox sirdoombox marked this pull request as ready for review October 16, 2024 13:52
@sirdoombox
Copy link
Collaborator Author

Could you confirm that #259 isn't regressed with these changes @AuroraZiling? If the crash is back, please send me a repro and I'll try and implement a better solution.

@AuroraZiling
Copy link
Collaborator

confirmed, the issue isn't regressed

@sirdoombox
Copy link
Collaborator Author

Thanks for the suggestions @maxkatz6, PR is ready to go.

I won't merge it myself just so one final sanity check gets done before it gets merged.

@kikipoulet
Copy link
Owner

Thanks for the suggestions @maxkatz6, PR is ready to go.

I won't merge it myself just so one final sanity check gets done before it gets merged.

I tested it, LGTM

@sirdoombox sirdoombox merged commit e0bed14 into kikipoulet:main Oct 17, 2024
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 this pull request may close these issues.

4 participants