19
\$\begingroup\$

I have created canvas (800x400) - and filled it with a grid. It works but the rendering of the grid (lines) takes around 3 seconds, which seems excessively long. Am I doing something wrong?

var drawGrid = function(w, h, id) {
    var canvas = document.getElementById(id);
    var ctx = canvas.getContext('2d');
    ctx.canvas.width  = w;
    ctx.canvas.height = h;


    for (x=0;x<=w;x+=20) {
        for (y=0;y<=h;y+=20) {
            ctx.moveTo(x, 0);
            ctx.lineTo(x, h);
            ctx.stroke();
            ctx.moveTo(0, y);
            ctx.lineTo(w, y);
            ctx.stroke();
        }
    }

    };


    drawGrid(800, 400, "grid");
<canvas id="grid"></canvas>

\$\endgroup\$
  • \$\begingroup\$ Have you tried using a background image on the canvas? You should be able to use a repeating SVG (size mutable) background. \$\endgroup\$ – Quill Dec 21 '15 at 23:35
  • \$\begingroup\$ No, I haven't. I'll try and see the difference. \$\endgroup\$ – Wasteland Dec 21 '15 at 23:39
  • \$\begingroup\$ 3 seconds is nuts! whats ya computer, thats instant on mine. \$\endgroup\$ – PAEz Dec 24 '15 at 10:08
16
\$\begingroup\$

Quill already did a good job speeding up your code, so I'll focus on the code you have right now.


Give me the object!

(Not how to get it)

Your drawGrid function takes the id of a DOM element that the function is expected to find on its own. However, this is not good practice. Instead, you should pass the DOM element itself. This could also speed up your code by a lot if used correctly.

For more on why it's bad practice, think about it this way: if you need to do some specific checking on an element after you've found it before you are ready to give it to the function, how are you supposed to pass that prepared element to the function?

Don't let the function worry about how to get what it needs; just give it what it needs.


Fetch me the context! Now throw it away!

var ctx = canvas.getContext('2d');

This is created every time your function is called. Assuming more drawing will take place in this code, why not put both the canvas and the context in a global scope? There is no point in finding the context, putting it in a variable, and then destroying it at the end of the function every time it is called.


Ask the canvas where the context is. Then, go ask the context where the canvas is.

var canvas = document.getElementById(id);
...
ctx.canvas.width  = w;
ctx.canvas.height = h;

This makes absolutely zero sense. First, you use the canvas to get the ctx. Then, you use the ctx to get the canvas again by accessing its properties. Why not just use the canvas that you literally just defined?


There is always another way

Yes, the way you are currently forming the grid is very slow. As Quill has already shown, there are other options:

  • Render it as SVG (Quill has already shown this)
  • Get a single image that is the portion of a grip, then copy pasta that image around the area you need.
  • Draw out the squares.

I did not test these out, so I don't know how much of a speed boost they provide (if any). However, feel free to try some of them.

\$\endgroup\$
  • \$\begingroup\$ Thank you. Could you clarify your first point "Give me the object". Are you saying that I should create the canvas element entirely from javascript or hard code the canvas id in the function definition? I wanted to make it as generic as possible to reuse the same function for creating other canvas on the same website. Is it discouraged from? Thank you \$\endgroup\$ – Wasteland Dec 22 '15 at 10:58
  • \$\begingroup\$ Btw, I should probably clarify that I'm doing a maths website to illustrate/simulate some maths concepts for learners to practise. \$\endgroup\$ – Wasteland Dec 22 '15 at 11:00
  • \$\begingroup\$ @Wasteland I'm saying that you should pass the already-found JavaScript object into the function, rather than just the ID. \$\endgroup\$ – SirPython Dec 22 '15 at 22:44
  • \$\begingroup\$ The excessive drawing doesn't help for performance either \$\endgroup\$ – Deian Jul 19 '16 at 19:00
  • \$\begingroup\$ I may be incorrect, but I believe you are supposed to request the context every frame, as the context is not valid the next frame. That may possibly be some other system that I've used that I'm thinking of, though. \$\endgroup\$ – Eric Blade Jul 20 '16 at 5:25
21
\$\begingroup\$

Try using a SVG object on the canvas:

<svg width="100%" height="100%" xmlns="http://www.w3.org/2000/svg">
    <defs>
        <pattern id="smallGrid" width="8" height="8" patternUnits="userSpaceOnUse">
            <path d="M 8 0 L 0 0 0 8" fill="none" stroke="gray" stroke-width="0.5" />
        </pattern>
        <pattern id="grid" width="80" height="80" patternUnits="userSpaceOnUse">
            <rect width="80" height="80" fill="url(#smallGrid)" />
            <path d="M 80 0 L 0 0 0 80" fill="none" stroke="gray" stroke-width="1" />
        </pattern>
    </defs>

    <rect width="100%" height="100%" fill="url(#smallGrid)" />
</svg>

produces a grid like this:

enter image description here

whereas changing fill="url(#smallGrid)" to fill="url(#grid)" produces the following:

enter image description here

Which then you can feed into the canvas like the following:

var drawGrid = function(w, h, id) {
    var canvas = document.getElementById(id);
    var ctx = canvas.getContext('2d');
    ctx.canvas.width  = w;
    ctx.canvas.height = h;
    
    var data = '<svg width="100%" height="100%" xmlns="http://www.w3.org/2000/svg"> \
        <defs> \
            <pattern id="smallGrid" width="8" height="8" patternUnits="userSpaceOnUse"> \
                <path d="M 8 0 L 0 0 0 8" fill="none" stroke="gray" stroke-width="0.5" /> \
            </pattern> \
            <pattern id="grid" width="80" height="80" patternUnits="userSpaceOnUse"> \
                <rect width="80" height="80" fill="url(#smallGrid)" /> \
                <path d="M 80 0 L 0 0 0 80" fill="none" stroke="gray" stroke-width="1" /> \
            </pattern> \
        </defs> \
        <rect width="100%" height="100%" fill="url(#smallGrid)" /> \
    </svg>';

    var DOMURL = window.URL || window.webkitURL || window;
    
    var img = new Image();
    var svg = new Blob([data], {type: 'image/svg+xml;charset=utf-8'});
    var url = DOMURL.createObjectURL(svg);
    
    img.onload = function () {
      ctx.drawImage(img, 0, 0);
      DOMURL.revokeObjectURL(url);
    }
    img.src = url;
}
drawGrid(800, 400, "grid");
<canvas id="grid"></canvas>

Of course, you'll need to play around with the sizes of the grid to match what you want. However you'll find that this method is much faster.

\$\endgroup\$
  • \$\begingroup\$ It may be faster to prepare the data, blob it, and do all that stuff to it before-hand (maybe as part of an initialization process). Then, when it comes time to draw the data, it should be faster as the data will be all ready to go. \$\endgroup\$ – SirPython Dec 22 '15 at 0:43
  • \$\begingroup\$ Thank you Quill - I have a very limited experience with SVG - can it be easily manipulated with js? \$\endgroup\$ – Wasteland Dec 22 '15 at 11:08
  • \$\begingroup\$ SVG is very similar to HTML in that it follows the XML-like structure of <tag>, and yes, it can be easily manipulated with JavaScript and like in the example, able to fill itself as the background with ease. \$\endgroup\$ – Quill Dec 22 '15 at 11:10
  • \$\begingroup\$ I would suggest moving that SVG markup in something like a script tag. That way, your SVG won't be in the JS. Another alternative, if ES6 is possible, is template strings. At least this avoids the \ on the end of each line. \$\endgroup\$ – Joseph Dec 22 '15 at 13:00
16
+50
\$\begingroup\$

You're rendering the same line too many times.

To create the grid you need to draw (w/stepSize) vertical lines and (h/stepSize) horizontal lines. Total: (w+h)/stepSize lines

You used two nested loops, and you drew: (w/stepSize)*(h/stepSize) = (w * h)/(stepSize * stepSize) lines

To give you a perspective few examples:

For w=1000, h=1000, stepSize=20. Your function would draw 2500 lines where only 100 lines would suffice.

For w=800, h=400, stepSize=20. Your function would draw 800 lines where only 60 lines would suffice.

In addition the canvas resize you should look to put that outside of that function and do it only once at the beginning of the program!

// the canvas logic should be done once somewhere else 
var w = 800;
var h = 400;
// grid step
var step = 20; 
var canvasElementId = 'grid';


var canvas = document.getElementById(canvasElementId);
// this is how you resize the canvas
canvas.width  = w;
canvas.height = h;

var ctx = canvas.getContext('2d');


// the render logic should be focusing on the rendering 
var drawGrid = function(ctx, w, h, step) {
    ctx.beginPath(); 
    for (var x=0;x<=w;x+=step) {
            ctx.moveTo(x, 0);
            ctx.lineTo(x, h);
    }
    // set the color of the line
    ctx.strokeStyle = 'rgb(255,0,0)';
    ctx.lineWidth = 1;
    // the stroke will actually paint the current path 
    ctx.stroke(); 
    // for the sake of the example 2nd path
    ctx.beginPath(); 
    for (var y=0;y<=h;y+=step) {
            ctx.moveTo(0, y);
            ctx.lineTo(w, y);
    }
    // set the color of the line
    ctx.strokeStyle = 'rgb(20,20,20)';
    // just for fun
    ctx.lineWidth = 5;
    // for your original question - you need to stroke only once
    ctx.stroke(); 
};


drawGrid(ctx, w, h, step);
<canvas id="grid"></canvas>

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Congratulations on writing a very good answer, feel free to stick around :-) \$\endgroup\$ – Mast Jul 19 '16 at 20:11
5
\$\begingroup\$

You are running three statements repeatly for no reason. Try this:

var drawGrid = function(w, h, id) {
    var canvas = document.getElementById(id);
    var ctx = canvas.getContext('2d');
    ctx.canvas.width = w;
    ctx.canvas.height = h;


    for (x = 0; x <= w; x += 20) {
        ctx.moveTo(x, 0);
        ctx.lineTo(x, h);
        for (y = 0; y <= h; y += 20) {
            ctx.moveTo(0, y);
            ctx.lineTo(w, y);
        }
    }
    ctx.stroke();

};


drawGrid(800, 400, "grid");

Notice where I moved the top 3 statements of the inner for loop outside. Big difference.

\$\endgroup\$
  • \$\begingroup\$ I really don't get it - why do you need the nested loop? \$\endgroup\$ – Deian Jul 20 '16 at 20:09
  • \$\begingroup\$ @deian The inner loop is to draw the vertical lines. Based on other's suggestions here, you can move the ctx.stroke() to outside the loop. \$\endgroup\$ – Will Sep 17 '16 at 22:10
2
\$\begingroup\$

Fun question, the bounty should obviously go to deain.

Nobody seemed to have noticed the fact that x and y are globals.

Other than that I would use a line function (a bit slower, but more intuitive to read) to the line drawing.

//Draw the line on the context, caller needs to worry about `stroke()`
function pencilLine( ctx, fromX, fromY, toX, toY ){
  ctx.moveTo( fromX, fromY );
  ctx.lineTo( toX, toY );
}
\$\endgroup\$
  • \$\begingroup\$ Why not include ctx.stroke() in the function too? \$\endgroup\$ – 200_success Jul 20 '16 at 16:45
  • \$\begingroup\$ Because if you draw say 200 lines, you don't want to do 200 strokes. \$\endgroup\$ – konijn Jul 20 '16 at 16:48
  • \$\begingroup\$ I tend to prefix those routines that are meant for mass usage with pencil to indicate they do not ink in the lines so to speak. \$\endgroup\$ – konijn Jul 20 '16 at 16:48
  • \$\begingroup\$ kudos for you @konijn x,y should be local var of course! \$\endgroup\$ – Deian Jul 20 '16 at 20:08

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.