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

Allow to do DrawNode invalidation on cached BufferedContainer without redrawing the framebuffer #6404

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Uncomfy
Copy link

@Uncomfy Uncomfy commented Oct 30, 2024

This is needed for changing the parameters of a custom shader on BufferedContainer without tanking the performance (example: ppy/osu#30391 (comment))

Cached framebuffer is redrawn only if DrawNode invalidation comes from a child or if it is explicitly requested via ForceRedraw() method.

Companion to ppy/osu#30391

... when it is cached and Invalidate(Invalidation.DrawNode) is called,
and also when ForceRedraw() is called.
Framebuffer redraw now needs to be explicitly requested
by calling ForceRedraw() on the BufferedContainer when its cached.
It is also automatically triggered when the source of
Invalidation.DrawNode is a child.
@frenzibyte
Copy link
Member

frenzibyte commented Nov 11, 2024

Is there a specific reason why this is in draft? Seems like it's ready for review according to my eyes?

@bdach
Copy link
Collaborator

bdach commented Nov 11, 2024

I'm not sure the game-side feature proposal has been universally accepted, and this pull only makes sense in context of it, so I wouldn't disagree with draft.

For one I don't really see the point of this entire PR series so I'm not touching it.

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Fyi, on its own I think this direction is probably okay.

@@ -293,7 +297,11 @@ protected override bool OnInvalidate(Invalidation invalidation, InvalidationSour

if ((invalidation & Invalidation.DrawNode) > 0)
{
++updateVersion;
if (source == InvalidationSource.Child)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this will only work for immediate children, because Child invalidations currently don't propagate all the way to the root.

@@ -293,7 +297,11 @@ protected override bool OnInvalidate(Invalidation invalidation, InvalidationSour

if ((invalidation & Invalidation.DrawNode) > 0)
{
++updateVersion;
Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, we'll need to make sure this doesn't break any existing cases (or figure out how to fix those). I believe this currently means changing Padding or adding a child will redraw?

Copy link
Author

Choose a reason for hiding this comment

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

Added tests for Padding and adding new internal child, and no, they don't cause redraw. However, it doesn't cause redraw in master (bc83409) either.
I don't know if I should push those tests since they are failing, but it's not caused by this change, so for now - you can find them here: Uncomfy/osu-framework@buffered_container_explicit_framebuffer_redraw...buffered_container_extra_tests

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