feat: add kind parameter for beginShape function (supporting LINES, POINTS, TRIANGLE_FAN, TRIANGLE_STRIP, and TRIANGLES)

[georgeibrahim1]

I want to know your feedback @lee2sman

Examples :

require("L5")

function setup() 
  size(400, 400)
  windowTitle("beginShape(LINES) example")
  
  fill(0)
  beginShape(LINES)
  vertex(30, 20);
  vertex(85, 20);
  vertex(85, 75);
  vertex(30, 75);
  endShape()
  describe("custom shape with beginShape(LINES) function, vertices and endShape()")
end

require("L5")

function setup() 
  size(400, 400)
  windowTitle("beginShape(POINTS) example")
  
  fill(0)
  beginShape(POINTS)
  for i=0,10 do
    vertex(random(width),random(height))
  end
  endShape()
  describe("custom shape with beginShape(POINTS) function, vertices and endShape()")
end

Note : love.graphics.points() draws small squares as I mentioned in #9

require("L5")

function setup() 
  size(400, 400)
  windowTitle("beginShape(TRIANGLE_FAN) example")

  fill(150, 200, 255)
  stroke(51)
  strokeWeight(5)

  beginShape(TRIANGLE_FAN)
  vertex(200, 200)  -- center
  vertex(200, 50)   -- top
  vertex(320, 110)
  vertex(370, 250)
  vertex(280, 370)
  vertex(120, 370)
  vertex(30,  250)
  vertex(80,  110)
  vertex(200, 50)
  endShape()

  describe("octagon drawn with beginShape(TRIANGLE_FAN) from a center point, with stroke")
end

for TRIANGLE_FAN,I didn't want to use the implmented code for the other shapes (as beginShape() uses fan by default) to make the code readable

require("L5")

function setup()
  size(400, 400)
  windowTitle("beginShape(TRIANGLE_STRIP) example")

  fill(255, 180, 100)
  stroke(51)
  strokeWeight(2)

  beginShape(TRIANGLE_STRIP)
  vertex(50,  100)
  vertex(50,  300)
  vertex(150, 50)
  vertex(150, 300)
  vertex(250, 100)
  vertex(250, 300)
  vertex(350, 50)
  vertex(350, 300)
  endShape()

  describe("ribbon of triangles drawn with beginShape(TRIANGLE_STRIP), with stroke")
end

require("L5")

function setup() 
  size(400, 400)
  windowTitle("beginShape(TRIANGLES) example")
  
  fill(0)
  beginShape(TRIANGLES);

  -- Left triangle
  vertex(30, 75);
  vertex(40, 20);
  vertex(50, 75);

  -- Right triangle
  vertex(60, 20);
  vertex(70, 75);
  vertex(80, 20);

  endShape();

  describe("custom shape with beginShape(TRIANGLES) function, vertices and endShape()")
end

require("L5")

function setup()
  size(400, 400)
  windowTitle("texture() example")
  textureMode(NORMAL)

  cat = loadImage("examples/assets/cat.png")

  beginShape(TRIANGLES)
  texture(cat)
  vertex(50,  100, 0,    0.2)
  vertex(50,  300, 0,    1)
  vertex(150, 50,  0.33, 0)
  vertex(150, 300, 0.33, 1)
  vertex(250, 100, 0.67, 0.2)
  vertex(250, 300, 0.67, 1)
  vertex(350, 50,  1,    0)
  vertex(350, 300, 1,    1)
  endShape()

  describe("Wrapping a mesh texture around a custom shape with coordinates specified with u,v")
end

[lee2sman]

Excellent. I just tested all of your example code plus the original versions on Processing's beginShape() reference page.

Before merging, I thought worth considering one minor thing. What do you think we should return for incorrect passed kind parameters and un-implemented QUADS, QUAD_STRIP, TESS? Currently, there isn't error checking for incorrect kind parameter, but in p5.js it will return an error message: ReferenceError: BLARG_STRIP is not defined and in Processing BLARG_STRIP cannot be resolved to a variable. My hunch is that we should add in a kind parameter checking that says basically the same: BLARG_STRIP is not defined. I'm thinking with this added we can then merge.

Afterwards, we also need to update the vertex() reference page (to add info about u,v params) and the beginShape reference pages for info on kind and to add the examples.

[georgeibrahim1]

I wrote this code in L5.lua to handle the error you've mentioned :

function beginShape(_kind)

  if(_kind ~= nil and not L5_env.shapeKinds[_kind]) then -- Lu_env.shapeKinds = {[POINTS]=true, [TRIANGLES]=true, ...}
    error(_kind .. "is not defined" , 2)
  end 
  -- reset custom shape vertices table
  L5_env.vertices = {}
  L5_env.useTexture = false
  L5_env.kind = _kind
end

In Lua, any undefined global variable return nil, so if I called beginShape(NOT_DEFINED_SHAPE), It'll return nil and _kind ~= nil will not pass.

I think to override the returning nil behavior for the undefined variables or it adds overhead?

Do you suggest another way to handle this error?

[lee2sman]

Okay, good to brainstorm this. Unfortunately, I don't think that approach will work since there's no way to catch an undefined variable inside the function. Imagine someone passes an empty variable like BLAH, it will be undefined/nil, which will skip past the _kind ~= nil check in the conditional. Maybe this part should be removed then, unless you have any other solution.

[lee2sman]

To be clear, I'm saying that if _kind ~= nil and not L5_env.shapeKinds[_kind] then error(_kind .. " is not a valid shape kind", 2) end
works with
beginShape(), beginShape(POINTS) and even errors correctly on beginShape("BLAH") but not beginShape(BLAH).
unless I've missed something.

[georgeibrahim1]

I got to this approach

function beginShape(...)

  local n = select('#' , ...)
  local _kind = select(1, ...)

  if(n > 0 and _kind == nil) then
    error(_kind .. "is not defined" , 2)
  end

  if _kind ~= nil and not L5_env.shapeKinds[_kind] then -- to check if you pass strings
    error(_kind .. " is not defined", 2)
  end

  -- reset custom shape vertices table
  L5_env.vertices = {}
  L5_env.useTexture = false
  L5_env.kind = _kind
end

but we can't print the _kind as it's nil, so we can raise error like This shape kind is not defined.

I think we need to override the returning nil behavior for undefined variables to print BLAH is not defined, but it'll affect other places in the code.

[Nitish-bot]

I don't mean to leave this as a definitive review, I might be wrong here but I feel like there should also be a default value for _kind in order to keep consistent with the processing api, if the user calls beginShape without any arguments, it shouldn't throw an error.

[georgeibrahim1]

It wouldn't work too as both beginShape() and beginShape(NOT_DEFINED_KIND) return nil , If you used _kind = _kind or TRIANGLE_FAN (default value) , in both cases lead to _kind = TRIANGLE_FAN

[Nitish-bot]

No I think you can do it, you're almost there with your implementation. The select("#", ... ) function returns the number of arguments passed to the function. So you can just check like so:

function beginShape(...)
  local argc = select('#' , ...)
  if argc > 1 then
    error("beginShape([kind]) accepts at most one argument", 2)
  end

  local _kind = select(1, ...)

  if argc == 0 then
    _kind = TRIAGNLE_FAN -- or any other default
  elseif _kind == nil then
    error("beginShape(kind) received nil. This usually means you passed an undefined global, for example POINT instead of POINTS.", 2)
  elseif not L5_env.shapeKinds[_kind] then
    error("Invalid beginShape kind '" .. tostring(_kind) .. "'", 2)
  end

  -- reset custom shape vertices table
  L5_env.vertices = {}
  L5_env.useTexture = false
  L5_env.kind = _kind
end

[georgeibrahim1]

@lee2sman can you see this please :D ?

[lee2sman]

Cool idea @Nitish-bot That seems like a smart approach to solving that.

Only alteration I can think of:

if argc == 0 then
  _kind = nil  -- polygon mode, no specific primitive
end

No argument should fall through to polygon. Technically I think we can actually leave this out as it's a no-op, but there's no harm in leaving it in for verification.

And thanks @georgeibrahim1 for all your work.

Since the kind are mutually exclusive, elseif is the right choice for the conditionals, not if. The current code works correctly but is slightly inefficient. Once one if matches and returns, the rest should be ignored, and it also makes the intent clearer that these are distinct alternatives.

So like this:

if L5_env.kind == POINTS then
  -- ...
  return
elseif L5_env.kind == LINES then
  -- ...
  return
elseif L5_env.kind == TRIANGLES then
  -- ...
  return
elseif L5_env.kind == TRIANGLE_STRIP then
  -- ...
  return
elseif L5_env.kind == TRIANGLE_FAN then
  -- ...
  return
else
  -- polygon mode (nil or unrecognized)
end

Ok, one other thing I wanted to note that seems important. The Love2d wiki points out inefficient / "expensive" functions. The one that concerns us here in creating custom shapes is love.graphics.newMesh. (Although through that discovery I see the same issue with love.graphics.newQuad, which is used in the copy() and blendMode() functions elsewhere in the L5 library, so should also concern us, maybe in a new issue here on github).

You'll see this warning at the top of each of those wiki pages:

This function can be slow if it is called repeatedly, such as from love.update or love.draw. If you need to use a specific resource often, create it once and store it somewhere it can be reused!

Particularly if we are trying to make sure this library is efficient and can work on older hardware we should try to deal with that. One idea for an approach suggested could be to instead of creating a new mesh every draw call, for non-textured polygons we could create one global mesh upfront and just update it. Something like when we set the L5_env variables in define_env_globals() function:

-- inside the define_en_globals() function
L5_env.mesh = love.graphics.newMesh(
  {{"VertexPosition", "float", 2}},
  4096, "triangles", "dynamic"
)

What this does: creates an instance of a new mesh only once. Gives it default x,y vertices. Specifies the same max number of vertices that Processing allows (I think): 4096, uses the 'triangulate' polygon fill default, and specifies neither "static" NOR "stream" but "dynamic" which means the vertices COULD change sometimes (such as for an animation) but not every frame (stream).

Then we can replace non-textured new Mesh calls in endShape() function:

L5_env.mesh:setVertices(verts, 1, #verts)
L5_env.mesh:setDrawMode("triangles") 
L5_env.mesh:setDrawRange(1, #verts)
love.graphics.draw(L5_env.mesh)

But we'd still just leave the (less efficient) use of newMesh() in place for drawing textures on shapes since they use more complex arrangement of vertices with x,y,u,v.

Ok and bear with me as this looks like a monster.

I think endShape() would look like like this. But I'm putting it here now for a moment for consideration.

function endShape(_close)
  if #L5_env.vertices == 0 then return end

  -- helper to convert flat {x,y,x,y...} to {{x,y},{x,y}...}
  local function toVertTable(verts)
    if type(verts[1]) == "number" then
      local converted = {}
      for i = 1, #verts, 2 do
        converted[#converted+1] = {verts[i], verts[i+1]}
      end
      return converted
    end
    return verts
  end

  -- draw points
  if L5_env.kind == POINTS then
    local r, g, b, a = love.graphics.getColor()
    love.graphics.setColor(unpack(L5_env.stroke_color))
    for i = 1, #L5_env.vertices, 2 do
      love.graphics.points(L5_env.vertices[i], L5_env.vertices[i+1])
    end
    love.graphics.setColor(r, g, b, a)

  -- draw unconnected lines
  elseif L5_env.kind == LINES then
    local r, g, b, a = love.graphics.getColor()
    love.graphics.setColor(unpack(L5_env.stroke_color))
    for i = 1, #L5_env.vertices - 2, 4 do
      love.graphics.line(
        L5_env.vertices[i], L5_env.vertices[i+1],
        L5_env.vertices[i+2], L5_env.vertices[i+3]
      )
    end
    love.graphics.setColor(r, g, b, a)

  -- draw separated triangles
  elseif L5_env.kind == TRIANGLES then
    local verts = toVertTable(L5_env.vertices)
    if L5_env.useTexture and L5_env.currentTexture then
      local mesh = love.graphics.newMesh(verts, TRIANGLES)
      mesh:setTexture(L5_env.currentTexture)
      L5_env.currentTexture:setWrap(L5_env.textureWrap, L5_env.textureWrap)
      love.graphics.draw(mesh)
    else
      if L5_env.fill_mode == "fill" then
        L5_env.mesh:setVertices(verts, 1, #verts)
        L5_env.mesh:setDrawMode("triangles")
        L5_env.mesh:setDrawRange(1, #verts)
        love.graphics.draw(L5_env.mesh)
      end
      local r, g, b, a = love.graphics.getColor()
      love.graphics.setColor(unpack(L5_env.stroke_color))
      for i = 1, #verts, 3 do
        local v1, v2, v3 = verts[i], verts[i+1], verts[i+2]
        if v1 == nil or v2 == nil or v3 == nil then break end
        love.graphics.line(v1[1],v1[2], v2[1],v2[2])
        love.graphics.line(v2[1],v2[2], v3[1],v3[2])
        love.graphics.line(v3[1],v3[2], v1[1],v1[2])
      end
      love.graphics.setColor(r, g, b, a)
    end

  -- draw triangle strip
  elseif L5_env.kind == TRIANGLE_STRIP then
    local verts = toVertTable(L5_env.vertices)
    if L5_env.useTexture and L5_env.currentTexture then
      local mesh = love.graphics.newMesh(verts, TRIANGLE_STRIP)
      mesh:setTexture(L5_env.currentTexture)
      L5_env.currentTexture:setWrap(L5_env.textureWrap, L5_env.textureWrap)
      love.graphics.draw(mesh)
    else
      if L5_env.fill_mode == "fill" then
        L5_env.mesh:setVertices(verts, 1, #verts)
        L5_env.mesh:setDrawMode("strip")
        L5_env.mesh:setDrawRange(1, #verts)
        love.graphics.draw(L5_env.mesh)
      end
      local r, g, b, a = love.graphics.getColor()
      love.graphics.setColor(unpack(L5_env.stroke_color))
      for i = 1, #verts-2 do
        local v1, v2, v3 = verts[i], verts[i+1], verts[i+2]
        if v1 == nil or v2 == nil or v3 == nil then break end
        love.graphics.line(v1[1],v1[2], v2[1],v2[2])
        love.graphics.line(v2[1],v2[2], v3[1],v3[2])
        love.graphics.line(v3[1],v3[2], v1[1],v1[2])
      end
      love.graphics.setColor(r, g, b, a)
    end

  -- draw triangle fan
  elseif L5_env.kind == TRIANGLE_FAN then
    local verts = toVertTable(L5_env.vertices)
    if L5_env.useTexture and L5_env.currentTexture then
      local mesh = love.graphics.newMesh(verts, TRIANGLE_FAN)
      mesh:setTexture(L5_env.currentTexture)
      L5_env.currentTexture:setWrap(L5_env.textureWrap, L5_env.textureWrap)
      love.graphics.draw(mesh)
    else
      if L5_env.fill_mode == "fill" then
        L5_env.mesh:setVertices(verts, 1, #verts)
        L5_env.mesh:setDrawMode("fan")
        L5_env.mesh:setDrawRange(1, #verts)
        love.graphics.draw(L5_env.mesh)
      end
      local r, g, b, a = love.graphics.getColor()
      love.graphics.setColor(unpack(L5_env.stroke_color))
      for i = 2, #verts-1 do
        local v1, v2, v3 = verts[1], verts[i], verts[i+1]
        if v1 == nil or v2 == nil or v3 == nil then break end
        love.graphics.line(v1[1],v1[2], v2[1],v2[2])
        love.graphics.line(v2[1],v2[2], v3[1],v3[2])
        love.graphics.line(v3[1],v3[2], v1[1],v1[2])
      end
      love.graphics.setColor(r, g, b, a)
    end

  -- polygon fallback (kind == nil)
  else
    if L5_env.useTexture and L5_env.currentTexture then
      local mesh = love.graphics.newMesh(L5_env.vertices, "fan")
      mesh:setTexture(L5_env.currentTexture)
      L5_env.currentTexture:setWrap(L5_env.textureWrap, L5_env.textureWrap)
      love.graphics.draw(mesh)
    else
      if L5_env.fill_mode == "fill" then
        local ok, triangles = pcall(love.math.triangulate, L5_env.vertices)
        if ok then
          local meshVerts = {}
          for _, tri in ipairs(triangles) do
            for i = 1, 6, 2 do
              meshVerts[#meshVerts+1] = {tri[i], tri[i+1]}
            end
          end
          L5_env.mesh:setVertices(meshVerts, 1, #meshVerts)
          L5_env.mesh:setDrawMode("triangles")
          L5_env.mesh:setDrawRange(1, #meshVerts)
          love.graphics.draw(L5_env.mesh)
        else
          love.graphics.polygon("fill", L5_env.vertices)
        end
      end
      local r, g, b, a = love.graphics.getColor()
      love.graphics.setColor(unpack(L5_env.stroke_color))
      if _close == CLOSE then
        local verts = L5_env.vertices
        love.graphics.line(verts[1], verts[2], unpack(verts, 3, #verts))
        love.graphics.line(verts[#verts-1], verts[#verts], verts[1], verts[2])
      else
        love.graphics.line(L5_env.vertices)
      end
      love.graphics.setColor(r, g, b, a)
    end
  end
end

[georgeibrahim1]

@lee2sman you nailed it wow! , I think my code is a trash now, I'll add your suggestions + implementations for QUADS , QUAD_STRIP , TESS implementations to close this issue.