3
\$\begingroup\$

I am looking for any performance insights or better coding style for my rendering method.

public Bitmap bmp;
public void render()
{
    Brush[] brushes = new SolidBrush[NumOfShapes];
    PointF[][] points = new PointF[NumOfShapes][];
    float[] tensions = new float[NumOfShapes];

    // We have multiply shapes, every shape has parameters for brush,points,tension
    for (int i = 0; i < NumOfShapes; i++)
    {
        int curT = i * ParameterNumber;
        brushes[i] = getBrush(curT); // get value and clip it if need
        points[i] = getPoints(curT); 
        tensions[i] = getTension(curT);
    }

    using (Graphics graphics = Graphics.FromImage(bmp))
    {
        graphics.Clear(Color.Black);
        graphics.SmoothingMode = Globals.smoothingMode;
        for (int i = 0; i < NumOfShapes; i++)
        {
            graphics.FillPolygon(brushes[i], points[i]);
            // I also use shapes bellow
            //graphics.FillClosedCurve(brushes[i], points[i]);
            //graphics.DrawPolygon(new Pen(brushes[i],tensions[i]), points[i]);
            //graphics.FillClosedCurve(brushes[i], points[i], FillMode.Alternate, tensions[i]);
            //graphics.DrawCurve(new Pen(brushes[i], tensions[i]), points[i]);
        }

        graphics.Flush();
    }
}

With this, I am currently getting ~2000 bitmaps per second.

Generated bitmap is not displayed, I just get pixels with Marshal.Copy() and dispose bitmap. So is there a quicker way to render bitmap from shapes and get pixels.


private SolidBrush getBrush(int curT)
{
    parameters[curT] = clippingColor(parameters[curT]);
    parameters[curT + 1] = clippingColor(parameters[curT + 1]);
    parameters[curT + 2] = clippingColor(parameters[curT + 2]);
    parameters[curT + 3] = clippingColor(parameters[curT + 3]);
    Color color = Color.FromArgb(Convert.ToInt32(parameters[curT]), Convert.ToInt32(parameters[curT + 1]), Convert.ToInt32(parameters[curT + 2]), Convert.ToInt32(parameters[curT + 3]));
    return new SolidBrush(color);
}

private float getTension(int curT)
{
    parameters[curT + 4] = clippingTension(parameters[curT + 4]);
    return Convert.ToSingle(parameters[curT + 4]);
}

private PointF[] getPoints(int curT)
{
    PointF[] tempPoints = new PointF[(ParametersNum - 5) / 2];
    int pointsIndex = 0;
    int endSize = curT + ParametersNum;
    for (int j = curT + 5; j < endSize; j += 2)
    {
        parameters[j] = clippingPointsX(parameters[j]);
        parameters[j + 1] = clippingPointsY(parameters[j + 1]); 
        tempPoints[pointsIndex] = new PointF(Convert.ToSingle(parameters[j]), Convert.ToSingle(parameters[j + 1]));
        pointsIndex++;
    }
    return tempPoints;
}

Update

I change bitmap pixel format to Format32bppPArgb and added rendering settings to graphics object as described in this guide: Rendering fast with GDI+ - What to do and what not to do! With this I am getting around ~2300 rendered bitmaps per second.

graphics.CompositingMode = CompositingMode.SourceOver;
graphics.CompositingQuality = CompositingQuality.HighSpeed;
graphics.PixelOffsetMode = PixelOffsetMode.None;
graphics.InterpolationMode = InterpolationMode.Default;
\$\endgroup\$
  • 2
    \$\begingroup\$ What does getBrush do? Do you dispose of the GDI objects at any point? \$\endgroup\$ – Ron Beyer Nov 27 '16 at 4:12
  • \$\begingroup\$ Hmm, I believe GDI objects gets dispose when method is out of scope, or should I explicitly dispose them? I added code \$\endgroup\$ – TomazStoiljkovic Nov 27 '16 at 4:29
  • 1
    \$\begingroup\$ Yes, you should be disposing them explicitly, there is a limit to how many GDI objects you can generate and if you just let GC take care of it you may be causing memory leaks by not disposing them. GDI objects are especially important because they use unmanaged resources. \$\endgroup\$ – Ron Beyer Nov 27 '16 at 13:53
2
\$\begingroup\$

It looks like you may come from a Java or scripting background. For .NET, the standard recommends PascalCasing for method names. This would turn your methods into:

Render()
GetBrush(...)
GetPoints(...)
GetTension(...)

Variable names should be camelCasing, where the first letter is lower case and the remaining words are capitalized. I'm not sure if NumOfShapes or ParameterNumber are class properties (Pascal Cased) or class-level fields (camel Cased), but you should follow the standard.

As far as your method names, you should avoid GetXXX which suggests that they should be properties. I would recommend the following changes:

GetBrush(...) -> CreateBrush(...)
GetPoints(...) -> GeneratePoints(...)
GetTension(...) -> CalculateTension(...)

For your variable names, curT doesn't help me understand what the parameter is for. :

for (int i = 0; i < NumOfShapes; i++)
{
    int curT = i * ParameterNumber;
    brushes[i] = getBrush(curT); // get value and clip it if need
    points[i] = getPoints(curT); 
    tensions[i] = getTension(curT);
}

The definition and modification must be elsewhere. I'd have to go searching for it. It would be much simpler if you named it something meaningful, even if it is long, like currentImageIteration.

As far as coding goes, always, always dispose of GDI objects when you are done using them. You can get some pretty strange errors (OutOfMemoryException for example) when hitting the GDI object limit. Because GDI uses unmanaged objects, you should always dispose of them when you are done, see this question, and this question. The object will be disposed at some time in the future, but because the GC doesn't clean up managed objects, this will cause a memory leak.

Other than that, everything seems OK.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for?Browse other questions tagged or ask your own question.