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.

Sign up for GitHub

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

Jump to bottom

[Skia] Fix RadialGradientBrush for non center origin #17925

Merged
merged 4 commits into from
Jan 20, 2025
Merged

[Skia] Fix RadialGradientBrush for non center origin #17925

merged 4 commits into from
Jan 20, 2025

Conversation

Gillibald
Copy link
Contributor

@Gillibald Gillibald commented Jan 8, 2025 •
edited
Loading

What does the pull request do?

This PR adjusts the Skia RadialGradientBrush not to reverse the order of gradient stops when the gradient isn't covering the full radius. Otherwise, the shader isn't working as expected.

What is the current behavior?

Screenshot 2025-01-08 151714

What is the updated/expected behavior with this PR?

Screenshot 2025-01-08 145831

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

@Gillibald Gillibald force-pushed the fixes/RadialGradientBrush branch from c2379a0 to 29ee228 Compare January 8, 2025 14:08
@Gillibald Gillibald changed the title [Skia] Fix RadialGradientBrush for non center origin [WIP][Skia] Fix RadialGradientBrush for non center origin Jan 8, 2025
@Gillibald Gillibald force-pushed the fixes/RadialGradientBrush branch from 29ee228 to 90125e4 Compare January 9, 2025 06:22
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054165-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@Gillibald Gillibald changed the title [WIP][Skia] Fix RadialGradientBrush for non center origin [Skia] Fix RadialGradientBrush for non center origin Jan 9, 2025
@Gillibald Gillibald requested a review from kekekeks January 9, 2025 14:32
@Gillibald Gillibald added bug customer-priority Issue reported by a customer with a support agreement. labels Jan 9, 2025
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054186-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054196-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054243-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@Gillibald Gillibald force-pushed the fixes/RadialGradientBrush branch from 603f7ff to e29a256 Compare January 14, 2025 11:36
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054265-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul added the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Jan 14, 2025
MrJul
MrJul approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

The logic looks good to me!

src/Skia/Avalonia.Skia/DrawingContextImpl.cs
* Matrix.CreateTranslation(centerPoint);


if (radialGradient.Transform != null)
{
Copy link
Member

Choose a reason for hiding this comment

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

Formatting nit: the whole case has an extra indentation level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done by VS for some reason. Ideally I would want to split the method into multiple parts to better organize it.

@Gillibald Gillibald force-pushed the fixes/RadialGradientBrush branch from 2586b6b to 9a94a62 Compare January 15, 2025 07:53
@Gillibald
Copy link
Contributor Author

Something is wrong with the initial formatting of the DrawingContextImpl.cs I leave it as is now. Too much pain to manually adjust the formatting.

@MrJul
Copy link
Member

MrJul commented Jan 15, 2025

Something is wrong with the initial formatting of the DrawingContextImpl.cs I leave it as is now. Too much pain to manually adjust the formatting.

The diff is now perfectly readable so it's good :)

(We should probably implement auto formatting on commit at some point to get consistent files everywhere, way out of scope of this PR.)

@MrJul MrJul enabled auto-merge January 15, 2025 07:58
@MrJul
Copy link
Member

MrJul commented Jan 20, 2025

It seems some lines were reverted when they shouldn't, this doesn't compile anymore.

@Gillibald Gillibald force-pushed the fixes/RadialGradientBrush branch from 701323b to 0b46143 Compare January 20, 2025 18:49
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054355-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul added this pull request to the merge queue Jan 20, 2025
Merged via the queue into AvaloniaUI:master with commit d3b61cd Jan 20, 2025
10 checks passed
@Gillibald Gillibald deleted the fixes/RadialGradientBrush branch January 22, 2025 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrJul MrJul MrJul approved these changes

@kekekeks kekekeks Awaiting requested review from kekekeks

Assignees
No one assigned
Labels
backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch bug customer-priority Issue reported by a customer with a support agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gillibald @avaloniaui-bot @MrJul