-
Notifications
You must be signed in to change notification settings - Fork 764
Performance improvement for the Plotter example #9836
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great achievement.
Could you explain what you did to get there and why it helps?
I'm concerned that using set_render_notifier for this is not as portable as the first method, and also a bit more complicated for an example. I'd like to understand why this is faster though.
examples/plotter/main.rs
Outdated
| new_height, | ||
| )); | ||
| } | ||
| main_window_strong.window().request_redraw(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that you will force a refresh all the time even if nothing changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, no choice there is no resize event in slint and using the texture dimensions requires this
Because Slint also uses the request_redraw so we can't only re-render when we need to because its constantly called anyway by for example the slider thumb animation, and other factors
Even if you were to add for example a callback on width and height change on image it would spam it because the resizing is not instantaneous
As seen in this earlier issue I recorded, the slider spams render window, because the render loop given is tied to the UI rendering, so they both end up inefficient, its a design flaw
Note that on my main project I stopped using Slint for this exact specific issue ironically
On this video we do not request a redraw every frame so the texture is not automatically updated
And as you can see the slider's position change enforce redraw calls multiple times, rather than compute its final position before rendering, this is likely tied with an issue with Rectangle where when an image is inserted inside a rectangle, it affects the parent rectangle's size
rec.mp4
|
|
||
| edition = "2021" | ||
| use_small_heuristics = "Max" | ||
| max_width = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that not the default? why do you change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default which you use, but this specification wasn't there so my formatter used the wrong format
It'll prevent CI reformatting for future contributors who do not have this option in their vscode settings
| VerticalBox { | ||
| amplitude := Slider { | ||
| orientation: Orientation.vertical; | ||
| transform-rotation: -180deg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rotation shouldn't be needed. Instead the we should fix the sliderto be in the right direction.
(I remember some discussion about that in #4705 (comment) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, technically it should be orientation up, down, right or left, or maybe just vertical / horizontal but where min and max are replaced by start and end and it can start at 10 and end at zero
Although ideally in the future a slider could have 2 ends to select a range, this is something else to consider
|
Perhaps I’m missing something. Why do you think the old code re-rendered the plot every frame instead of on-demand? |
|
Generally speaking, one of the objectives of this example is to demonstrate how to render such a graphic lazily using a binding that’s re-evaluated only when a parameter changes. That happening automatically is IMO very convenient and a good pattern that’s worth preserving. |
|
The inability to specify the width and height of the surrounding element for the render function though is an annoying bug (in Slint, not the example). So hardcoding the size sucks, that I agree with. |
|
I don't think that's right. although there is maybe a bug that caused the plot to be rendered on mouse event, and there would be many mouse event being handled preventing the frame to be drawn. Possibly related to #9038 IMHO we should probably compress/merge events so that we process only one mouse move event per frame |
|
It was assumed by experience with other renderers I did not test anything, I just saw and estimated then fixed, it might be a completely different issue |

newnew.mp4