Pass extra args to World:queryBoundingBox callback

[slime73]

Original report by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

The callback to World:queryBoundingBox currently takes a single argument, containing a fixture intersected by the box. It can be hard to avoid creating a closure when writing the callback function. For example, consider this code:

#!lua

function Something:drawWorld (world)
    -- keep track of any bodies we've seen
    local bodies = {}
    
    -- this is our callback for the viewport query
    local function draw (fixture)
        local body = fixture:getBody()
        if not bodies[body] then
            local i = #bodies + 1
            bodies[i] = body
            bodies[body] = i
        end
        self:drawFixture(fixture)
        return true
    end
    
    -- draw what's on the screen
    world:queryBoundingBox(0, 0, 800, 600, draw)
        
    -- draw bodies connected to the fixtures (angle indicators, etc.)
    for i = 1, #bodies do
        self:drawBody(bodies[i])
    end
end

In this case the callback needs 2 upvalues from the closure, self and bodies. The closure would be unneeded if the API were changed so that any extra arguments passed to World:queryBoundingBox were passed along to the callback, after the fixture. Instead we could write a separate reusable function:

#!lua

-- this is our callback for the viewport query
local function draw (fixture, self, bodies)
    local body = fixture:getBody()
    if not bodies[body] then
        local i = #bodies + 1
        bodies[i] = body
        bodies[body] = i
    end
    self:drawFixture(fixture)
    return true
end
    
function Something:drawWorld (world)
    -- keep track of any bodies we've seen
    local bodies = {}
    
    -- draw what's on the screen
    world:queryBoundingBox(0, 0, 800, 600, draw, self, bodies)
        
    -- draw bodies connected to the fixtures (angle indicators, etc.)
    for i = 1, #bodies do
        self:drawBody(bodies[i])
    end
end

This should improve performance and code reuse.

[slime73]

Original comment by mkdxdx (Bitbucket: mkdxdx, GitHub: mkdxdx).

---

Isn't fixture's setUserData and getUserData is an option here? You can assign a reference to whatever entity you want that way by using this method so that physics can call whatever method it has later on from it's callbacks.

Like:

#!lua

-- somewhere in your object declaration code section
-- given that entity already has some methods
entity.fixture:setUserData(entity)

-- later on before querying AABB

local function querycallback(fixture)
 local entity = fixture:getUserData()
 entity:someEntityMethod()
 -- someEntityMethod might be a draw call for example
 -- and in that draw call entity will refer to anything in itself 
 -- that it relies to in order to render it
 return true
end

world:queryBoundingBox(0, 0, 800, 600, querycallback)

[slime73]

Original comment by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

I don't think it's really an option (at least, not a very good one). You might never have a handle to the fixture before querying the viewport, and even if you do, its userdata isn't really an appropriate place to store stuff like this. In the example I gave, the values being passed through to the callback really have nothing to do with the fixture. And we don't really want to store it anywhere, it's just transient data. In the case of library code, we'd be polluting the fixture's userdata, and it might not have any userdata associated with it, so we'd have to assign it a userdata that might get overwritten.

One thing that would be an option is passing a callable table as the callback, if queryBoundingBox will accept one (I haven't checked). As far as I know, closures can always be avoided with callable tables. It's ugly and inconvenient, though. Having passthrough varargs would be nicer.

[slime73]

Original comment by mkdxdx (Bitbucket: mkdxdx, GitHub: mkdxdx).

---

Well, tbh, i have (atleast, had) same issue with stencil functions (e.g. when i want to draw dynamically sized stencil that takes value from some object's field, and use that function as an object's method like usually is done through metatables and "selves"), so for that i had to create them at runtime and then call it a day. But knowing no better option i sometimes had a bit of a twitch while doing that.

However physics callbacks atleast have a way to communicate with what i want through get/set userdata (and for me it's perfectly fine and fit anywhere i want), and stencil functions don't except for creating a closure.

[slime73]

Original comment by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

I haven't used the stencil functions, but in general I think any function with a callback parameter could benefit from passing varargs through to the callback, if varargs aren't already used for something else.

Using a physics object's userdata for that purpose might be alright for some very specific cases, but in the general case it's more of a hack and in some cases (like the example I gave above) it's really not a viable option. That example came more or less directly from some library code that has no knowledge about specific fixtures before it finds them through queryBoundingBox, so it couldn't store stuff in their userdata beforehand even if that were desirable.

[slime73]

Original comment by Antonio Moder (Bitbucket: AntonioModer, GitHub: AntonioModer).

---

Hi airstruck. :)

Need observe strict physics API for awoid any confusion. I myself use "fixture:setUserData" or "body:setUserData" to draw bodies or fixtures, I dont have problems with it.

I use this:

#!lua

entity = fixture:getBody():getUserData()

[slime73]

Original comment by mkdxdx (Bitbucket: mkdxdx, GitHub: mkdxdx).

---

I guess the point of not using userdata is that it imposes unwanted coupling to an entity object structure. I.e. it assumes that the value that callback needs in its body does exist in "entity" table which makes callback code hard to move around into different systems. For example if callback code needs value of a key named "someproperty" (be it function or value), there is no guarantee that it exists in someone's code where this callback is used.

However solution actually fits if said callback code is used only by you and only in your projects (where reusability concern is, by default, not a high priority).

One suggestion though is to play with metamethods of what you pass as a callback to a function. In OPs code, for example

#!c++

world:queryBoundingBox(0, 0, 800, 600, draw)

"draw" can be a table like

#!c++

draw = {actuallyDraw = function() ... end, transientData = transientValue}
-- and then you modify draw's metamethods like __index and __call for it to behave the way you want
-- so that when table is called, it will redirect that call to actuallyDraw and if it is indexed
-- then return transientData's value

But i don't know if it is achieaveable and if it is, how exactly - i've never dug meta- parts of lua deeper than i where need to create prototype-instance stuff.

[slime73]

Original comment by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

Hi @AntonioModer, I think that approach can work when you already know about the objects you want to draw, but if you don't know about the objects before finding them with queryBoundingBox, you never have a chance to set their user data.

Also, in some cases (like library code, when they're "someone else's" objects) you might not want to mess with their user data even if you have that option.

Take a look here and I think you'll see what I mean.

[slime73]

Original comment by Antonio Moder (Bitbucket: AntonioModer, GitHub: AntonioModer).

---

airstruck:

• "but if you don't know about the objects before finding them with queryBoundingBox, you never have a chance to set their user data."

You can use World:getBodyList() (https://www.love2d.org/wiki/World:getBodyList)

Try use Lua to avoid your issue. Change physics API for "draw stuff" - it bad idea.

[slime73]

Original comment by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

@AntonioModer, I'm not sure how getBodyList would help in this case. The information I need in the callback is a list of bodies I've already encountered while drawing one particular frame. Storing that in the objects with setUserData seems like a massive kludge. What if they don't have userdata yet? What if they do already have userdata, but it's not a table? This is just transient data that's only relevant to a specific moment in time. I don't want to store it, I just want to pass it to the callback.

This would be a non-breaking change to the API, since the new varargs would be optional. I suspect that this is useful for more than just drawing stuff, even though I don't have another use case in mind right now. I also think that in general, passing varargs through to callbacks when possible is not a bad idea, for example in the case of the stencil functions @mkdxdx mentioned earlier.

Of course the issue can easily be avoided by living with the closure necessitated by the current situation. This was just a suggestion to improve that situation by removing (what I see as) the necessity for closures in some cases.

[slime73]

Original comment by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

@mkdxdx, just saw the edit to your earlier comment. Yes, callable tables are a workaround for eliminating closures, and there can be performance benefits, but it's usually so awkward that I'd personally rather just use closures. Of course, passthrough varargs should be cleaner and perform better than either closures or the callable tables workaround, so I thought it was worth suggesting.

[slime73]

Original comment by Bart van Strien (Bitbucket: bartbes, GitHub: bartbes).

---

I think this should be fairly easy, but only for those callbacks that are called immediately, like World:queryBoundingBox' callback. For "long-term" callbacks I imagine the overhead involved on love's side would be roughly on par with, or larger than creating a closure. If anyone is willing to find all callbacks that are called immediately, that'd be useful. And if anyone is willing to write a patch for us, that'd be even more useful ;).

[slime73]

Original comment by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

Hmm, I'll take a look. This and the stencil thing are the only API functions I'm aware of that take callbacks, do you know of any others offhand?

Here's another idea that just occurred to me that might be more "Lua-esque," but I'm not sure how performance would compare, or if the departure from Box2D is a good idea.

#!lua

for fixture in world:queryBoundingBox(0, 0, 800, 600) do
    if not draw(fixture, self, bodies) then break end
end

Lua iterators seem kind of slow in general, but I'm not sure how that would compare to callbacks. I'd guess the iterator might be faster than callbacks with closures, but slower than callbacks without them?

This could even be done in a non-breaking way (return an iterator if no callback function was passed, otherwise do the callback thing) so that the callback thing could be deprecated first and removed later.

Of course this would only make sense for immediate callbacks, not long-term ones.

[slime73]

Original comment by Alex Szpakowski (Bitbucket: slime73, GitHub: slime73).

---

This and the stencil thing are the only API functions I'm aware of that take callbacks, do you know of any others offhand?

I think this is all of them, maybe I'm missing one though:

World:setCallbacks

World:queryBoundingBox

World:rayCast

Fixture:rayCast

Canvas:renderTo

Channel:performAtomic

love.graphics.stencil

ImageData:mapPixel

One issue with supplying arguments to a callback as parameters to the function is that it makes LÖVE's APIs less flexible. I can't really have optional arguments if I do that (well, it's possible, but not good API design IMO), and I can't add new arguments at the end if I want to extend the API in the future without breaking compatibility, I'd have to put them before the function-callback-parameter-list.

[slime73]

Original comment by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

@slime73 good points about extensibility and optional args. I thought about this, but thought it wouldn't matter much in this particular case since there's really nothing more to add unless Box2D changes, and that kind of change would likely break things anyway. That doesn't apply for the non-physics stuff, though.

What do you think about the iterator idea?

[slime73]

Original comment by Bart van Strien (Bitbucket: bartbes, GitHub: bartbes).

---

The iterator idea definitely deserves a benchmark, it looks aesthetically pleasing and it leads to much less awkward function signatures.

[slime73]

Original comment by Tanner Rogalsky (Bitbucket: TannerRogalsky, GitHub: TannerRogalsky).

---

I looked into the iterator approach. The loop for querying which fixtures are in bounds is in Box2D and does not lend itself to extracting that control to Lua in the style of a Lua iterator. I do not think that a standard iterator or a coroutine-style iterator is possible without modifying Box2D.

For reference, here are the relevant places in the codebase.

• Box2D query function
• The lua callback entrypoint

The simple solution is to return an aggregate of all fixtures in the bounding box. The callback can be made optional as a way to maintain the short circuiting feature and main backwards compatibility.

#!lua

local fixtureList = world:queryBoundingBox(0, 0, 800, 600)
for i, fixture in ipairs(fixtureList) do
  if not draw(fixture, self, bodies) then break end
end

world:queryBoundingBox(0, 0, 800, 600, function(fixture) -- does not create or return the aggregate
  return draw(fixture, self, bodies)
end)

This keeps the implementation simple, simplifies the main use case (getting all fixtures in the bounding box) and still allows for the optimization of stoping the query midway through. What do you think, @airstruck ?

Edit: updated to make the callback version not return the fixture list.

[slime73]

Original comment by airstruck (Bitbucket: airstruck, GitHub: airstruck).

---

Ugh, I see what you mean. Seems like it could be somehow possible with longjmp voodoo or mutable lambda wizardry in a default callback, but I'm not sure. It would be nice if the implementation weren't 3 levels deep in a private member, then we could just extend world and make it a new query iterator thing.

[slime73]

As of d24ccde, World:queryBoundingBox, World:rayCast, Canvas:renderTo, and Channel:performAtomic all have extra user parameters passed through to their callback functions. love.graphics.stencil no longer exists so that's not a concern. ImageData:mapPixel doesn't have user arg passthrough but I think I want to address that by making ImageData:get/setPixel more usable instead.