Need Critique on Tutorial Series on Making Arkanoid-type Game.

[airstruck]

Looks good!

I have one small suggestion. If you store AABBs as a midpoint and a half-width and half-height, some of your code and calculations can be simplified quite a bit. This also means balls being stored as a center point and radius will be more in line with bricks and paddles.

For example, the cyclomatic complexity of the "check_rectangles_overlap" function can be reduced to 1, with half the LOC.

Before:

Code: Select all

function collisions.check_rectangles_overlap( a, b )
   local overlap = false
   local shift_b_x, shift_b_y = 0, 0
   if not( a.x + a.width < b.x  or b.x + b.width < a.x  or
           a.y + a.height < b.y or b.y + b.height < a.y ) then
      overlap = true
      if ( a.x + a.width / 2 ) < ( b.x + b.width / 2 ) then
         shift_b_x = ( a.x + a.width ) - b.x                    --(*1a)
      else 
         shift_b_x = a.x - ( b.x + b.width )                    --(*1b)
      end
      if ( a.y + a.height / 2 ) < ( b.y + b.height / 2 ) then
         shift_b_y = ( a.y + a.height ) - b.y                   --(*2) 
      else
         shift_b_y = a.y - ( b.y + b.height )                   --(*2)
      end      
   end
   return overlap, shift_b_x, shift_b_y                         --(*3)
end

After:

Code: Select all

function collisions.check_rectangles_overlap( a, b )
    local x_diff, y_diff = b.x - a.x, b.y - a.y
    local x_dist, y_dist = math.abs(x_diff), math.abs(y_diff)
    local x_overlap = a.width + b.width - x_dist
    local y_overlap = a.height + b.height - y_dist
    return x_overlap > 0 and y_overlap > 0,
        x_diff / x_dist * x_overlap, 
        y_diff / y_dist * y_overlap
end

Untested, but you get the idea. This should be easier to reason about, especially for beginners.

You could also create a separate function to get overlap and "shift" on a single axis. This should be even easier to read, and could be a step toward getting rid of those temporary tables created by ball_platform_collision (probably best not to create temporary tables every time you check for a collision).

Code: Select all

local function get_overlap( a_pos, b_pos, a_size, b_size )
    local diff = b_pos - a_pos
    local dist = math.abs(diff)
    local overlap = a_size + b_size - dist
    return overlap, diff / dist * overlap
end

function collisions.check_rectangles_overlap( a, b )
    local x_overlap, x_shift = get_overlap(a.x, b.x, a.width, b.width)
    local y_overlap, y_shift = get_overlap(a.y, b.y, a.height, b.height)
    return x_overlap > 0 and y_overlap > 0, x_shift, y_shift
end

[noway]

For example, the cyclomatic complexity of the "check_rectangles_overlap" function can be reduced to 1, with half the LOC. ...
This should be easier to reason about, especially for beginners.

Indeed, this method looks much simpler (especially with this nice trick of two separate overlap checks) than the current one I use.
I remember I have seen it, but I can't recall why I have decided to ignore it.
I'll update the code to use this method. Thanks.

[airstruck]

I'll update the code to use this method. Thanks.

Glad to hear that! This is the kind of thing a lot of Love tutorials miss. Planning data model carefully can go a long way.

I just noticed something else. In construct_level, pairs should be ipairs. Iteration order of pairs is undefined, so it won't necessarily put your blocks in the right order.

On a side note, I'd probably use a string to hold that kind of level data; it's just easier to look at and edit.

Code: Select all

levels.sequence[1] = [[
           
           
# # ### # #
# # #   # #
### ##   # 
# # #    # 
# # ###  # 
           
]]

Downside to that is you'll need some (very simple) pattern matching to construct your levels. Other than that, construct_level won't change much at all.

Code: Select all

function bricks.construct_level( level_bricks_arrangement )
   local row_index = 0
   for row in level_bricks_arrangement:gmatch('(.-)\n') do
      row_index = row_index + 1
      for col_index, bricktype in row:gmatch('()(.)') do
         -- do your stuff here
      end
   end
end

I guess you can't assume the audience is familiar with Lua's pattern matching, but maybe it could go in an appendix.

[noway]

I just noticed something else. In construct_level, pairs should be ipairs. Iteration order of pairs is undefined, so it won't necessarily put your blocks in the right order.

I don't think it's true.

Code: Select all

function bricks.construct_level( level_bricks_arrangement )
   bricks.no_more_bricks = false
   for row_index, row in pairs( level_bricks_arrangement ) do
      for col_index, bricktype in pairs( row ) do
	 if bricktype ~= 0 then
	    local new_brick_position_x = bricks.top_left_position_x +
	       ( col_index - 1 ) *
	       ( bricks.brick_width + bricks.horizontal_distance )
	    local new_brick_position_y = bricks.top_left_position_y +
	       ( row_index - 1 ) *
	       ( bricks.brick_height + bricks.vertical_distance )
	    local new_brick = bricks.new_brick( new_brick_position_x,
						new_brick_position_y )
	    table.insert( bricks.current_level_bricks, new_brick )
	 end
      end
   end
end

Each brick position is computed from 'col_index' and 'row_index'; the order of processing of the table defining bricks layout is irrelevant.

On a side note, I'd probably use a string to hold that kind of level data; it's just easier to look at and edit.
...
I guess you can't assume the audience is familiar with Lua's pattern matching, but maybe it could go in an appendix.

This can be a good solution for medium-sized maps, where tables are too inconvenient and Tiled is overkill.
I've put an idea about this appendix into the todo-list.

[airstruck]

I just noticed something else. In construct_level, pairs should be ipairs. Iteration order of pairs is undefined, so it won't necessarily put your blocks in the right order.

I don't think it's true.

My mistake. If the order of the bricks in current_level_bricks doesn't matter, I suppose pairs is alright. But why use pairs here anyway? Pairs isn't doing anything here that ipairs or numeric for wouldn't do, besides possibly doing things in a weird order, and definitely going more slowly (JIT can't compile pairs). The arrays are dense, we don't need to handle holes or arbitrary keys, so we don't need pairs.

I wouldn't expect a novice user to understand/notice that pairs is only acceptable in this case because we don't care about the order of elements in current_level_bricks. And what if we start caring about the order later; maybe we want to rely on the bricks being y-sorted for draw order at some point (funky perspective graphics)?

This doesn't seem like a good practice to encourage; it's really a job for ipairs or numeric for (at least, those would be the idiomatic ways to do it).

[noway]

This doesn't seem like a good practice to encourage; it's really a job for ipairs or numeric for (at least, those would be the idiomatic ways to do it).

Ok. I've pushed the change to the github.

[noway]

I've updated two more parts in the wiki:
3-4: Spawning Bonuses
3-5: Bonus Effects

In parts 1-03 -- 3-05 I've changed the collision detection algorithm to the one suggested by airstruck. The temporary tables are still there, though. To change representations of the game objects to get rid of these tables will require a lot of work, which I do not want to engage in right now. Alternative way to avoid them could be to insert check on game object type into `collisions.check_rectangles_overlap( a, b )`, something like:

Code: Select all

function collisions.ball_platform_collision( ball, platform )
   local overlap, shift_ball
   overlap, shift_ball = collisions.check_rectangles_overlap( platform, ball )   
   .....
end

function collisions.check_rectangles_overlap( a, b )
   if has_key(a, "width") then 
      local a_center_x, a_center_y = a.position.x + a.width/2, a.position.y + a.height / 2
      local a_halfwidht, a_halfheight = a.width / 2, a.height / 2
   elseif has_key( a, "radius") then
      local a_center_x, a_center_y = a.position.x, a.position.y
      local a_halfwidth, a_haflheight = a.radius, a.radius
   end
   .....
   -- same for b
   .....
   local x_overlap, x_b_shift = collisions.overlap_along_axis(
      a_center_x, a_halfwidth, b_center_x, b_halfwidth )
   local y_overlap, y_b_shift = collisions.overlap_along_axis(
      a_center_y, a_halfheight, b_center_y, b_halfheight )
   local overlap = ( x_overlap > 0 ) and ( y_overlap > 0 )
   return overlap, vector( x_b_shift, y_b_shift )
end

Another way to mitigate this problem of temporary tables could be to simply predeclare a couple of tables in advance and use them for all the checks.

Code: Select all

collisions.A = { center_x = 0, center_y = 0, halfwidth = 0, halfheight = 0 }
collisions.B = { center_x = 0, center_y = 0, halfwidth = 0, halfheight = 0 }

function collisions.ball_platform_collision( ball, platform )
   local overlap, shift_ball
   collisions.A.center_x = platform.position.x + platform.width / 2
   collisions.A.center_y = platform.position.y + platform.height / 2
   collisions.A.halfwidth = platform.width / 2
   collisions.A.halfheight = platform.height / 2
   collisions.B.center_x = ball.position.x
   collisions.B.center_y = ball.position.y,
   collisions.B.halfwidth = ball.radius,
   collisions.B.halfheight = ball.radius
   overlap, shift_ball = collisions.check_rectangles_overlap( collisions.A, collisions.B )   
   if overlap then
      ball.platform_rebound( shift_ball, platform )
   end      
end

The third alternative I can think of is to store 'center', 'halfwidht' and 'halfheight' fields in each game object. Since both 'position' and 'center' fields have to be updated simultaneously, it will be necessary to use setters methods to change these fields, instead of modifying them directly.

[ivan]

Hello noway, it's a good move using the centers of objects.
That will make your collision code and math a little bit simpler.
Unfortunately, the advantage is lost when you manually calculate the centers of objects before each collision check.
Keep in mind that collision checks are usually more frequent than rendering.
I think you'll be fine if you just store the center positions and extents -
the only issue as you know is that love2d draws rects based on the top/left corner.
All you have to do is tweak your rendering code to offset the rects you are drawing.

[noway]

Unfortunately, the advantage is lost when you manually calculate the centers of objects before each collision check.

Ok, I'll keep that in mind when I get back to that question.

[noway]

I've updated one more part in the wiki: implementation of a "glue" bonus, which makes the ball stick to the platform.

P.S.: I've thought that I should start to add some images here to indicate what is going on.