From 6b7172bd40a0401d778973da3b242c6d0b95e578 Mon Sep 17 00:00:00 2001 From: raidho36 Date: Fri, 14 Apr 2017 05:53:36 +0300 Subject: [PATCH] Fix Issue #1273 Added Lua index registry clear routine to `destroy` method of classes that use it. If this is left to GC, the abandoned userdata would stay in memory for one full extra cycle. Prettified the code slightly. Clarified some comments. Refactored `Fixture` class `udata` name. Removed Reference class include from `Shape` (it doesn't use it). --- src/common/Reference.cpp | 2 +- src/common/runtime.cpp | 2 +- src/modules/physics/box2d/Body.cpp | 16 +++++++++--- src/modules/physics/box2d/Body.h | 2 +- src/modules/physics/box2d/Fixture.cpp | 37 +++++++++++++++++++-------- src/modules/physics/box2d/Fixture.h | 2 +- src/modules/physics/box2d/Joint.cpp | 23 ++++++++++++++--- src/modules/physics/box2d/Shape.h | 1 - src/modules/physics/box2d/World.cpp | 8 ++++++ 9 files changed, 71 insertions(+), 22 deletions(-) diff --git a/src/common/Reference.cpp b/src/common/Reference.cpp index 6fa34e077..6a50d7e98 100644 --- a/src/common/Reference.cpp +++ b/src/common/Reference.cpp @@ -45,7 +45,7 @@ Reference::~Reference() void Reference::ref(lua_State *L) { - unref(); // Just to be safe. + unref(); // Previously created reference needs to be cleared pinnedL = luax_getpinnedthread(L); luax_insist(L, LUA_REGISTRYINDEX, REFERENCE_TABLE_NAME); lua_insert(L, -2); // Move reference table behind value. diff --git a/src/common/runtime.cpp b/src/common/runtime.cpp index f6ecdebf3..2c505fef0 100644 --- a/src/common/runtime.cpp +++ b/src/common/runtime.cpp @@ -83,7 +83,7 @@ Reference *luax_refif(lua_State *L, int type) // Create a reference only if the test succeeds. if (lua_type(L, -1) == type) r = new Reference(L); - else // Pop the value even if it fails (but also if it succeeds). + else // Pop the value manually if it fails (done by Reference if it succeeds). lua_pop(L, 1); return r; diff --git a/src/modules/physics/box2d/Body.cpp b/src/modules/physics/box2d/Body.cpp index c9dad71c1..e3b256e16 100644 --- a/src/modules/physics/box2d/Body.cpp +++ b/src/modules/physics/box2d/Body.cpp @@ -67,8 +67,12 @@ Body::Body(b2Body *b) Body::~Body() { - if (udata != nullptr) + if (!udata) + return; + + if (udata->ref) delete udata->ref; + delete udata; } @@ -521,6 +525,10 @@ void Body::destroy() Memoizer::remove(body); body = NULL; + // Remove userdata reference to avoid it sticking around after GC + if (udata && udata->ref) + udata->ref->unref(); + // Box2D body destroyed. Release its reference to the love Body. this->release(); } @@ -535,8 +543,10 @@ int Body::setUserData(lua_State *L) body->SetUserData((void *) udata); } - delete udata->ref; - udata->ref = new Reference(L); + if(!udata->ref) + udata->ref = new Reference(); + + udata->ref->ref(L); return 0; } diff --git a/src/modules/physics/box2d/Body.h b/src/modules/physics/box2d/Body.h index 8803eff11..567c53974 100644 --- a/src/modules/physics/box2d/Body.h +++ b/src/modules/physics/box2d/Body.h @@ -69,7 +69,7 @@ public: friend class Shape; friend class Fixture; - // The Box2D body. (Should not be public?) + // Public because joints et al ask for b2body b2Body *body; /** diff --git a/src/modules/physics/box2d/Fixture.cpp b/src/modules/physics/box2d/Fixture.cpp index 7f0b31e28..89668de82 100644 --- a/src/modules/physics/box2d/Fixture.cpp +++ b/src/modules/physics/box2d/Fixture.cpp @@ -41,11 +41,11 @@ Fixture::Fixture(Body *body, Shape *shape, float density) : body(body) , fixture(nullptr) { - data = new fixtureudata(); - data->ref = nullptr; + udata = new fixtureudata(); + udata->ref = nullptr; b2FixtureDef def; def.shape = shape->shape; - def.userData = (void *)data; + def.userData = (void *)udata; def.density = density; fixture = body->body->CreateFixture(&def); this->retain(); @@ -55,7 +55,7 @@ Fixture::Fixture(Body *body, Shape *shape, float density) Fixture::Fixture(b2Fixture *f) : fixture(f) { - data = (fixtureudata *)f->GetUserData(); + udata = (fixtureudata *)f->GetUserData(); body = (Body *)Memoizer::find(f->GetBody()); if (!body) body = new Body(f->GetBody()); @@ -65,10 +65,13 @@ Fixture::Fixture(b2Fixture *f) Fixture::~Fixture() { - if (data != nullptr) - delete data->ref; + if (!udata) + return; - delete data; + if (udata->ref) + delete udata->ref; + + delete udata; } Shape::Type Fixture::getType() const @@ -239,16 +242,24 @@ int Fixture::setUserData(lua_State *L) { love::luax_assert_argc(L, 1, 1); - delete data->ref; - data->ref = new Reference(L); + if (udata == nullptr) + { + udata = new fixtureudata(); + fixture->SetUserData((void *) udata); + } + + if(!udata->ref) + udata->ref = new Reference(); + + udata->ref->ref(L); return 0; } int Fixture::getUserData(lua_State *L) { - if (data->ref != nullptr) - data->ref->push(L); + if (udata->ref != nullptr) + udata->ref->push(L); else lua_pushnil(L); @@ -321,6 +332,10 @@ void Fixture::destroy(bool implicit) Memoizer::remove(fixture); fixture = nullptr; + // Remove userdata reference to avoid it sticking around after GC + if (udata && udata->ref) + udata->ref->unref(); + // Box2D fixture destroyed. Release its reference to the love Fixture. this->release(); } diff --git a/src/modules/physics/box2d/Fixture.h b/src/modules/physics/box2d/Fixture.h index 278befa00..fa001901f 100644 --- a/src/modules/physics/box2d/Fixture.h +++ b/src/modules/physics/box2d/Fixture.h @@ -211,7 +211,7 @@ public: protected: Body *body; - fixtureudata *data; + fixtureudata *udata; b2Fixture *fixture; }; diff --git a/src/modules/physics/box2d/Joint.cpp b/src/modules/physics/box2d/Joint.cpp index b5e5fdcff..6a3727bb9 100644 --- a/src/modules/physics/box2d/Joint.cpp +++ b/src/modules/physics/box2d/Joint.cpp @@ -61,8 +61,12 @@ Joint::Joint(Body *body1, Body *body2) Joint::~Joint() { - if (udata != nullptr) + if (!udata) + return; + + if (udata->ref) delete udata->ref; + delete udata; } @@ -175,6 +179,11 @@ void Joint::destroyJoint(bool implicit) world->world->DestroyJoint(joint); Memoizer::remove(joint); joint = NULL; + + // Remove userdata reference to avoid it sticking around after GC + if (udata && udata->ref) + udata->ref->unref(); + // Release the reference of the Box2D joint. this->release(); } @@ -193,8 +202,16 @@ int Joint::setUserData(lua_State *L) { love::luax_assert_argc(L, 1, 1); - delete udata->ref; - udata->ref = new Reference(L); + if (udata == nullptr) + { + udata = new jointudata(); + joint->SetUserData((void *) udata); + } + + if(!udata->ref) + udata->ref = new Reference(); + + udata->ref->ref(L); return 0; } diff --git a/src/modules/physics/box2d/Shape.h b/src/modules/physics/box2d/Shape.h index c5a3db249..5a187cbd0 100644 --- a/src/modules/physics/box2d/Shape.h +++ b/src/modules/physics/box2d/Shape.h @@ -24,7 +24,6 @@ // LOVE #include "physics/Shape.h" #include "physics/box2d/Body.h" -#include "common/Reference.h" // Box2D #include diff --git a/src/modules/physics/box2d/World.cpp b/src/modules/physics/box2d/World.cpp index 92ff802d4..8993cf39e 100644 --- a/src/modules/physics/box2d/World.cpp +++ b/src/modules/physics/box2d/World.cpp @@ -577,6 +577,14 @@ void World::destroy() world->DestroyBody(groundBody); Memoizer::remove(world); + + // Remove userdata reference to avoid it sticking around after GC + if (begin.ref) begin.ref->unref(); + if (end.ref) end.ref->unref(); + if (presolve.ref) presolve.ref->unref(); + if (postsolve.ref) postsolve.ref->unref(); + if (filter.ref) filter.ref->unref(); + delete world; world = nullptr; }