From 711dd00cdec4b4ce1479364e031c6b16222298a6 Mon Sep 17 00:00:00 2001 From: Vinicius Sa Date: Wed, 14 Oct 2020 16:56:48 -0300 Subject: [PATCH] Close #13000: Refactor ObjectFactory to use unique_ptr Employs the smart pointer unique_ptr for safer memory management. Classes involved: - ObjectRepository - ObjectManager --- .../windows/EditorInventionsList.cpp | 4 +- .../windows/EditorObjectSelection.cpp | 23 ++-- src/openrct2/object/ImageTable.cpp | 3 +- src/openrct2/object/ObjectFactory.cpp | 93 +++++++-------- src/openrct2/object/ObjectFactory.h | 11 +- src/openrct2/object/ObjectManager.cpp | 109 +++++++++--------- src/openrct2/object/ObjectManager.h | 4 +- src/openrct2/object/ObjectRepository.cpp | 29 +---- src/openrct2/object/ObjectRepository.h | 7 +- src/openrct2/rct1/S4Importer.cpp | 3 +- src/openrct2/rct2/T6Importer.cpp | 6 +- 11 files changed, 129 insertions(+), 163 deletions(-) diff --git a/src/openrct2-ui/windows/EditorInventionsList.cpp b/src/openrct2-ui/windows/EditorInventionsList.cpp index 1ee3eec921..5aa8aea9de 100644 --- a/src/openrct2-ui/windows/EditorInventionsList.cpp +++ b/src/openrct2-ui/windows/EditorInventionsList.cpp @@ -553,7 +553,7 @@ static void window_editor_inventions_list_paint(rct_window* w, rct_drawpixelinfo // Draw preview widget = &w->widgets[WIDX_PREVIEW]; - void* object = object_manager_get_loaded_object(entry); + auto* object = object_manager_get_loaded_object(entry); if (object != nullptr) { rct_drawpixelinfo clipDPI; @@ -562,7 +562,7 @@ static void window_editor_inventions_list_paint(rct_window* w, rct_drawpixelinfo int32_t height = widget->height() - 1; if (clip_drawpixelinfo(&clipDPI, dpi, screenPos, width, height)) { - object_draw_preview(object, &clipDPI, width, height); + object->DrawPreview(&clipDPI, width, height); } } diff --git a/src/openrct2-ui/windows/EditorObjectSelection.cpp b/src/openrct2-ui/windows/EditorObjectSelection.cpp index 7ea9d89e06..be877476f8 100644 --- a/src/openrct2-ui/windows/EditorObjectSelection.cpp +++ b/src/openrct2-ui/windows/EditorObjectSelection.cpp @@ -221,7 +221,7 @@ static bool filter_source(const ObjectRepositoryItem* item); static bool filter_chunks(const ObjectRepositoryItem* item); static void filter_update_counts(); -static std::string object_get_description(const void* object); +static std::string object_get_description(const Object* object); static int32_t get_selected_object_type(rct_window* w); enum @@ -260,7 +260,7 @@ using sortFunc_t = bool (*)(const list_item&, const list_item&); static std::vector _listItems; static int32_t _listSortType = RIDE_SORT_TYPE; static bool _listSortDescending = false; -static void* _loadedObject = nullptr; +static std::unique_ptr _loadedObject; static void visible_list_dispose() { @@ -414,8 +414,8 @@ static void window_editor_object_selection_close(rct_window* w) editor_load_selected_objects(); editor_object_flags_free(); - object_delete(_loadedObject); - _loadedObject = nullptr; + if (_loadedObject != nullptr) + _loadedObject->Unload(); if (gScreenFlags & SCREEN_FLAGS_EDITOR) { @@ -744,8 +744,8 @@ static void window_editor_object_selection_scroll_mouseover( { w->selected_list_item = selectedObject; - object_delete(_loadedObject); - _loadedObject = nullptr; + if (_loadedObject != nullptr) + _loadedObject->Unload(); if (selectedObject == -1) { @@ -1045,7 +1045,7 @@ static void window_editor_object_selection_paint(rct_window* w, rct_drawpixelinf int32_t height = widget->height() - 1; if (clip_drawpixelinfo(&clipDPI, dpi, screenPos, width, height)) { - object_draw_preview(_loadedObject, &clipDPI, width, height); + _loadedObject->DrawPreview(&clipDPI, width, height); } } @@ -1060,7 +1060,7 @@ static void window_editor_object_selection_paint(rct_window* w, rct_drawpixelinf } // Draw description of object - auto description = object_get_description(_loadedObject); + auto description = object_get_description(_loadedObject.get()); if (!description.empty()) { auto ft = Formatter(); @@ -1524,14 +1524,13 @@ static rct_string_id get_ride_type_string_id(const ObjectRepositoryItem* item) return result; } -static std::string object_get_description(const void* object) +static std::string object_get_description(const Object* object) { - const Object* baseObject = static_cast(object); - switch (baseObject->GetObjectType()) + switch (object->GetObjectType()) { case OBJECT_TYPE_RIDE: { - const RideObject* rideObject = static_cast(baseObject); + const RideObject* rideObject = static_cast(object); return rideObject->GetDescription(); } default: diff --git a/src/openrct2/object/ImageTable.cpp b/src/openrct2/object/ImageTable.cpp index f7ee20b276..d73d1b28d4 100644 --- a/src/openrct2/object/ImageTable.cpp +++ b/src/openrct2/object/ImageTable.cpp @@ -193,7 +193,7 @@ std::vector> ImageTable::LoadObjectIm auto obj = ObjectFactory::CreateObjectFromLegacyFile(context->GetObjectRepository(), objectPath.c_str()); if (obj != nullptr) { - auto& imgTable = static_cast(obj)->GetImageTable(); + auto& imgTable = static_cast(obj.get())->GetImageTable(); auto numImages = static_cast(imgTable.GetCount()); auto images = imgTable.GetImages(); size_t placeHoldersAdded = 0; @@ -210,7 +210,6 @@ std::vector> ImageTable::LoadObjectIm placeHoldersAdded++; } } - delete obj; // Log place holder information if (placeHoldersAdded > 0) diff --git a/src/openrct2/object/ObjectFactory.cpp b/src/openrct2/object/ObjectFactory.cpp index 264c98404f..220680c239 100644 --- a/src/openrct2/object/ObjectFactory.cpp +++ b/src/openrct2/object/ObjectFactory.cpp @@ -164,7 +164,7 @@ namespace ObjectFactory * @param jRoot Must be JSON node of type object * @note jRoot is deliberately left non-const: json_t behaviour changes when const */ - static Object* CreateObjectFromJson( + static std::unique_ptr CreateObjectFromJson( IObjectRepository& objectRepository, json_t& jRoot, const IFileDataRetriever* fileRetriever); static uint8_t ParseSourceGame(const std::string& s) @@ -183,11 +183,11 @@ namespace ObjectFactory return (result != LookupTable.end()) ? result->second : OBJECT_SOURCE_CUSTOM; } - static void ReadObjectLegacy(Object* object, IReadObjectContext* context, OpenRCT2::IStream* stream) + static void ReadObjectLegacy(Object& object, IReadObjectContext* context, OpenRCT2::IStream* stream) { try { - object->ReadLegacy(context, stream); + object.ReadLegacy(context, stream); } catch (const IOException&) { @@ -200,11 +200,11 @@ namespace ObjectFactory } } - Object* CreateObjectFromLegacyFile(IObjectRepository& objectRepository, const utf8* path) + std::unique_ptr CreateObjectFromLegacyFile(IObjectRepository& objectRepository, const utf8* path) { log_verbose("CreateObjectFromLegacyFile(..., \"%s\")", path); - Object* result = nullptr; + std::unique_ptr result; try { auto fs = OpenRCT2::FileStream(path, OpenRCT2::FILE_MODE_OPEN); @@ -225,7 +225,7 @@ namespace ObjectFactory auto chunkStream = OpenRCT2::MemoryStream(chunk->GetData(), chunk->GetLength()); auto readContext = ReadObjectContext(objectRepository, objectName, !gOpenRCT2NoGraphics, nullptr); - ReadObjectLegacy(result, &readContext, &chunkStream); + ReadObjectLegacy(*result, &readContext, &chunkStream); if (readContext.WasError()) { throw std::runtime_error("Object has errors"); @@ -236,19 +236,17 @@ namespace ObjectFactory catch (const std::exception& e) { log_error("Error: %s when processing object %s", e.what(), path); - delete result; - result = nullptr; } return result; } - Object* CreateObjectFromLegacyData( + std::unique_ptr CreateObjectFromLegacyData( IObjectRepository& objectRepository, const rct_object_entry* entry, const void* data, size_t dataSize) { Guard::ArgumentNotNull(entry, GUARD_LINE); Guard::ArgumentNotNull(data, GUARD_LINE); - Object* result = CreateObject(*entry); + auto result = CreateObject(*entry); if (result != nullptr) { utf8 objectName[DAT_NAME_LENGTH + 1]; @@ -256,12 +254,11 @@ namespace ObjectFactory auto readContext = ReadObjectContext(objectRepository, objectName, !gOpenRCT2NoGraphics, nullptr); auto chunkStream = OpenRCT2::MemoryStream(data, dataSize); - ReadObjectLegacy(result, &readContext, &chunkStream); + ReadObjectLegacy(*result, &readContext, &chunkStream); if (readContext.WasError()) { - delete result; - result = nullptr; + log_error("Error when processing object."); } else { @@ -271,52 +268,51 @@ namespace ObjectFactory return result; } - Object* CreateObject(const rct_object_entry& entry) + std::unique_ptr CreateObject(const rct_object_entry& entry) { - Object* result; + std::unique_ptr result; switch (entry.GetType()) { case OBJECT_TYPE_RIDE: - result = new RideObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_SMALL_SCENERY: - result = new SmallSceneryObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_LARGE_SCENERY: - result = new LargeSceneryObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_WALLS: - result = new WallObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_BANNERS: - result = new BannerObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_PATHS: - result = new FootpathObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_PATH_BITS: - result = new FootpathItemObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_SCENERY_GROUP: - result = new SceneryGroupObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_PARK_ENTRANCE: - result = new EntranceObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_WATER: - result = new WaterObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_SCENARIO_TEXT: - result = nullptr; break; case OBJECT_TYPE_TERRAIN_SURFACE: - result = new TerrainSurfaceObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_TERRAIN_EDGE: - result = new TerrainEdgeObject(entry); + result = std::make_unique(entry); break; case OBJECT_TYPE_STATION: - result = new StationObject(entry); + result = std::make_unique(entry); break; default: throw std::runtime_error("Invalid object type"); @@ -355,9 +351,8 @@ namespace ObjectFactory return 0xFF; } - Object* CreateObjectFromZipFile(IObjectRepository& objectRepository, const std::string_view& path) + std::unique_ptr CreateObjectFromZipFile(IObjectRepository& objectRepository, const std::string_view& path) { - Object* result = nullptr; try { auto archive = Zip::Open(path, ZIP_ACCESS::READ); @@ -369,46 +364,38 @@ namespace ObjectFactory json_t jRoot = Json::FromVector(jsonBytes); - Object* obj = nullptr; - if (jRoot.is_object()) { auto fileDataRetriever = ZipDataRetriever(*archive); - obj = CreateObjectFromJson(objectRepository, jRoot, &fileDataRetriever); + return CreateObjectFromJson(objectRepository, jRoot, &fileDataRetriever); } - - return obj; } catch (const std::exception& e) { Console::Error::WriteLine("Unable to open or read '%s': %s", path.data(), e.what()); - - delete result; - result = nullptr; } - return result; + return nullptr; } - Object* CreateObjectFromJsonFile(IObjectRepository& objectRepository, const std::string& path) + std::unique_ptr CreateObjectFromJsonFile(IObjectRepository& objectRepository, const std::string& path) { log_verbose("CreateObjectFromJsonFile(\"%s\")", path.c_str()); - Object* result = nullptr; try { json_t jRoot = Json::ReadFromFile(path.c_str()); auto fileDataRetriever = FileSystemDataRetriever(Path::GetDirectory(path)); - result = CreateObjectFromJson(objectRepository, jRoot, &fileDataRetriever); + return CreateObjectFromJson(objectRepository, jRoot, &fileDataRetriever); } catch (const std::runtime_error& err) { Console::Error::WriteLine("Unable to open or read '%s': %s", path.c_str(), err.what()); } - return result; + return nullptr; } - static void ExtractSourceGames(const std::string& id, json_t& jRoot, Object* result) + static void ExtractSourceGames(const std::string& id, json_t& jRoot, Object& result) { auto sourceGames = jRoot["sourceGame"]; if (sourceGames.is_array() || sourceGames.is_string()) @@ -420,33 +407,34 @@ namespace ObjectFactory } if (!sourceGameVector.empty()) { - result->SetSourceGames(sourceGameVector); + result.SetSourceGames(sourceGameVector); } else { log_error("Object %s has an incorrect sourceGame parameter.", id.c_str()); - result->SetSourceGames({ OBJECT_SOURCE_CUSTOM }); + result.SetSourceGames({ OBJECT_SOURCE_CUSTOM }); } } // >90% of objects are custom, so allow omitting the parameter without displaying an error. else if (sourceGames.is_null()) { - result->SetSourceGames({ OBJECT_SOURCE_CUSTOM }); + result.SetSourceGames({ OBJECT_SOURCE_CUSTOM }); } else { log_error("Object %s has an incorrect sourceGame parameter.", id.c_str()); - result->SetSourceGames({ OBJECT_SOURCE_CUSTOM }); + result.SetSourceGames({ OBJECT_SOURCE_CUSTOM }); } } - Object* CreateObjectFromJson(IObjectRepository& objectRepository, json_t& jRoot, const IFileDataRetriever* fileRetriever) + std::unique_ptr CreateObjectFromJson( + IObjectRepository& objectRepository, json_t& jRoot, const IFileDataRetriever* fileRetriever) { Guard::Assert(jRoot.is_object(), "ObjectFactory::CreateObjectFromJson expects parameter jRoot to be object"); log_verbose("CreateObjectFromJson(...)"); - Object* result = nullptr; + std::unique_ptr result; auto objectType = ParseObjectType(Json::GetString(jRoot["objectType"])); if (objectType != 0xFF) @@ -472,7 +460,6 @@ namespace ObjectFactory result->ReadJson(&readContext, jRoot); if (readContext.WasError()) { - delete result; throw std::runtime_error("Object has errors"); } @@ -487,7 +474,7 @@ namespace ObjectFactory } result->SetAuthors(std::move(authorVector)); - ExtractSourceGames(id, jRoot, result); + ExtractSourceGames(id, jRoot, *result); } return result; } diff --git a/src/openrct2/object/ObjectFactory.h b/src/openrct2/object/ObjectFactory.h index a7a01f6e9c..c626a39fc7 100644 --- a/src/openrct2/object/ObjectFactory.h +++ b/src/openrct2/object/ObjectFactory.h @@ -11,6 +11,7 @@ #include "../common.h" +#include #include struct IObjectRepository; @@ -19,11 +20,11 @@ struct rct_object_entry; namespace ObjectFactory { - Object* CreateObjectFromLegacyFile(IObjectRepository& objectRepository, const utf8* path); - Object* CreateObjectFromLegacyData( + std::unique_ptr CreateObjectFromLegacyFile(IObjectRepository& objectRepository, const utf8* path); + std::unique_ptr CreateObjectFromLegacyData( IObjectRepository& objectRepository, const rct_object_entry* entry, const void* data, size_t dataSize); - Object* CreateObjectFromZipFile(IObjectRepository& objectRepository, const std::string_view& path); - Object* CreateObject(const rct_object_entry& entry); + std::unique_ptr CreateObjectFromZipFile(IObjectRepository& objectRepository, const std::string_view& path); + std::unique_ptr CreateObject(const rct_object_entry& entry); - Object* CreateObjectFromJsonFile(IObjectRepository& objectRepository, const std::string& path); + std::unique_ptr CreateObjectFromJsonFile(IObjectRepository& objectRepository, const std::string& path); } // namespace ObjectFactory diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index 20c49a68a5..a7ebb8341e 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -35,7 +35,7 @@ class ObjectManager final : public IObjectManager { private: IObjectRepository& _objectRepository; - std::vector _loadedObjects; + std::vector> _loadedObjects; std::array, RIDE_TYPE_COUNT> _rideTypeToObjectMap; // Used to return a safe empty vector back from GetAllRideEntries, can be removed when std::span is available @@ -62,7 +62,7 @@ public: { return nullptr; } - return _loadedObjects[index]; + return _loadedObjects[index].get(); } Object* GetLoadedObject(int32_t objectType, size_t index) override @@ -114,14 +114,15 @@ public: int32_t slot = FindSpareSlot(objectType); if (slot != -1) { - loadedObject = GetOrLoadObject(ori); - if (loadedObject != nullptr) + auto object = GetOrLoadObject(ori); + if (object != nullptr) { if (_loadedObjects.size() <= static_cast(slot)) { _loadedObjects.resize(slot + 1); } - _loadedObjects[slot] = loadedObject; + loadedObject = object.get(); + _loadedObjects[slot] = std::move(object); UpdateSceneryGroupIndexes(); ResetTypeToRideEntryIndexMap(); } @@ -140,7 +141,7 @@ public: size_t numNewLoadedObjects = 0; auto loadedObjects = LoadObjects(requiredObjects, &numNewLoadedObjects); - SetNewLoadedObjectList(loadedObjects); + SetNewLoadedObjectList(std::move(loadedObjects)); LoadDefaultObjects(); UpdateSceneryGroupIndexes(); ResetTypeToRideEntryIndexMap(); @@ -177,9 +178,9 @@ public: void UnloadAll() override { - for (auto object : _loadedObjects) + for (auto& object : _loadedObjects) { - UnloadObject(object); + UnloadObject(object.get()); } UpdateSceneryGroupIndexes(); ResetTypeToRideEntryIndexMap(); @@ -187,7 +188,7 @@ public: void ResetObjects() override { - for (auto loadedObject : _loadedObjects) + for (auto& loadedObject : _loadedObjects) { if (loadedObject != nullptr) { @@ -334,7 +335,8 @@ private: Guard::ArgumentNotNull(object, GUARD_LINE); auto result = std::numeric_limits().max(); - auto it = std::find(_loadedObjects.begin(), _loadedObjects.end(), object); + auto it = std::find_if( + _loadedObjects.begin(), _loadedObjects.end(), [object](auto& obj) { return obj.get() == object; }); if (it != _loadedObjects.end()) { result = std::distance(_loadedObjects.begin(), it); @@ -342,7 +344,7 @@ private: return result; } - void SetNewLoadedObjectList(const std::vector& newLoadedObjects) + void SetNewLoadedObjectList(std::vector> newLoadedObjects) { if (newLoadedObjects.empty()) { @@ -352,13 +354,15 @@ private: { UnloadObjectsExcept(newLoadedObjects); } - _loadedObjects = newLoadedObjects; + _loadedObjects = std::move(newLoadedObjects); } void UnloadObject(Object* object) { if (object != nullptr) { + object->Unload(); + // TODO try to prevent doing a repository search const ObjectRepositoryItem* ori = _objectRepository.FindObject(object->GetObjectEntry()); if (ori != nullptr) @@ -370,40 +374,37 @@ private: // slots, we have to make sure find and set all of them to nullptr for (auto& obj : _loadedObjects) { - if (obj == object) + if (obj.get() == object) { obj = nullptr; } } - - object->Unload(); - delete object; } } - void UnloadObjectsExcept(const std::vector& newLoadedObjects) + void UnloadObjectsExcept(const std::vector>& newLoadedObjects) { // Build a hash set for quick checking auto exceptSet = std::unordered_set(); - for (auto object : newLoadedObjects) + for (auto& object : newLoadedObjects) { if (object != nullptr) { - exceptSet.insert(object); + exceptSet.insert(object.get()); } } // Unload objects that are not in the hash set size_t totalObjectsLoaded = 0; size_t numObjectsUnloaded = 0; - for (auto object : _loadedObjects) + for (auto& object : _loadedObjects) { if (object != nullptr) { totalObjectsLoaded++; - if (exceptSet.find(object) == exceptSet.end()) + if (exceptSet.find(object.get()) == exceptSet.end()) { - UnloadObject(object); + UnloadObject(object.get()); numObjectsUnloaded++; } } @@ -414,7 +415,7 @@ private: void UpdateSceneryGroupIndexes() { - for (auto loadedObject : _loadedObjects) + for (auto& loadedObject : _loadedObjects) { if (loadedObject != nullptr) { @@ -423,26 +424,26 @@ private: { case OBJECT_TYPE_SMALL_SCENERY: sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->small_scenery.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + sceneryEntry->small_scenery.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); break; case OBJECT_TYPE_LARGE_SCENERY: sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->large_scenery.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + sceneryEntry->large_scenery.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); break; case OBJECT_TYPE_WALLS: sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->wall.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + sceneryEntry->wall.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); break; case OBJECT_TYPE_BANNERS: sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->banner.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + sceneryEntry->banner.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); break; case OBJECT_TYPE_PATH_BITS: sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->path_bit.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + sceneryEntry->path_bit.scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); break; case OBJECT_TYPE_SCENERY_GROUP: - auto sgObject = dynamic_cast(loadedObject); + auto sgObject = dynamic_cast(loadedObject.get()); sgObject->UpdateEntryIndexes(); break; } @@ -507,17 +508,15 @@ private: } else { - Object* loadedObject = nullptr; - loadedObject = ori->LoadedObject; + auto loadedObject = ori->LoadedObject; if (loadedObject == nullptr) { - loadedObject = _objectRepository.LoadObject(ori); - if (loadedObject == nullptr) + auto object = _objectRepository.LoadObject(ori); + if (object == nullptr) { invalidEntries.push_back(entry); ReportObjectLoadProblem(&entry); } - delete loadedObject; } } } @@ -577,9 +576,10 @@ private: } } - std::vector LoadObjects(std::vector& requiredObjects, size_t* outNewObjectsLoaded) + std::vector> LoadObjects( + std::vector& requiredObjects, size_t* outNewObjectsLoaded) { - std::vector objects; + std::vector> objects; std::vector loadedObjects; std::vector badObjects; objects.resize(OBJECT_ENTRY_COUNT); @@ -589,14 +589,14 @@ private: std::mutex commonMutex; ParallelFor(requiredObjects, [this, &commonMutex, requiredObjects, &objects, &badObjects, &loadedObjects](size_t i) { auto ori = requiredObjects[i]; - Object* loadedObject = nullptr; + std::unique_ptr object; if (ori != nullptr) { - loadedObject = ori->LoadedObject; + auto loadedObject = ori->LoadedObject; if (loadedObject == nullptr) { - loadedObject = _objectRepository.LoadObject(ori); - if (loadedObject == nullptr) + object = _objectRepository.LoadObject(ori); + if (object == nullptr) { std::lock_guard guard(commonMutex); badObjects.push_back(ori->ObjectEntry); @@ -605,13 +605,13 @@ private: else { std::lock_guard guard(commonMutex); - loadedObjects.push_back(loadedObject); + loadedObjects.push_back(object.get()); // Connect the ori to the registered object - _objectRepository.RegisterLoadedObject(ori, loadedObject); + _objectRepository.RegisterLoadedObject(ori, object.get()); } } } - objects[i] = loadedObject; + objects[i] = std::move(object); }); // Load objects @@ -637,22 +637,23 @@ private: return objects; } - Object* GetOrLoadObject(const ObjectRepositoryItem* ori) + std::unique_ptr GetOrLoadObject(const ObjectRepositoryItem* ori) { - Object* loadedObject = ori->LoadedObject; + std::unique_ptr object; + auto loadedObject = ori->LoadedObject; if (loadedObject == nullptr) { // Try to load object - loadedObject = _objectRepository.LoadObject(ori); - if (loadedObject != nullptr) + object = _objectRepository.LoadObject(ori); + if (object != nullptr) { - loadedObject->Load(); + object->Load(); // Connect the ori to the registered object - _objectRepository.RegisterLoadedObject(ori, loadedObject); + _objectRepository.RegisterLoadedObject(ori, object.get()); } } - return loadedObject; + return object; } void ResetTypeToRideEntryIndexMap() @@ -717,18 +718,18 @@ std::unique_ptr CreateObjectManager(IObjectRepository& objectRep return std::make_unique(objectRepository); } -void* object_manager_get_loaded_object_by_index(size_t index) +Object* object_manager_get_loaded_object_by_index(size_t index) { auto& objectManager = OpenRCT2::GetContext()->GetObjectManager(); Object* loadedObject = objectManager.GetLoadedObject(index); - return static_cast(loadedObject); + return loadedObject; } -void* object_manager_get_loaded_object(const rct_object_entry* entry) +Object* object_manager_get_loaded_object(const rct_object_entry* entry) { auto& objectManager = OpenRCT2::GetContext()->GetObjectManager(); Object* loadedObject = objectManager.GetLoadedObject(entry); - return static_cast(loadedObject); + return loadedObject; } ObjectEntryIndex object_manager_get_loaded_object_entry_index(const void* loadedObject) diff --git a/src/openrct2/object/ObjectManager.h b/src/openrct2/object/ObjectManager.h index 3e2a0b4a01..6084b364fe 100644 --- a/src/openrct2/object/ObjectManager.h +++ b/src/openrct2/object/ObjectManager.h @@ -44,8 +44,8 @@ struct IObjectManager std::unique_ptr CreateObjectManager(IObjectRepository& objectRepository); -void* object_manager_get_loaded_object_by_index(size_t index); -void* object_manager_get_loaded_object(const rct_object_entry* entry); +Object* object_manager_get_loaded_object_by_index(size_t index); +Object* object_manager_get_loaded_object(const rct_object_entry* entry); ObjectEntryIndex object_manager_get_loaded_object_entry_index(const void* loadedObject); void* object_manager_load_object(const rct_object_entry* entry); void object_manager_unload_objects(const std::vector& entries); diff --git a/src/openrct2/object/ObjectRepository.cpp b/src/openrct2/object/ObjectRepository.cpp index 32a729e407..694eaa048f 100644 --- a/src/openrct2/object/ObjectRepository.cpp +++ b/src/openrct2/object/ObjectRepository.cpp @@ -94,7 +94,7 @@ public: public: std::tuple Create([[maybe_unused]] int32_t language, const std::string& path) const override { - Object* object = nullptr; + std::unique_ptr object; auto extension = Path::GetExtension(path); if (String::Equals(extension, ".json", true)) { @@ -117,7 +117,6 @@ public: item.Authors = object->GetAuthors(); item.Sources = object->GetSourceGames(); object->SetRepositoryItem(&item); - delete object; return std::make_tuple(true, item); } return std::make_tuple(false, ObjectRepositoryItem()); @@ -290,7 +289,7 @@ public: return nullptr; } - Object* LoadObject(const ObjectRepositoryItem* ori) override + std::unique_ptr LoadObject(const ObjectRepositoryItem* ori) override { Guard::ArgumentNotNull(ori, GUARD_LINE); @@ -332,7 +331,7 @@ public: object_entry_get_name_fixed(objectName, sizeof(objectName), objectEntry); // Check that the object is loadable before writing it - Object* object = ObjectFactory::CreateObjectFromLegacyData(*this, objectEntry, data, dataSize); + auto object = ObjectFactory::CreateObjectFromLegacyData(*this, objectEntry, data, dataSize); if (object == nullptr) { Console::Error::WriteLine("[%s] Unable to export object.", objectName); @@ -694,9 +693,9 @@ const rct_object_entry* object_list_find(rct_object_entry* entry) return result; } -void* object_repository_load_object(const rct_object_entry* objectEntry) +std::unique_ptr object_repository_load_object(const rct_object_entry* objectEntry) { - Object* object = nullptr; + std::unique_ptr object; auto& objRepository = GetContext()->GetObjectRepository(); const ObjectRepositoryItem* ori = objRepository.FindObject(objectEntry); if (ori != nullptr) @@ -707,7 +706,7 @@ void* object_repository_load_object(const rct_object_entry* objectEntry) object->Load(); } } - return static_cast(object); + return object; } void scenario_translate(scenario_index_entry* scenarioEntry) @@ -750,22 +749,6 @@ const ObjectRepositoryItem* object_repository_find_object_by_name(const char* na return objectRepository.FindObject(name); } -void object_delete(void* object) -{ - if (object != nullptr) - { - Object* baseObject = static_cast(object); - baseObject->Unload(); - delete baseObject; - } -} - -void object_draw_preview(const void* object, rct_drawpixelinfo* dpi, int32_t width, int32_t height) -{ - const Object* baseObject = static_cast(object); - baseObject->DrawPreview(dpi, width, height); -} - bool object_entry_compare(const rct_object_entry* a, const rct_object_entry* b) { // If an official object don't bother checking checksum diff --git a/src/openrct2/object/ObjectRepository.h b/src/openrct2/object/ObjectRepository.h index f165344f48..cfb37161e3 100644 --- a/src/openrct2/object/ObjectRepository.h +++ b/src/openrct2/object/ObjectRepository.h @@ -74,7 +74,7 @@ struct IObjectRepository virtual const ObjectRepositoryItem* FindObject(const std::string_view& legacyIdentifier) const abstract; virtual const ObjectRepositoryItem* FindObject(const rct_object_entry* objectEntry) const abstract; - virtual Object* LoadObject(const ObjectRepositoryItem* ori) abstract; + virtual std::unique_ptr LoadObject(const ObjectRepositoryItem* ori) abstract; virtual void RegisterLoadedObject(const ObjectRepositoryItem* ori, Object* object) abstract; virtual void UnregisterLoadedObject(const ObjectRepositoryItem* ori, Object* object) abstract; @@ -93,7 +93,4 @@ size_t object_repository_get_items_count(); const ObjectRepositoryItem* object_repository_get_items(); const ObjectRepositoryItem* object_repository_find_object_by_entry(const rct_object_entry* entry); const ObjectRepositoryItem* object_repository_find_object_by_name(const char* name); -void* object_repository_load_object(const rct_object_entry* objectEntry); - -void object_delete(void* object); -void object_draw_preview(const void* object, rct_drawpixelinfo* dpi, int32_t width, int32_t height); +std::unique_ptr object_repository_load_object(const rct_object_entry* objectEntry); diff --git a/src/openrct2/rct1/S4Importer.cpp b/src/openrct2/rct1/S4Importer.cpp index c90a3f4452..63f64de9a6 100644 --- a/src/openrct2/rct1/S4Importer.cpp +++ b/src/openrct2/rct1/S4Importer.cpp @@ -1983,13 +1983,12 @@ private: } else { - Object* object = objectRepository.LoadObject(ori); + auto object = objectRepository.LoadObject(ori); if (object == nullptr && objectType != OBJECT_TYPE_SCENERY_GROUP) { missingObjects.push_back(entry); Console::Error::WriteLine("[%s] Object could not be loaded.", objectName); } - delete object; } } } diff --git a/src/openrct2/rct2/T6Importer.cpp b/src/openrct2/rct2/T6Importer.cpp index 4d1a29d776..09e97a9917 100644 --- a/src/openrct2/rct2/T6Importer.cpp +++ b/src/openrct2/rct2/T6Importer.cpp @@ -217,16 +217,16 @@ public: if (RCT2RideTypeNeedsConversion(td->type)) { std::scoped_lock lock(_objectLookupMutex); - auto* rawObject = object_repository_load_object(&td->vehicle_object); + auto rawObject = object_repository_load_object(&td->vehicle_object); if (rawObject != nullptr) { const auto* rideEntry = static_cast( - static_cast(rawObject)->GetLegacyData()); + static_cast(rawObject.get())->GetLegacyData()); if (rideEntry != nullptr) { td->type = RCT2RideTypeToOpenRCT2RideType(td->type, rideEntry); } - object_delete(rawObject); + rawObject->Unload(); } } }