[wip] implement SkiaSharp PlotViews #71
Conversation
| while (true) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| this.renderRequiredEvent.WaitOne(); |
There was a problem hiding this comment.
I don't know if the guidance has changed on this (and I'm just glancing at the PR, not taken a proper look yet, so may be talking nonsense), but I don't think locking up a thread like this is ideal. Using https://learn.microsoft.com/en-us/dotnet/api/system.threading.semaphoreslim.waitasync?view=net-8.0 or similar would allow it to run on the thread-pool and may be a better solution
There was a problem hiding this comment.
You are correct of course, and usually I would prefer not to block a thread here.
However SemaphoreSlim does not really cover the use-case we need from the AutoResetEvent here: It's possible that updates are requested more often than they can be performed, and I don't think this scenario really works with a semaphore. Usually I'd like to use AsyncAutoResetEvent from AsyncEx for this purpose, but I didn't want to bring in this dependency just for this. Shame that .NET doesn't provide stuff like this out of the box...
There was a problem hiding this comment.
SemaphoreSlim+CAS has always been my go-to for this; as you say, it's annoying that there's no equivalent method on AutoResetEvent. Not familiar enough with the code to say for sure, but if we move the CAS logic earlier it should implicitly solve that problem: I'm happy to look into this in more detail.
(another option is to just start a new Task as necessary; I'm not sure how that would compare performance wise; as I recall the latency on wait methods is pretty terrible as it is; or probably best of all is just to use a TCS like the semaphore does already https://referencesource.microsoft.com/#mscorlib/system/threading/SemaphoreSlim.cs,695)
There was a problem hiding this comment.
I am not quite sure how this could be made to work with a SemaphoreSlim, but I would be curious to find out if that is possible. So you are quite welcome to look into this if you are motivated :)
There was a problem hiding this comment.
Idea is that you use a dirty flag to decide whether the code should actually run (it already does this for update vs render), then the semaphore will just spin quickly until the count is 0 (improvement is then to use the flags to not set the semaphore unless it needs to be set). I'll stop posting inane commentary here, and hopefully take a proper look at the whole PR over the weekend ;)
There was a problem hiding this comment.
There is definitely room for improvement here, I just realized that the isRenderRequired flag isn't actually used anywhere. And thinking about it, I guess this makes sense, because in the double-buffer configuration we only need to render when something actually has changed in the PlotModel...
There was a problem hiding this comment.
A few of these comments are just remarks about Avalonia vs WPF, so treat them however you want. Other observations:
- XAML Bar Series example is broken (missing the blue series), and it crashes if you try to hide one of the series by clicking in the legend.
Not looked at the threading properly yet; will have a play with that next.
Source/Examples/Avalonia/ExampleBrowser/NotNullBooleanConverter.cs
Outdated
Show resolved
Hide resolved
| OxyPlot.Avalonia.OxyPlotModule.EnsureLoaded(); | ||
| OxyPlot.SkiaSharp.Avalonia.OxyPlotModule.EnsureLoaded(); |
There was a problem hiding this comment.
What purpose do these serve?
There was a problem hiding this comment.
No idea, I have this from some Avalonia example which I can't find anymore. This seemed to be required at some point when I was testing, but I just tried without these lines and it seems to work fine...
There was a problem hiding this comment.
I had wondered if it was to force the XML namespaces or something, but I don't know.
If it works without, I'd say scrub them until we have reason to put them back in.
| /// </summary> | ||
| protected PlotBase() | ||
| { | ||
| this.TrackerDefinitions = []; |
There was a problem hiding this comment.
This is really going to annoy me: hard not to see this as an immutable list. I assume these are automatic fixes? (I'd prefer the full type was here, given it's opaque; new() would be preferable to [] otherwise, but this is just personal preference)
There was a problem hiding this comment.
Yes those are automated fixes which are offered on latest Visual Studio on default settings. I don't mind this notation and don't have the association with immutable lists, but I can see how this might be an opinionated matter.
There are a couple of issues regarding code style in this PR. The problem is that there are neither clear guidelines (even less so than in the core repo) nor a .editorconfig which could be used to make sure the style is ok.
There was a problem hiding this comment.
Yeah, I just need to get with the times; it's fine. Could pull the .editorconfig from the core repo, that would be fine by me
|
|
||
| namespace OxyPlot.SkiaSharp.Avalonia | ||
| { | ||
| public class PlotRenderer(PlotView plotView) : Control, IDisposable |
There was a problem hiding this comment.
Would this be better called SkiaPlotRenderer?
| /// <param name="updateData">The update Data.</param> | ||
| public virtual void InvalidatePlot(bool updateData = true) | ||
| { | ||
| this.isUpdateRequired = updateData ? 2 : 1; |
There was a problem hiding this comment.
This isn't sufficient: may change a 2 to a 1
There was a problem hiding this comment.
Old code's purpose is to ensure we only ever increase the value (so we can't go from requesting an update with data-changes to an update without data-changes) and only fires off a request if there isn't one active already (i.e. if the user calls
plot.InvalidatePlot(true) followed immediately by plot.InvalidatePlot(false), the old code will ensure that if the work isn't picked up by the thread doing the update before the second call that it will still observe the updateData=true bit and it will only be invoked once)
There was a problem hiding this comment.
Updated the code to make sure a 2 is not overwritten by a 1. Regarding the rest of the old behaviour (only one invoke for consecutive update calls), I will have to look into it some more later.
| xmlns:local3="http://oxyplot.org/skiasharp/avalonia/doublebuffered" | ||
| xmlns:local2="http://oxyplot.org/skiasharp/avalonia" | ||
| xmlns:local="http://oxyplot.org/avalonia" |
There was a problem hiding this comment.
These could do with better names
There was a problem hiding this comment.
Would be nice to share the Tracker theme somehow, but I can't get it work with the shared project (never really used them, so I've no idea what I'm doing)
| { | ||
| return new OxyRect(rect.Left, rect.Top, rect.Width, rect.Height); | ||
| } | ||
| public static Rect ToRect(this OxyRect rect) |
There was a problem hiding this comment.
Micronit: Probably wants a line break between these
|
|
||
| private void StartRenderLoop() | ||
| { | ||
| new Task(() => |
There was a problem hiding this comment.
Because all this stuff is deferred, I think it's possible for it to be started by being attached, and then be detached before it gets around to creating the CTS, and so run when not attached.
| this.StopRenderLoop(); | ||
|
|
||
| // release renderRequiredEvent once more in case the render loop currently is waiting for the mutex; otherwise it might wait forever and will not be canceled | ||
| this.RequestRender(); |
There was a problem hiding this comment.
If we can switch to using async for the render loop, can then await the cancellation token rather than having to pulse it when we are disposing. As is, I think this should be in StopRenderLoop so that OnDetachedFromVisualTree also ensures the render loop doesn't sit waiting.
|
Latest commits make the render loop of the double buffered renderer There is still some work to do, to be continued. |
|
Quick question. Why won't you use SKPictureRecorder and then wrap the resulting SKPicture into ICustomDrawOperation instead of rasterizing into SKBitmap? Is the bottleneck the rasterization itself? |
|
Summing up my understandig of this topic, please correct if I am wrong:
I have not tried this, but I assume that rasterization might be fairly expensive as well, of course depending on the plot. But I have not actually evaluated how the two compare in terms of performance, I will look into it when I find time. Thanks for the suggestion, I had not thought of this before! |
|
I'll try to take another look at this next weekend |
|
Avalonia does not do any drawing on the UI thread. The DrawingContext you get in the Render method simply records drawing commands to be replayed on the render thread using a GPU-accelerated graphics context. |
|
Ah I see, that sounds great! I will definitely give this a try. |
|
Finally found some time to give the Here is my summary of the three approaches, as far as I am concerned:
For me the most compelling point of the double-buffer approach is that the UI remains responsive even when rendering very complex plots. Unfortunately, this advantage seems to be lost with the PictureRecorder approach. While this definitely also has its advantages, I am not sure if this is worth the extra complexity... |
|
@Jonarw my 2 cents here: I've used Regarding My app is still in early development stage so do not take my approach as 100% perfect. |
|
@Jonarw is this ready for another review? (should have time later today if so) |
This adds two new
PlotViews that can be used as a direct replacement of the existingOxyPlot.Avalonia.PlotView:OxyPlot.SkiaSharp.Avalonia.PlotViewOxyPlot.SkiaSharp.Avalonia.DoubleBuffered.PlotViewBoth of the new
PlotViews useOxyPlot.SkiaSharpfor rendering and only work when SkiaSharp is used as the rendering backend for Avalonia. As far as I understand, this is the case most of the time (but not always).OxyPlot.SkiaSharp.Avalonia.PlotViewoverrides AvaloniasVisual.Rendermethod, leases aSkCanvasfrom Avalonia in aICustomDrawOperationand uses this to render the plot. In terms of performance in comparison toCanvasRenderContext, this is a mixed bag; some examples are significantly faster, others quite a bit slower - at least that is the case on my machine.Similar to
OxyPlot.Avalonia.PlotView, updating and rendering always takes place on the UI thread. So probably in most cases there is not much incentive to switch fromOxyPlot.Avalonia.PlotViewtoOxyPlot.SkiaSharp.Avalonia.PlotView, it's rather just an alternative solution that has some advantages and some drawbacks depending on the specific application.Much more interesting is
OxyPlot.SkiaSharp.Avalonia.DoubleBuffered.PlotView: This usesOxyPlot.SkiaSharpto render the plot into an in-memory Bitmap in a double-buffer configuration. This has the nice implication that rendering and updating can be done completely on a background thread; the UI thread only needs to draw the finished bitmap on the screen. This can help a lot with keeping the UI responsive even when rendering very complicated plots.This PR is not quite finished, there are still some things I need to fix, improve and verify. But at least the
ExampleBrowseris fully functional and allows switching between the threePlotViews.I am not quite sure how much time I will have to work on this for the next couple of weeks, but I will definitely pick this up afterwards and finish it up. In the meantime, I thought I'd open this PR so people who are interested can play around with it. Feedback would be very welcome, espcially since I am fairly new to Avalonia (coming from WPF).