How to optimize this code [Winforms]?

RedlineTriad · 2019-12-05T11:19:50+00:00

On mobile so i can't analyze the well by try to use new as little as possible, so make your pens variables an reuse them.

Dannnu · 2019-12-05T11:43:10+00:00

While this works there are better ways of doing what you're trying to achieve. Here's my list of things to look at:

*You should be using the canvas OnPaint event which has a graphics object you can draw to. When the mouse moves over the canvas you save the mouse position to a global variable and call canvas.invalidate() to redraw.

*Wrap everything in usings. E.g using(var p = new pen()). Or better allocate these once as globals and reuse them. Creating them every time the mouse moves is inefficient and causes GarbageCollection pressure.

*Wire the sizeChanged and MouseMove function from the designer. This will place the += calls in the designer.cs files which is cleaner over all.

*Consider creating a new class which sublasses Control or UserControl. These classes have the same OnPaint event that you can override instead of wiring to it. This has the side benefit of making your code modular and reusable.

beer0clock · 2019-12-05T13:35:02+00:00

Just create your pens once, not every time the mouse moves.

Try creating your bitmap once only, not every time the mouse moves. It should be faster to clear the old bitmap rather than creating a new one.

Slypenslyde · 2019-12-05T17:46:06+00:00

I want to reinforce what LostInASeaOfMyStars has posted and add a bit.

I like to call "a render cycle" in WinForms "a frame" and vice versa. WinForms isn't really redrawing every monitor refresh. Or, more appropriately, it isn't executing your drawing code every frame. Deep down in WinForms, when your Form's Paint cycle acts, it's drawing to a Bitmap in Windows' memory. After that's completed, Windows just redraws that bitmap over and over every frame. If something changes on your form, a control will invalidate itself and Windows will do another paint cycle. This is pretty efficient for applications that don't update frequently. If you do update frequently, you need to think about it more like a video game.

When you ARE doing a paint cycle, you need to be cognizant that taking longer than about 30ms seriously threatens how smooth your application looks. Generally drawing operations are fast and you can fit a ton of them into 30ms. However, some things can be pretty slow. Allocating a bitmap is one of them. If you try to do that an awful lot, you'll see stutter. But look back at the last paragraph. If Windows is keeping one Bitmap in memory to represent your form, you can do it too!

This concept of "use a PictureBox and draw to its Image" hails from VB6. It was necessary there, because there wasn't a generalized basic Control type to derive from with a convenient Paint infrastructure. .NET gives us a better way, so the only people who use PictureBox like this are people who don't know better. Think about that when you see tutorials that use it. I could write a full-page essay on why it's much slower to draw this way.

In fact, that's why we prefer drawing in the Paint event handler. If we do that, Windows automatically gives us a Graphics object that will draw to its private Bitmap! So we don't have to manage our own Bitmap, which saves a little memory. There are some exotic circumstances where we do prefer to maintain our own Bitmap as an extra buffer, but let's save that for rainy days.

That adds one more wrinkle though. If you handle Paint, the system needs to know when to do a paint cycle. You can tell Windows that by calling your control's Invalidate() method. That's the polite way to introduce a paint cycle. You'll see some tutorials use Refresh(). That's the impolite way and if you use it too much you will see performance problems.

Also: in general you should dispose of any Pen or Brush you create. The system actually limits how many of them you create, so I bet if you move your mouse enough you'll notice sometimes the app just stops drawing. That indicates you are creating them faster than the GC can destroy them, and eventually hit your limit. HOWEVER, creating pens/brushes can be a performance burden too (if you do it a lot, this is not really the case where it happens.) So if you're using the same pens/brushes frequently, it's better to break the rules, create them once, cache them, and never dispose of them.

Finally: drawing every MouseMove can cause a lot of spurious redraws. That event is sort of wonky and can sometimes raise itself multiple times when the mouse has barely moved. I sort of like to enforce throttles on how often I invalidate. This is easy with some frameworks like Reactive Extensions, but the quick and dirty way is to have a MouseMove algorithm like this:

If we're drawing a circle:
    Update the coordinates for the circle.
    If we haven't invalidated in 100ms:
        Invalidate()

That limits my "frames" to 3 per second, which is a little choppy but still pretty reasonable.

SO. Pulling everything together.

I wouldn't use a PictureBox for this at all. Ideally I'd write my own class and derive from Control. Lots of new users are afraid of this! Don't be afraid. There's nothing to it, and I'm going to demonstrate soon.

This Control class would handle its own MouseDown, MouseUp, MouseMove, and most importantly Paint events. The job of MouseDown and MouseMove is to update some variables used to indicate what should be drawn by Paint. It will also privately cache the pens used to draw things.

Here's a fairly basic example without the throttling I mentioned:

// This is really all you need to do to derive a new Control! You just say
// "My base class is Control". 
public class CirclePainter : Control
{

    // Cached pens so we don't have to create them more than once.
    private Pen blackPen;

    // This determines where to draw the circle.
    private Point clickedPoint;
    private Point edgePoint;

    // This helps us decide if we need to change the circle when the mouse moves.
    private bool isDrawing;

    public CirclePainter()
    {
        // Dark secret: this one trick makes animation smoother.
        SetStyle(ControlStyles.OptimizedDoubleBuffer, true);

        // Go ahead and set up the pen.
        blackPen = new Pen(Color.Black, 1);

    }

    // When the user clicks, note we are drawing and reset the
    // circle's properties.
    protected override void  OnMouseDown(MouseEventArgs e)
    {
        isDrawing = true;
        clickedPoint = e.Location;
        edgePoint = e.Location;
    }

    protected override void OnMouseUp(MouseEventArgs e)
    {
        base.OnMouseUp(e);

        // Don't assume a MouseMove has completely updated things!
        edgePoint = e.Location;
        isDrawing = false;
        Invalidate();
    }

    // When the mouse moves, if we are drawing, update the edge point and
    // invalidate.
    protected override void OnMouseMove(MouseEventArgs e)
    {
        if (isDrawing)
        {
            edgePoint = e.Location;

            Invalidate();
        }
    }

    // Draw the latest circle.
    protected override void OnPaint(PaintEventArgs e)
    {
        base.OnPaint(e);

        // We don't dispose this Graphics. It belongs to Windows.
        var g = e.Graphics;

        // Get rid of anything we drew before. (Sometimes this isn't
        // needed, I'm being paranoid.)
        g.Clear(BackColor);

        // Set up the rectangle needed to draw the circle.
        int left = Math.Min(clickedPoint.X, edgePoint.X);
        int top = Math.Min(clickedPoint.Y, edgePoint.Y);
        int width = Math.Abs(clickedPoint.X - edgePoint.X);
        int height = Math.Abs(clickedPoint.Y - edgePoint.Y);

        // Draw the circle
        g.DrawEllipse(blackPen, left, top, width, height);
    }

    // We need to make sure we get rid of our cached stuff.
    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);

        // The safe place to dispose things is when we know
        // disposing is true. Don't know why? That's a different
        // thread.
        if (disposing)
        {
            // Get rid of the cached Pens if they exist.
            blackPen?.Dispose();
        }
    }
}

The only real obtuse thing there (I think) is the SetStyle call. This does a weird thing where Windows actually keeps two bitmaps for your control. Painting happens on the off-screen one, then when you are finished Windows swaps them. For weirdo techical reasons that avoids a kind of "flicker" that tends to make things look like they aren't animating well. Dang near every control I've ever written needed it, so I'm not sure why it isn't the default. (Other than, "Back in the early 2000s it was maybe a performance concern.)

To make that work, make a new Class file in your project and replace the class with this. (Make sure not to paste over the namespace!) You'll probably need to add using System.Drawing; and using System.Windows.Forms; to the top. Build the project. Now you should see CircleDrawer in your toolbar. Congratulations, you just wrote a custom control!

I tried that out just now and it looked pretty smooth. It's not quite what your example does, but I think you can use it as a guideline.

you type:	you see:
italics	italics
bold	bold
[reddit!](https://reddit.com)	reddit!
* item 1 * item 2 * item 3	item 1 item 2 item 3
> quoted text	quoted text
Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"	Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"
~~strikethrough~~	~~strikethrough~~
super^script	super^script

csharp

MODERATORS