Pass extra args to World:queryBoundingBox callback

Issue #1189 open
airstruck
created an issue

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:

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:

-- 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.

Comments (18)

  1. mkdx

    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:

    -- 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)
    
  2. airstruck reporter

    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.

  3. mkdx

    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.

  4. airstruck reporter

    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.

  5. Antonio Moder

    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:

    entity = fixture:getBody():getUserData()
    
  6. mkdx

    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

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

    "draw" can be a table like

    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.

  7. airstruck reporter

    Hi @Antonio Moder, 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.

  8. airstruck reporter

    @Antonio Moder, 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 @mkdx 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.

  9. airstruck reporter

    @mkdx, 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.

  10. Bart van Strien
    • changed status to open

    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 ;).

  11. airstruck reporter

    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.

    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.

  12. Alex Szpakowski

    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.

  13. airstruck reporter

    @Alex Szpakowski 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?

  14. 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.

    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.

    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.

  15. airstruck reporter

    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.

  16. Log in to comment