From eba262b7894970217af66f30b23798b043270fd7 Mon Sep 17 00:00:00 2001 From: Anonymous Maarten Date: Wed, 13 Sep 2017 18:14:34 +0200 Subject: [PATCH] rwengine: make Activity a std::unique_ptr Should fix this memory leak: ==16721== 120 bytes in 5 blocks are definitely lost in loss record 187 of 264 ==16721== at 0x4C2F1CA: operator new(unsigned long) (vg_replace_malloc.c:334) ==16721== by 0x8EBA26: DefaultAIController::update(float) (DefaultAIController.cpp:63) ==16721== by 0x82ED5A: CharacterObject::tick(float) (CharacterObject.cpp:240) ==16721== by 0x763630: RWGame::tick(float) (RWGame.cpp:506) ==16721== by 0x763128: RWGame::run() (RWGame.cpp:420) ==16721== by 0x750394: main (main.cpp:15) --- rwengine/src/ai/CharacterController.cpp | 23 +++++++----------- rwengine/src/ai/CharacterController.hpp | 24 ++++++++++++++----- rwengine/src/ai/DefaultAIController.cpp | 12 ++++++---- rwengine/src/ai/PlayerController.cpp | 6 ++--- rwengine/src/objects/CharacterObject.cpp | 3 ++- .../src/script/modules/GTA3ModuleImpl.inl | 21 ++++++++++------ tests/test_character.cpp | 13 ++++++---- 7 files changed, 60 insertions(+), 42 deletions(-) diff --git a/rwengine/src/ai/CharacterController.cpp b/rwengine/src/ai/CharacterController.cpp index 223c70bc..9f7634da 100644 --- a/rwengine/src/ai/CharacterController.cpp +++ b/rwengine/src/ai/CharacterController.cpp @@ -29,9 +29,8 @@ bool CharacterController::updateActivity() { return false; } -void CharacterController::setActivity(CharacterController::Activity *activity) { - if (_currentActivity) delete _currentActivity; - _currentActivity = activity; +void CharacterController::setActivity(std::unique_ptr activity) { + _currentActivity.swap(activity); } void CharacterController::skipActivity() { @@ -43,14 +42,12 @@ void CharacterController::skipActivity() { setActivity(nullptr); } -void CharacterController::setNextActivity( - CharacterController::Activity *activity) { +void CharacterController::setNextActivity(std::unique_ptr activity) { if (_currentActivity == nullptr) { - setActivity(activity); + setActivity(std::move(activity)); _nextActivity = nullptr; } else { - if (_nextActivity) delete _nextActivity; - _nextActivity = activity; + _nextActivity.swap(activity); } } @@ -97,13 +94,9 @@ void CharacterController::update(float dt) { if (updateActivity()) { character->activityFinished(); - if (_currentActivity) { - delete _currentActivity; - _currentActivity = nullptr; - } + _currentActivity = nullptr; if (_nextActivity) { - setActivity(_nextActivity); - _nextActivity = nullptr; + setActivity(std::move(_nextActivity)); } } } @@ -282,7 +275,7 @@ bool Activities::EnterVehicle::update(CharacterObject *character, // out. character->playCycle(cycle_pullout); currentOccupant->controller->setNextActivity( - new Activities::ExitVehicle(true)); + std::make_unique(true)); } else { character->playCycle(cycle_enter); character->enterVehicle(vehicle, seat); diff --git a/rwengine/src/ai/CharacterController.hpp b/rwengine/src/ai/CharacterController.hpp index fd783c8c..a9506832 100644 --- a/rwengine/src/ai/CharacterController.hpp +++ b/rwengine/src/ai/CharacterController.hpp @@ -3,6 +3,8 @@ #define _CHARACTERCONTROLLER_HPP_ #include #include + +#include #include struct AIGraphNode; @@ -61,11 +63,11 @@ protected: */ CharacterObject* character; - Activity* _currentActivity; - Activity* _nextActivity; + std::unique_ptr _currentActivity; + std::unique_ptr _nextActivity; bool updateActivity(); - void setActivity(Activity* activity); + void setActivity(std::unique_ptr activity); float m_closeDoorTimer; @@ -80,12 +82,22 @@ public: virtual ~CharacterController() { } + /** + * Get the current Activity. + * Callers may not store the returned pointer. + * @return Activity pointer. + */ Activity* getCurrentActivity() const { - return _currentActivity; + return _currentActivity.get(); } + /** + * Get the next Activity + * Callers may not store the returned pointer. + * @return Activity pointer. + */ Activity* getNextActivity() const { - return _nextActivity; + return _nextActivity.get(); } /** @@ -98,7 +110,7 @@ public: * @param activity * @param position */ - void setNextActivity(Activity* activity); + void setNextActivity(std::unique_ptr activity); /** * @brief IsCurrentActivity diff --git a/rwengine/src/ai/DefaultAIController.cpp b/rwengine/src/ai/DefaultAIController.cpp index 70134532..aae739dd 100644 --- a/rwengine/src/ai/DefaultAIController.cpp +++ b/rwengine/src/ai/DefaultAIController.cpp @@ -25,12 +25,12 @@ void DefaultAIController::update(float dt) { if (leader->getCurrentVehicle() != getCharacter()->getCurrentVehicle()) { skipActivity(); - setNextActivity(new Activities::ExitVehicle); + setNextActivity(std::make_unique()); } // else we're already in the right spot. } else { if (leader->getCurrentVehicle()) { - setNextActivity(new Activities::EnterVehicle( + setNextActivity(std::make_unique( leader->getCurrentVehicle(), 1)); } else { glm::vec3 dir = @@ -42,7 +42,7 @@ void DefaultAIController::update(float dt) { leader->getPosition() + (glm::normalize(-dir) * followRadius * 0.7f); skipActivity(); - setNextActivity(new Activities::GoTo(gotoPos)); + setNextActivity(std::make_unique(gotoPos)); } } } @@ -60,9 +60,11 @@ void DefaultAIController::update(float dt) { std::uniform_int_distribution<> d( 0, lastTarget->connections.size() - 1); targetNode = lastTarget->connections.at(d(re)); - setNextActivity(new Activities::GoTo(targetNode->position)); + setNextActivity(std::make_unique( + targetNode->position)); } else if (getCurrentActivity() == nullptr) { - setNextActivity(new Activities::GoTo(targetNode->position)); + setNextActivity(std::make_unique( + targetNode->position)); } } else { // We need to pick an initial node diff --git a/rwengine/src/ai/PlayerController.cpp b/rwengine/src/ai/PlayerController.cpp index 27344df7..b56ff423 100644 --- a/rwengine/src/ai/PlayerController.cpp +++ b/rwengine/src/ai/PlayerController.cpp @@ -33,7 +33,7 @@ void PlayerController::updateMovementDirection(const glm::vec3& dir, void PlayerController::exitVehicle() { if (character->getCurrentVehicle()) { - setNextActivity(new Activities::ExitVehicle()); + setNextActivity(std::make_unique()); } } @@ -54,7 +54,7 @@ void PlayerController::enterNearestVehicle() { } if (nearest) { - setNextActivity(new Activities::EnterVehicle(nearest, 0)); + setNextActivity(std::make_unique(nearest, 0)); } } } @@ -69,6 +69,6 @@ glm::vec3 PlayerController::getTargetPosition() { void PlayerController::jump() { if (!character->isInWater()) { - setNextActivity(new Activities::Jump()); + setNextActivity(std::make_unique()); } } diff --git a/rwengine/src/objects/CharacterObject.cpp b/rwengine/src/objects/CharacterObject.cpp index 15fd9d15..1b8d98a3 100644 --- a/rwengine/src/objects/CharacterObject.cpp +++ b/rwengine/src/objects/CharacterObject.cpp @@ -624,7 +624,8 @@ void CharacterObject::useItem(bool active, bool primary) { if (primary) { if (!currentState.primaryActive && active) { // If we've just started, activate - controller->setNextActivity(new Activities::UseItem(item)); + controller->setNextActivity( + std::make_unique(item)); } else if (currentState.primaryActive && !active) { // UseItem will cancel itself upon !primaryActive } diff --git a/rwengine/src/script/modules/GTA3ModuleImpl.inl b/rwengine/src/script/modules/GTA3ModuleImpl.inl index f3bf8164..508e903c 100644 --- a/rwengine/src/script/modules/GTA3ModuleImpl.inl +++ b/rwengine/src/script/modules/GTA3ModuleImpl.inl @@ -5365,7 +5365,8 @@ void opcode_01d3(const ScriptArguments& args, const ScriptCharacter character, c RW_UNUSED(vehicle); RW_UNUSED(args); character->controller->skipActivity(); - character->controller->setNextActivity(new Activities::ExitVehicle); + character->controller->setNextActivity( + std::make_unique());(new Activities::ExitVehicle); } /** @@ -5378,7 +5379,9 @@ void opcode_01d3(const ScriptArguments& args, const ScriptCharacter character, c void opcode_01d4(const ScriptArguments& args, const ScriptCharacter character, const ScriptVehicle vehicle) { RW_UNUSED(args); character->controller->skipActivity(); - character->controller->setNextActivity(new Activities::EnterVehicle(vehicle,Activities::EnterVehicle::ANY_SEAT)); + character->controller->setNextActivity( + std::make_unique( + vehicle,Activities::EnterVehicle::ANY_SEAT)); } /** @@ -5390,7 +5393,8 @@ void opcode_01d4(const ScriptArguments& args, const ScriptCharacter character, c */ void opcode_01d5(const ScriptArguments& args, const ScriptCharacter character, const ScriptVehicle vehicle) { RW_UNUSED(args); - character->controller->setNextActivity(new Activities::EnterVehicle(vehicle)); + character->controller->setNextActivity( + std::make_unique(vehicle)); } /** @@ -6170,10 +6174,12 @@ void opcode_0211(const ScriptArguments& args, const ScriptCharacter character, S if( character->getCurrentVehicle() ) { // Since we just cleared the Activities, this will become current immediatley. - character->controller->setNextActivity(new Activities::ExitVehicle); + character->controller->setNextActivity( + std::make_unique()); } - - character->controller->setNextActivity(new Activities::GoTo(target)); + + character->controller->setNextActivity( + std::make_unique(target)); } /** @@ -6625,7 +6631,8 @@ void opcode_0237(const ScriptArguments& args, const ScriptGang gangID, const Scr */ void opcode_0239(const ScriptArguments& args, const ScriptCharacter character, ScriptVec2 coord) { auto target = script::getGround(args, glm::vec3(coord, -100.f)); - character->controller->setNextActivity(new Activities::GoTo(target, true)); + character->controller->setNextActivity( + std::make_unique(target, true)); } /** diff --git a/tests/test_character.cpp b/tests/test_character.cpp index c36cb2f4..829b4c80 100644 --- a/tests/test_character.cpp +++ b/tests/test_character.cpp @@ -22,7 +22,7 @@ BOOST_AUTO_TEST_CASE(test_create) { // Check that Idle activities are instantly displaced. controller->setNextActivity( - new Activities::GoTo(glm::vec3{1000.f, 0.f, 0.f})); + std::make_unique(glm::vec3{1000.f, 0.f, 0.f})); BOOST_CHECK_EQUAL(controller->getCurrentActivity()->name(), "GoTo"); BOOST_CHECK_EQUAL(controller->getNextActivity(), nullptr); @@ -42,7 +42,7 @@ BOOST_AUTO_TEST_CASE(test_activities) { BOOST_REQUIRE(controller != nullptr); controller->setNextActivity( - new Activities::GoTo(glm::vec3{10.f, 10.f, 0.f})); + std::make_unique(glm::vec3{10.f, 10.f, 0.f})); BOOST_CHECK_EQUAL(controller->getCurrentActivity()->name(), "GoTo"); @@ -69,7 +69,8 @@ BOOST_AUTO_TEST_CASE(test_activities) { auto controller = character->controller; BOOST_REQUIRE(controller != nullptr); - controller->setNextActivity(new Activities::EnterVehicle(vehicle, 0)); + controller->setNextActivity( + std::make_unique(vehicle, 0)); for (float t = 0.f; t < 0.5f; t += (1.f / 60.f)) { character->tick(1.f / 60.f); @@ -85,7 +86,8 @@ BOOST_AUTO_TEST_CASE(test_activities) { BOOST_CHECK_EQUAL(vehicle, character->getCurrentVehicle()); - controller->setNextActivity(new Activities::ExitVehicle()); + controller->setNextActivity( + std::make_unique()); for (float t = 0.f; t < 9.0f; t += (1.f / 60.f)) { character->tick(1.f / 60.f); @@ -95,7 +97,8 @@ BOOST_AUTO_TEST_CASE(test_activities) { BOOST_CHECK_EQUAL(nullptr, character->getCurrentVehicle()); character->setPosition(glm::vec3(5.f, 0.f, 0.f)); - controller->setNextActivity(new Activities::EnterVehicle(vehicle, 0)); + controller->setNextActivity( + std::make_unique(vehicle, 0)); for (float t = 0.f; t < 0.5f; t += (1.f / 60.f)) { character->tick(1.f / 60.f);