Repository navigation
fix(impeller): preserve SDF gradient shader coordinate precision - #195036
richyo-codes wants to merge 4 commits into
Conversation
Keep gradient coordinates, inverse lengths, and shared linear/radial calculations at high precision. Mediump dot products can overflow on ordinary desktop-sized gradients and clamp abruptly to the final color. (cherry picked from commit c4ce654512e94413dbb2880e9eb937ad70b03876)
github-actions
Bot
added
engine
Render horizontal, diagonal, and radial gradients through the SDF backend and check GPU pixel readback against analytic color samples. Cover coordinates whose intermediate dot products or lengths overflow mediump despite a normalized gradient parameter below one. Validation: C++ formatting and diff checks; translation-unit compilation with branch headers. Executed against cached integration engine objects on Mesa llvmpipe: the corrected shaders pass, while replacing only the SDF shader sources with their pre-fix versions produces 16 failed color assertions. Metal execution and a fresh full branch build remain outstanding.
richyo-codes
changed the title
|
@richyo-codes I think this PR is ready for review, but it's stuck in "draft" because you already have two other active PRs in the repo. Sorry about the active PR limit. It causes some road bumps for cases like this, and it's something that the people in charge of the review process are looking into as an area of improvement. Do you mind if I create a fork of this PR so that we can have an "active" version to work on? It will still attribute you as a collaborator since your commit will be moved over. |
Hi @b-luk . I re-shuffled my PR's to allocate "Ready" to this one. So no fork should be required. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Impeller gradient shaders and common SDF shader code to use highp precision for gradient coordinates, lengths, and calculations, preventing overflow issues on medium precision. It also adds a comprehensive unit test, SDFGradientCoordinatePrecision, to verify correct rendering of large gradients. The review feedback suggests correcting a typo (positon to position) in gradient.glsl and removing an unused include (dl_dispatcher.h) in the unit test file.
| highp float IPComputeLinearGradientT(highp vec2 positon, | ||
| highp vec2 start, | ||
| highp vec2 delta, | ||
| highp float inv_gradient_length) { | ||
| return dot(positon - start, delta) * inv_gradient_length; | ||
| } |
There was a problem hiding this comment.
There is a typo in the parameter name positon (should be position). Correcting this improves code readability and consistency with other parameter names in the shader library.
highp float IPComputeLinearGradientT(highp vec2 position,
highp vec2 start,
highp vec2 delta,
highp float inv_gradient_length) {
return dot(position - start, delta) * inv_gradient_length;
}
References
- Optimize for readability: Code is read more often than it is written. (link)
| #include "flutter/testing/testing.h" | ||
| #include "impeller/core/device_buffer.h" | ||
| #include "impeller/core/device_buffer_descriptor.h" | ||
| #include "impeller/display_list/dl_dispatcher.h" |
There was a problem hiding this comment.
The include #include "impeller/display_list/dl_dispatcher.h" is not used in this test file. Removing unused includes keeps the codebase clean and adheres to the Google C++ Style Guide.
References
- C++ code should follow the Google C++ Style Guide, which discourages unused includes. (link)
|
There was a ci.yaml error, so I re-synced the branch. @b-luk will need to re-add CICD label |
|
|
@richyo-codes Thanks for the detailed analysis and the proposed fix. But I am closing this in favor of #195703. That fix consolidates the highp requirement in one place at the root of the SDF shader and covers other cases (circles, ovals, arcs) in addition to gradients, so it's preferable over this fix which only handles the gradient case and requires changes across multiple parameters and callers. |
Fixes #195159.
Prevents Impeller’s SDF gradient shaders from abruptly switching to the final color when
mediumpcalculations overflow.Affected files
For a 400-pixel linear gradient on FP16 implementations, the dot product exceeds the maximum finite value of 65,504 before normalization. Radial distance calculations are also affected.
This keeps gradient coordinates, inverse lengths, linear/radial calculations, and the resulting gradient parameter at
highp.Testing
AiksTest.SDFGradientCoordinatePrecision, covering horizontal, diagonal, and radial gradients on the OpenGLES SDF and Metal SDF backends.impellercand compares their rendered output.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.