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

Add ProgressRing #6003

Closed
wants to merge 10 commits into from
Closed

Add ProgressRing #6003

wants to merge 10 commits into from

Conversation

Splitwirez
Copy link
Contributor

What does the pull request do?

Adds the bagel-shaped ProgressRing control to Avalonia, as seen in WinUI.
image
...wait, were they supposed to be gluten-free?

What is the current behavior?

Avalonia has progress-indicating controls in baguette shape, but not bagel shape.

What is the updated/expected behavior with this PR?

A bagel-shaped progress-indicating control called ProgressRing is now available for use in Avalonia. How many different ways do you need me to spell it out? lol

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

WinUI ProgressRing control is done via Lottie animations
"But could it have been done without Lottie?"
Challenge accepted >:3
?????
Profit

Checklist

Breaking changes

None...?

Obsoletions / Deprecations

None

Fixed issues

N/A, the matter was raised in a Discussion post, not an Issue post

@Takoooooo
Copy link
Contributor

I would like to mention what dragging window by the titlebar on windows would stop the animation
#4551

@Takoooooo
Copy link
Contributor

Also, i would suggest making more specific converters.
image

At this moment you already know whether your progressring is indeterminate or not, so no additional checks and more optimized code. Also, I'm not sure if this possible but I suggest to pre-create those geometries and only bind mutable values I assume you could do that in styles.
image

@Splitwirez
Copy link
Contributor Author

Also, i would suggest making more specific converters.
At this moment you already know whether your progressring is indeterminate or not, so no additional checks and more optimized code. Also, I'm not sure if this possible but I suggest to pre-create those geometries and only bind mutable values I assume you could do that in styles.

Yeah @maxkatz6 and I had a nice discussion about that on Discord last night...I think a better plan for that is starting to take shape.

@Splitwirez Splitwirez mentioned this pull request Jun 3, 2021
set => SetValue(PreserveAspectProperty, value);
}

public double ValueAngle
Copy link
Member

Choose a reason for hiding this comment

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

If this property was added only to simplify templating, it makes sense to move it to inner property with relevant name.
See https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/ProgressBar.cs#L151
So it's it will be like "TemplateProperties.ValueAngle"

Copy link
Member

Choose a reason for hiding this comment

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

Meaning that developers shouldn't try to use it elsewhere

@Splitwirez
Copy link
Contributor Author

Sorry for the slow movement on this...currently having some trouble with git stuff.

For now, though...by pure coincidence, a friend of mine happened to raise an intriguing point on Discord:
progress clock message from klox

I bring this up because the way I've currently implemented this...isn't exactly able to produce the "slice of a pie" shape shown in the example image:
progress clock

Sure, this is of no consequence for the Fluent theme...but assuming all themes will be that way wouldn't be very lookless of us, now would it?

@robloo
Copy link
Contributor

robloo commented Jun 7, 2021

I think the progress clock or filled progress ring is a good idea. Actually, I would argue the "progress ring" is a special case of the "progress clock" where the thickness of the "slice" is reduced to only the edge.

To achieve this correctly I think you need another shape. Just like a rectangle is the two-dimensional equivalent of a line, a sector is the two-dimensional equivalent of an arc. Adding a sector shape would allow you to achieve the "progress clock" look using the same math (angle calculation) as for the ring.

That said, sometimes you want a 'hollow' donut shape so I'll need to check if there is a good/established way to set an inside and outside diameter of a sector.

I also looked in more detail at the Arc shape. Most arcs don't specify both a start and stop angle. Instead they specify the start and then a +/- sweep angle. I think it would be a good idea to change this for convention.

@Takoooooo
Copy link
Contributor

@Takoooooo Takoooooo mentioned this pull request Oct 25, 2021
@Takoooooo
Copy link
Contributor

I will close that PR, i tried to make it work with a master, but sadly it doesn't work correctly, but even original implementation consumes around ~300mb of memory on my PC(memory consumption jumps from 200mb to 500mb), Splitwirez will not be able to update this PR because he has deleted the branch so I think we can close that PR

@Takoooooo Takoooooo closed this Oct 25, 2021
@maxkatz6
Copy link
Member

I wonder if majority of memory usages comes from this part, which is executed each animation tick
https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/Shapes/Arc.cs#L81-L91

It might make sense to create something like FluentProgressRingPresenter in Fluent project which will render specific ring animation directly to the context without animations. Similarly with Default theme. In this way it still will be possible to change progress ring template and build-in implementation will work nicely (should).

@Splitwirez
Copy link
Contributor Author

I wonder if majority of memory usages comes from this part, which is executed each animation tick https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/Shapes/Arc.cs#L81-L91

It might make sense to create something like FluentProgressRingPresenter in Fluent project which will render specific ring animation directly to the context without animations. Similarly with Default theme. In this way it still will be possible to change progress ring template and build-in implementation will work nicely (should).

...if the StreamGeometry itself is the culprit, then...how would that even help?

@maxkatz6
Copy link
Member

At least we don't need to copy geometry to apply transforms
https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/Shapes/Shape.cs#L106

Also rotation can be done with a transform and not with recalculating whole geometry.
I assume right now we on each animation tick we recreate geometry with new Offset, then with new Sweep, and copy all of this on Render to apply transforms.
Although it still won't be perfect solution. Not to mention how more complex it is.

@robloo robloo mentioned this pull request Aug 5, 2022
3 tasks
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.

4 participants