Repository navigation
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If both you and the PR author believe this PR qualifies for a test exemption, escalate to the tech lead for the changed area to review and apply the |
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
|
@gaaclarke thoughts on this? I think this should be fine because we're targeting desktop with uber_sdf. Alternatively, we could do some scaling down in parts of the code that blow up. E.x., IMO we can live with highp for now, and we can explore tricky alternatives if we decide to use uber on mobile |
That sounds fine to me. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
I'd like a test that demonstrates the requirement.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
lgtm assuming you're happy with the golden output
|
@gaaclarke malioc'd |
| "stack_spill_bytes": 0, | ||
| "thread_occupancy": 100, | ||
| "uniform_registers_used": 62, | ||
| "uniform_registers_used": 78, |
There was a problem hiding this comment.
Oof we've tried to draw a hard limit here for 64. Any ideas here?
There was a problem hiding this comment.
Yeah, I think we could do a few things to try and squeeze it down. Since we're only targeting desktop right now and this is something that's affecting a bunch of users - thoughts about landing this now and squeezing later?
There was a problem hiding this comment.
We can afford some time to try to address it. Maybe there is an easy win here. Sic mad dog gemini on it and see what it says.
There was a problem hiding this comment.
Gemini was kind of a mess here, but my feeble human mind came up with #196392.
There's an easier way to save uniforms, which is to completely remove the RSE from the uber shader - but that removes the gradient performance gains from RSEs. We can get to 64 registers with #196392 and more targeted highp directives
|
This also fixes #195159. Can you add a test for large gradients as well, and you can update the description to say it fixes that issue too? |
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
Refactors a few things in uber_sdf to save on uniform registers. - makes aa_pixels a constant in the fragment shader rather than being set by uniform - reworks the superellipse corner calculation to remove atan and mod - Folds the superellipse angle_span into unused `radii` fields - Moves `center` and `gradient_origin` to vertex inputs Prework for flutter#195703 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
UberSDF uses mediump float for all it's calculations - for large coordinates, this causes overflow when calculating things like length - leading to rendering errors as in #195208.
This change just switches the shader to highp floats, fixing the rendering issue - this should also make the antialiasing look sharper in general.
Before:
After
Fixes #195208
Pre-launch Checklist
///).