Implemented IDisposable for ScatterGL

[onur-akaydin]

Regarding the conversations in #4693

• ScatterGL.InitializeGL() rolled back to the previous version.
• Implemented IDisposable for ScatterGL and called CleanupGL() inside Dispose().
• CleanupGL() method is improved.
• Changed protected readonly int VerticesCount >> protected int VerticesCount
• Called Dispose() of individual IPlottables inside from Plot.Dispose()

Dear @StendProg, I would be happy if you review it.

[StendProg]

In addition we have a logic loop. If we call CleanupGL(), then we automatically run InitializeGL in Render(), which will recreate all GLPrograms without deleting them correctly. And probably recreating every time is not what we need.
I suggest to initialize initially all GLPrograms with null in the constructor. And in InitializeGL() create them only once if they are not null.

[StendProg]

Let's add correct Dispose() for ScatterGLCustom too:

    private bool _disposed = false;

    protected virtual void Dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                GL.Finish();
                IMarkersDrawProgram?.Dispose();
                IMarkersDrawProgram = null;                
            }
       
            _disposed = true;
        }

        base.Dispose();
    }
    
    ~ScatterGLCustom()
    {
        Dispose(false);
    }

[onur-akaydin]

In addition we have a logic loop. If we call CleanupGL(), then we automatically run InitializeGL in Render(), which will recreate all GLPrograms without deleting them correctly. And probably recreating every time is not what we need.
I suggest to initialize initially all GLPrograms with null in the constructor. And in InitializeGL() create them only once if they are not null.

I may not have fully understood you. But my most current usage scenario is like: I only use the CleanupGPUMemory() - formerly CleanupGL() method only during disposal. For example, when I remove a ScatterGL from PlottableList, or disposing the Plot (e.g. closing a Window containing a WpfPlotGL). So there is no situation where I have to call InitializeGL() again in the same ScatterGL object.

Regarding my first aim of UpdateableScatterGL, I implemented a custom UpdateVertices() method:

public class UpdateableScatterGLCustom : ScatterGLCustom
{
    public TaggedUpdateableScatterGLCustom(UpdateableScatterSourceDoubleArray data, IPlotControl control) : base(data, control)
    {
        UpdateableData.DataUpdated += UpdateableData_DataUpdated;
    }

    private void UpdateableData_DataUpdated(object? sender, EventArgs e)
    {
        UpdateVertices();
    }

    public UpdateableScatterSourceDoubleArray UpdateableData => Data as UpdateableScatterSourceDoubleArray;

    public virtual void UpdateVertices()
    {
        var dataPoints = Data.GetScatterPoints();

        if (Vertices == null || Vertices.Length != dataPoints.Count * 2)
        {
            Vertices = new double[dataPoints.Count * 2];
        }

        // Update vertices in-place
        for (int i = 0; i < dataPoints.Count; i++)
        {
            Vertices[i * 2] = dataPoints[i].X;
            Vertices[i * 2 + 1] = dataPoints[i].Y;
        }

        VerticesCount = Vertices.Length / 2;
        
        if (GLHasBeenInitialized)
        {
            GL.BindBuffer(BufferTarget.ArrayBuffer, VertexBufferObject);
            GL.BufferData(BufferTarget.ArrayBuffer, Vertices.Length * sizeof(double),
                Vertices, BufferUsageHint.DynamicDraw);
        }
    }
}

So I could not see a problem here regarding a logic loop. Please correct me if I'm wrong.

[onur-akaydin]

I couldn't understand why PR1 / Quick Test had an error.

[StendProg]

HI @onur-akaydin,

Great job, I think resource management for ScatterGL is noticeably better now.

So I could not see a problem here regarding a logic loop. Please correct me if I'm wrong.

I thought you would call CleanupGPUMemory many times from your derived class. Since this is not the case, there is really no problem.

2. Looks like implementing finalizers was a bad idea. They run in a separate thread, and cause an error on any OpenGL instruction. OpenTK does not support this scenario either. It looks like we have no choice but to accept that resources will remain unreleased unless Dispose() is called manually.
   Just remove the finalizers.
   I apologize for leading you down the wrong path.

[onur-akaydin]

Hi @StendProg, thanks for your feedback. I removed finalizers from ScatterGL and ScatterGLCustom. But still getting error at PR1 / Quick test & I don't know why.

[allrightsreserved]

hi @onur-akaydin ,Thank you for adding the update function, which can be used like in the original ScottPlot 4: After initializing a plot, new data can be added to the plot through the update function. I previously tried using ScottPlot V5.0.47 and added an Update function similar to ScottPlot4 directly to ScotterGL. It is indeed necessary to run InitializeGL() in the Update function every time, otherwise the new image cannot be rendered. This has a memory leakage problem:every time the Plot is cleared, the memory suddenly increases by several hundred MB when reinitializing:

public class ScatterGL : Scatter, IPlottableGL
{
    public IPlotControl PlotControl { get; }
    protected int VertexBufferObject;
    protected int VertexArrayObject;
    protected ILinesDrawProgram? LinesProgram;
    protected IMarkersDrawProgram? MarkerProgram;
    protected double[] Vertices;

    //modify
    //protected readonly int VerticesCount;
    protected int VerticesCount { get; set; }

    //added
    /// <summary>
    /// Replace Xs and Ys arrays with new ones
    /// </summary>
    public new void Update(double[] xs, double[] ys)
    {
        base.Update(xs,ys);

        var dataPoints = base.Data.GetScatterPoints();
        Vertices = new double[dataPoints.Count * 2];
        for (int i = 0; i < dataPoints.Count; i++)
        {
            Vertices[i * 2] = dataPoints[i].X;
            Vertices[i * 2 + 1] = dataPoints[i].Y;
        }
        VerticesCount = Vertices.Length / 2;

        InitializeGL();
    }

It seems that your program can solve this problem. I had copied your branch and relevant programs(UpdateableScatterGLCustom and UpdateableScatterSourceDoubleArray ) ,but encountered the following errors. If possible, could you please share some of your code then i can test it? thank you in advance :)

[StendProg]

Hi @allrightsreserved,

I could be wrong, but the current PR suggests that you just recreate ScatterGL with the updated data. While being able to clean up the old one by calling Dispose() on it.

If you want to develop the idea of updating the data without recreating ScatterGL, then multiple calls to
CleanupGPUMemory()->InitializeGL() will recreate the shader programs each time without properly cleaning them up.

[allrightsreserved]

Sure, appreciate it.

…

---- Replied Message ---- | From | ***@***.***> | | Date | 01/21/2025 22:46 | | To | ScottPlot/ScottPlot ***@***.***> | | Cc | BadMan ***@***.***>, Mention ***@***.***> | | Subject | Re: [ScottPlot/ScottPlot] Implemented IDisposable for ScatterGL (PR #4702) | Hi @allrightsreserved, I could be wrong, but the current PR suggests that you just recreate ScatterGL with the updated data. While being able to clean up the old one by calling Dispose() on it. If you want to develop the idea of updating the data without recreating ScatterGL, then multiple calls to CleanupGPUMemory()->InitializeGL() will recreate the shader programs each time without properly cleaning them up. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[onur-akaydin]

Hi @allrightsreserved

It seems that your program can solve this problem. I had copied your branch and relevant programs(UpdateableScatterGLCustom and UpdateableScatterSourceDoubleArray ) ,but encountered the following errors. If possible, could you please share some of your code then i can test it? thank you in advance :)

public static class HelperMethods
{
    public static void UpdateArray(this double[] source, ref double[] destination)
    {
        if (destination != null && source.Length == destination.Length)
        {
            // If the lengths are the same, copy the values to avoid allocating new arrays
            Array.Copy(source, destination, source.Length);
        }
        else
        {
            // If the lengths differ, allocate new arrays
            destination = source.ToArray();
        }
    }
}

If we can release this PR as it is, my strategy will be as follows:

• If I remove a ScatterGL from PlottableList, I call Dispose() immediately
• If I want to update the Xs and Ys values, I call UpdateVertices() method of UpdateableScatterGLCustom, to avoid unnecessary allocation.

public class UpdateableScatterGLCustom : ScatterGLCustom
{
    public UpdateableScatterGLCustom(UpdateableScatterSourceDoubleArray data, IPlotControl control) : base(data, control)
    {
        UpdateableData.DataUpdated += UpdateableData_DataUpdated;
    }

    private void UpdateableData_DataUpdated(object? sender, EventArgs e)
    {
        UpdateVertices();
    }

    public UpdateableScatterSourceDoubleArray UpdateableData => Data as UpdateableScatterSourceDoubleArray;

    protected override void InitializeGL()
    {
        LinesProgram = new LinesProgramCustom();
        JoinsProgram = new MarkerFillCircleProgram();

        VertexArrayObject = GL.GenVertexArray();
        VertexBufferObject = GL.GenBuffer();
        GL.BindVertexArray(VertexArrayObject);
        GL.BindBuffer(BufferTarget.ArrayBuffer, VertexBufferObject);
        GL.BufferData(BufferTarget.ArrayBuffer, Vertices.Length * sizeof(double), Vertices, BufferUsageHint.DynamicDraw);
        GL.VertexAttribLPointer(0, 2, VertexAttribDoubleType.Double, 0, IntPtr.Zero);
        GL.EnableVertexAttribArray(0);
        //Vertices = Array.Empty<double>();
        GLHasBeenInitialized = true;
    }

    public virtual void UpdateVertices()
    {
        var dataPoints = Data.GetScatterPoints();

        // Check if we can reuse the existing array
        if (Vertices == null || Vertices.Length != dataPoints.Count * 2)
        {
            Vertices = new double[dataPoints.Count * 2];
        }

        // Update vertices in-place
        for (int i = 0; i < dataPoints.Count; i++)
        {
            Vertices[i * 2] = dataPoints[i].X;
            Vertices[i * 2 + 1] = dataPoints[i].Y;
        }

        VerticesCount = Vertices.Length / 2;
        
        // If GL is initialized, update the buffer
        if (GLHasBeenInitialized)
        {
            GL.BindBuffer(BufferTarget.ArrayBuffer, VertexBufferObject);
            GL.BufferData(BufferTarget.ArrayBuffer, Vertices.Length * sizeof(double),
                Vertices, BufferUsageHint.DynamicDraw); // Note: Changed to DynamicDraw
        }
    }
}

I have no experience with GL, but I got no memory leakage by following this strategy & I hope this would be useful for you too.