Skip to content

Conversation

@wmedrano
Copy link
Collaborator

@wmedrano wmedrano commented Jan 21, 2026

  • This is a straight port from Skia

Items remaining: None!

  • Testing
  • Thinking about future of RadialGradient API or other simplifications

This is a straight port from Skia
@RazrFalcon
Copy link
Collaborator

Thanks! Looks good to me.

As for RadialGradient vs ConicalGradient, I think it's fine to break the API a bit by keeping the RadialGradient name and simply modifying the constructor.

Keeping both at the same time would be too much work.

Other than that - if this change works fine for you and none of the resvg tests are failing after this change - we're ready to go.

@wmedrano
Copy link
Collaborator Author

wmedrano commented Jan 22, 2026

Made the changes so that RadialGradient is now the 2 point conical gradient.

@wmedrano wmedrano marked this pull request as ready for review January 22, 2026 02:02
@wmedrano
Copy link
Collaborator Author

Inspecting the 3 avx test failures its due to NaN related equality for data produced by XYTo2PtConicalStrip (new), XYTo2PtConicalSmaller (new), and XYTo2PtConicalGreater (pre-existing).

conical_smaller_radial

stages = [SeedShader, Transform, XYTo2PtConicalSmaller, Mask2PtConicalDegenerates (diff), ...]

AVX got different output for (3.1933205, 3.2251167, 3.2629573, 3.3092515, 3.3685663, 3.451985, 3.616074, NaN) != (3.1933205, 3.2251167, 3.2629573, 3.3092515, 3.3685663, 3.451985, 3.616074, NaN)

  • normal result: [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, NaN]
  • avx result: [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]

strip_gradient

stages = [SeedShader, Transform, XYTo2PtConicalStrip, Mask2PtConicalNan (diff), ...]

AVX got different output for (NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN) != (NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN)

  • normal result: [NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN]
  • avx result: [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]

conical_greater_radial

stages = [SeedShader, Transform, XYTo2PtConicalGreater, Mask2PtConicalDegenerates (diff), ...]

AVX got different output for (NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN) != (NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN)

  • normal result: [NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN]
  • avx result: [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]

@wmedrano
Copy link
Collaborator Author

wmedrano commented Jan 22, 2026

Any thoughts on how to move forward?

  • Accept change to AVX instruction in src/wide/f32x8_t.rs to match non-avx
    • The unit tests then match Chrome/Firefox SVG rendering
    • Seems to be what the pre-existing mask_2pt_conical_degenerates wanted. Which is to use cmp_ne to find NaNs.
  • Use different unit tests and accept AVX may have differences in some scenarios
  • Patch XYTo2Pt.* instructions to remove NaNs
    • NaN doesn't seem like that much of an edge case. It happens when |y| > r0 in $$\sqrt{r_0^2 - y^2}$$ operation

@wmedrano
Copy link
Collaborator Author

This PR is good to go!

  • The change to src/wide/f32x8_t.rs fixes an existing bug with mask_2pt_conical_degenerates that doesn't seem to be exercised by the existing unit tests or resvg.

@wmedrano wmedrano requested a review from RazrFalcon January 23, 2026 01:11
@wmedrano wmedrano self-assigned this Jan 23, 2026
- Notably, transform inversion happens after checking the color stops.
@RazrFalcon
Copy link
Collaborator

Thanks! Looks good to me.
And thanks for the SIMD fix.

I would also add this to the changelog:

### Changed
- The `RadialGradient::new` requires a start radius now. Set the second argument to 0.0 to preserve the old behavior. 

Hopefully, the current maintainer would be able to merge and publish it soon.

PS: there is one link to the original code left, not sure if intentionally or not.

@nicoburns nicoburns requested review from LaurenzV and dfrg January 26, 2026 17:22
@wmedrano wmedrano removed the request for review from RazrFalcon January 26, 2026 17:35
@wmedrano wmedrano requested a review from LaurenzV January 26, 2026 18:24
@LaurenzV
Copy link
Contributor

No need to move to them to the bottom, I just was curious. If I'm seeing correctly your newest commit reformatted a lot of unrelated code, can you revert that commit? Then I can merge.

@wmedrano
Copy link
Collaborator Author

Format changes reverted

@LaurenzV LaurenzV changed the title Support 2 point conical gradient Support 2 point conical gradients Jan 26, 2026
@LaurenzV LaurenzV changed the title Support 2 point conical gradients Add support for 2 point conical gradients Jan 26, 2026
@LaurenzV LaurenzV merged commit 6943c20 into linebender:main Jan 26, 2026
4 checks passed
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.

3 participants