HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Using graphics to draw on form, in C#

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
graphicsdrawformusing

Problem

I have a form, which I want to draw on-top some shapes. To be exact, rectangles and lines.

This is the "Redraw" method. I call this method every 100ms from a different thread.

public void Redraw()
    {
        g.FillRectangle(b, new Rectangle(13, 13, (int)drawMapX + 2, (int)drawMapY + 2)); /* Draw background */

        try
        {
            foreach (Location drawPoint in points)
            { 
                g.DrawLine(new Pen(y), new Point((int)Math.Round(masterpos.X), (int)Math.Round(masterpos.Y)), new Point((int)Math.Round(drawPoint.X), (int)Math.Round(drawPoint.Y)));
                g.FillRectangle(r, drawPoint.X, drawPoint.Y, 2, 2);

            }
            g.FillRectangle(gr, masterpos.X, masterpos.Y, 2, 2); 

            g.DrawString(Math.Round(pos.X).ToString() + " " + Math.Round(pos.Y), drawFont, gr, masterpos.X + 2, masterpos.Y - 20, centered);
        }
        catch { }
    }


Where "g" is

Graphics g = Application.OpenForms["Client"].CreateGraphics();


Nothing too interesting about it, draws some shapes in some given places, and some strings

My question: Is my thread safe and memory-leak proof?

void CreateDrawingThread() {new Thread(() => {
            Thread.CurrentThread.IsBackground = true;
            do
            {
                try
                {

                    Thread.Sleep(100);
                    Redraw();
                }
                catch(Exception ex) { Console.WriteLine(ex.ToString()); return; }
            } while (true);

        }).Start();}


Because as you can see, I don't do anything like g.Flush every redraw, I was wondering if it stores old "draws" in memory, or if I'm doing something in a cpu costraining way.

Solution

new Pen(y)


This should be disposed.

Graphics g = Application.OpenForms["Client"].CreateGraphics();


I wonder why are you getting the Graphics object like this? To redraw something you should actually handle the Paint event or override the OnPaint method where you get a ready to use Graphics object from the system. And of course like @Cody Gray said, you need to dispose a graphics object acquired this way.

Those try/catch blocks are suspicious too. There is nothing that could throw an exception. If this runs every 100ms there is no room for exception handling. This would be very bad for the performance.

Code Snippets

Graphics g = Application.OpenForms["Client"].CreateGraphics();

Context

StackExchange Code Review Q#151875, answer score: 4

Revisions (0)

No revisions yet.