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

refactor(core): 💡 skip paint for out of bounds widgets #677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tashcan
Copy link
Contributor

@tashcan tashcan commented Dec 22, 2024

Purpose of this Pull Request

Widgets shouldn't paint outside of their bounds, as such we can just skip painting anything, saving a lot of work
especially for large widget trees with a lot of widgets that are off-screen.
Additionally, since saving the painter state isn't entirely free, we can also skip doing that when we aren't painting anything.

Checklist Before Merging

Please ensure the following are completed before merging:

  • If this is linked to an issue, include the link in your description.
  • If you've made changes to the code or documentation, make sure these are updated in the CHANGELOG.md file.
  • If you've introduced any break changes, briefly describe them in the Breaking section of the CHANGELOG.md file.

Additional Information

The bot will replace #pr in CHANGELOG.md with your pull request number. If your branch is out of sync, use git pull --rebase to update it.

If you're unsure about which branch to submit your Pull Request to, or when it will be released after being merged, please refer to our Release Guide.

If you're working on a widget and need help writing test cases, we have some macros that can assist you. Please refer to the Ribir Dev Helper documentation.

@github-actions github-actions bot force-pushed the to-upstream/skip-paint-out-of-bounds branch from 26dc27a to 0bd7f9c Compare December 22, 2024 06:24
@M-Adoo M-Adoo added the B-test Notify bot to start testing label Dec 22, 2024
@tashcan tashcan force-pushed the to-upstream/skip-paint-out-of-bounds branch from 0bd7f9c to 2155e81 Compare December 22, 2024 07:59
@M-Adoo M-Adoo removed the B-test Notify bot to start testing label Dec 22, 2024
@github-actions github-actions bot force-pushed the to-upstream/skip-paint-out-of-bounds branch from 2155e81 to 275b96e Compare December 22, 2024 07:59
@M-Adoo M-Adoo added the B-test Notify bot to start testing label Dec 22, 2024
@tashcan tashcan force-pushed the to-upstream/skip-paint-out-of-bounds branch from 275b96e to 67c3a3f Compare December 22, 2024 08:03
@M-Adoo M-Adoo removed the B-test Notify bot to start testing label Dec 22, 2024
@github-actions github-actions bot force-pushed the to-upstream/skip-paint-out-of-bounds branch from 67c3a3f to 2ea230c Compare December 22, 2024 08:04
@M-Adoo M-Adoo added the B-test Notify bot to start testing label Dec 22, 2024
@M-Adoo
Copy link
Collaborator

M-Adoo commented Dec 22, 2024

Thanks a lot. In previous versions, we did have a paint bounds check. However, we decided to remove it because we were not adhering consistently to the rule of how a widget should handle painting overflow outside its parent at that time.

We shouldn't completely skip a whole subtree if it doesn't intersect with the paint bounds. This is because we want some widgets to be able to show overflow outside their parent.

Consider this example:

@Stack {
  @Text { anchor: Anchor::left(-100.) }
}

In this case, the text is allowed to show outside the stack.

I believe we should implement this optimization eventually, but we are still working on defining a stable rule for it.

Currently, I suggest allowing a widget to paint overflow outside its parent if the parent is not a fixed container. You can refer to #676 for more details on hit testing.

Alternatively, we could add a Render::allow_paint_overflow method to establish rules for each parent.
There

@tashcan
Copy link
Contributor Author

tashcan commented Dec 22, 2024

Makes sense. Not that I have any say in any of this, but I personally feel like painting outside the bounds should be an explicit opt-in for a widget.
I would think that a Render::allow_paint_overflow and piping that up the tree would be the ideal way to go about this.
Coming at this from a complete outsider perspective my mental expectation was that anything that is painted outside the bounds of a widget would get clipped.

@M-Adoo
Copy link
Collaborator

M-Adoo commented Dec 22, 2024

Makes sense. Not that I have any say in any of this, but I personally feel like painting outside the bounds should be an explicit opt-in for a widget. I would think that a Render::allow_paint_overflow and piping that up the tree would be the ideal way to go about this. Coming at this from a complete outsider perspective my mental expectation was that anything that is painted outside the bounds of a widget would get clipped.

Certainly, but performing additional clip checks and clip painting may not be the most efficient solution in the end. I believe we should consider whether to conduct the paint boundary check after implementing a straightforward and user-friendly LazyChildren feature. This way, we can prevent unnecessary generation of offscreen widgets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-test Notify bot to start testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants