From 1f4f0c015c3a5e65e5ef67ffe01d980fc49e508f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Fri, 3 Sep 2021 13:43:06 +0300 Subject: [PATCH 1/6] Fix ownership of loaded object data --- src/openrct2/object/ObjectManager.cpp | 287 ++++++++++----------- src/openrct2/object/ObjectRepository.cpp | 6 +- src/openrct2/object/ObjectRepository.h | 4 +- src/openrct2/object/SceneryGroupObject.cpp | 2 +- 4 files changed, 142 insertions(+), 157 deletions(-) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index b72f6c0bd8..c0bcc39faa 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -36,7 +36,8 @@ 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 @@ -63,7 +64,7 @@ public: { return nullptr; } - return _loadedObjects[index].get(); + return _loadedObjects[index]; } Object* GetLoadedObject(ObjectType objectType, size_t index) override @@ -82,13 +83,11 @@ public: Object* GetLoadedObject(const ObjectEntryDescriptor& entry) override { - Object* loadedObject = nullptr; const ObjectRepositoryItem* ori = _objectRepository.FindObject(entry); - if (ori != nullptr) - { - loadedObject = ori->LoadedObject; - } - return loadedObject; + if (ori == nullptr) + return nullptr; + + return ori->LoadedObject.get(); } ObjectEntryIndex GetLoadedObjectEntryIndex(const Object* object) override @@ -142,7 +141,7 @@ public: const ObjectRepositoryItem* ori = _objectRepository.FindObject(&entry); if (ori != nullptr) { - Object* loadedObject = ori->LoadedObject; + Object* loadedObject = ori->LoadedObject.get(); if (loadedObject != nullptr) { UnloadObject(loadedObject); @@ -162,7 +161,7 @@ public: { for (auto& object : _loadedObjects) { - UnloadObject(object.get()); + UnloadObject(object); } UpdateSceneryGroupIndexes(); ResetTypeToRideEntryIndexMap(); @@ -333,42 +332,42 @@ private: Object* RepositoryItemToObject(const ObjectRepositoryItem* ori, std::optional slot = {}) { - Object* loadedObject = nullptr; - if (ori != nullptr) + if (ori == nullptr) + return nullptr; + + Object* loadedObject = ori->LoadedObject.get(); + if (loadedObject != nullptr) + return loadedObject; + + ObjectType objectType = ori->ObjectEntry.GetType(); + if (slot) { - loadedObject = ori->LoadedObject; - if (loadedObject == nullptr) + if (_loadedObjects.size() > static_cast(*slot) && _loadedObjects[*slot] != nullptr) { - ObjectType objectType = ori->ObjectEntry.GetType(); - if (slot) - { - if (_loadedObjects.size() > static_cast(*slot) && _loadedObjects[*slot] != nullptr) - { - // Slot already taken - return nullptr; - } - } - else - { - slot = FindSpareSlot(objectType); - } - if (slot) - { - auto object = GetOrLoadObject(ori); - if (object != nullptr) - { - if (_loadedObjects.size() <= static_cast(*slot)) - { - _loadedObjects.resize(*slot + 1); - } - loadedObject = object.get(); - _loadedObjects[*slot] = std::move(object); - UpdateSceneryGroupIndexes(); - ResetTypeToRideEntryIndexMap(); - } - } + // Slot already taken + return nullptr; } } + else + { + slot = FindSpareSlot(objectType); + } + if (slot) + { + auto object = GetOrLoadObject(ori); + if (object != nullptr) + { + if (_loadedObjects.size() <= static_cast(*slot)) + { + _loadedObjects.resize(*slot + 1); + } + loadedObject = object; + _loadedObjects[*slot] = object; + UpdateSceneryGroupIndexes(); + ResetTypeToRideEntryIndexMap(); + } + } + return loadedObject; } @@ -396,8 +395,7 @@ private: Guard::ArgumentNotNull(object, GUARD_LINE); auto result = std::numeric_limits().max(); - auto it = std::find_if( - _loadedObjects.begin(), _loadedObjects.end(), [object](auto& obj) { return obj.get() == object; }); + auto it = std::find_if(_loadedObjects.begin(), _loadedObjects.end(), [object](auto& obj) { return obj == object; }); if (it != _loadedObjects.end()) { result = std::distance(_loadedObjects.begin(), it); @@ -405,7 +403,7 @@ private: return result; } - void SetNewLoadedObjectList(std::vector>&& newLoadedObjects) + void SetNewLoadedObjectList(std::vector&& newLoadedObjects) { if (newLoadedObjects.empty()) { @@ -420,30 +418,24 @@ private: void UnloadObject(Object* object) { - if (object != nullptr) + if (object == nullptr) + return; + + object->Unload(); + + // TODO try to prevent doing a repository search + const ObjectRepositoryItem* ori = _objectRepository.FindObject(object->GetObjectEntry()); + if (ori != nullptr) { - object->Unload(); - - // TODO try to prevent doing a repository search - const ObjectRepositoryItem* ori = _objectRepository.FindObject(object->GetObjectEntry()); - if (ori != nullptr) - { - _objectRepository.UnregisterLoadedObject(ori, object); - } - - // Because it's possible to have the same loaded object for multiple - // slots, we have to make sure find and set all of them to nullptr - for (auto& obj : _loadedObjects) - { - if (obj.get() == object) - { - obj = nullptr; - } - } + _objectRepository.UnregisterLoadedObject(ori, object); } + + // Because it's possible to have the same loaded object for multiple + // slots, we have to make sure find and set all of them to nullptr + std::replace(_loadedObjects.begin(), _loadedObjects.end(), object, static_cast(nullptr)); } - void UnloadObjectsExcept(const std::vector>& newLoadedObjects) + void UnloadObjectsExcept(const std::vector& newLoadedObjects) { // Build a hash set for quick checking auto exceptSet = std::unordered_set(); @@ -451,7 +443,7 @@ private: { if (object != nullptr) { - exceptSet.insert(object.get()); + exceptSet.insert(object); } } @@ -463,9 +455,9 @@ private: if (object != nullptr) { totalObjectsLoaded++; - if (exceptSet.find(object.get()) == exceptSet.end()) + if (exceptSet.find(object) == exceptSet.end()) { - UnloadObject(object.get()); + UnloadObject(object); numObjectsUnloaded++; } } @@ -478,50 +470,51 @@ private: { for (auto& loadedObject : _loadedObjects) { - if (loadedObject != nullptr) + // The list can contain unused slots, skip them. + if (loadedObject == nullptr) + continue; + + switch (loadedObject->GetObjectType()) { - switch (loadedObject->GetObjectType()) + case ObjectType::SmallScenery: { - case ObjectType::SmallScenery: - { - auto* sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); - break; - } - case ObjectType::LargeScenery: - { - auto* sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); - break; - } - case ObjectType::Walls: - { - auto* wallEntry = static_cast(loadedObject->GetLegacyData()); - wallEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); - break; - } - case ObjectType::Banners: - { - auto* bannerEntry = static_cast(loadedObject->GetLegacyData()); - bannerEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); - break; - } - case ObjectType::PathBits: - { - auto* pathBitEntry = static_cast(loadedObject->GetLegacyData()); - pathBitEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject.get()); - break; - } - case ObjectType::SceneryGroup: - { - auto sgObject = dynamic_cast(loadedObject.get()); - sgObject->UpdateEntryIndexes(); - break; - } - default: - // This switch only handles scenery ObjectTypes. - break; + auto* sceneryEntry = static_cast(loadedObject->GetLegacyData()); + sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + break; } + case ObjectType::LargeScenery: + { + auto* sceneryEntry = static_cast(loadedObject->GetLegacyData()); + sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + break; + } + case ObjectType::Walls: + { + auto* wallEntry = static_cast(loadedObject->GetLegacyData()); + wallEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + break; + } + case ObjectType::Banners: + { + auto* bannerEntry = static_cast(loadedObject->GetLegacyData()); + bannerEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + break; + } + case ObjectType::PathBits: + { + auto* pathBitEntry = static_cast(loadedObject->GetLegacyData()); + pathBitEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + break; + } + case ObjectType::SceneryGroup: + { + auto sgObject = dynamic_cast(loadedObject); + sgObject->UpdateEntryIndexes(); + break; + } + default: + // This switch only handles scenery ObjectTypes. + break; } } @@ -583,7 +576,7 @@ private: } else { - auto loadedObject = ori->LoadedObject; + auto* loadedObject = ori->LoadedObject.get(); if (loadedObject == nullptr) { auto object = _objectRepository.LoadObject(ori); @@ -651,61 +644,51 @@ private: } } - std::vector> LoadObjects( - std::vector& requiredObjects, size_t* outNewObjectsLoaded) + std::vector LoadObjects(std::vector& requiredObjects, size_t* outNewObjectsLoaded) { - std::vector> objects; - std::vector loadedObjects; + std::vector objects; + std::vector newLoadedObjects; std::vector badObjects; objects.resize(OBJECT_ENTRY_COUNT); - loadedObjects.reserve(OBJECT_ENTRY_COUNT); + newLoadedObjects.reserve(OBJECT_ENTRY_COUNT); // Read objects std::mutex commonMutex; - ParallelFor(requiredObjects, [this, &commonMutex, requiredObjects, &objects, &badObjects, &loadedObjects](size_t i) { + ParallelFor(requiredObjects, [this, &commonMutex, requiredObjects, &objects, &badObjects, &newLoadedObjects](size_t i) { auto requiredObject = requiredObjects[i]; - std::unique_ptr object; + Object* object = nullptr; if (requiredObject != nullptr) { - auto loadedObject = requiredObject->LoadedObject; + auto loadedObject = requiredObject->LoadedObject.get(); if (loadedObject == nullptr) { // Object requires to be loaded, if the object successfully loads it will register it // as a loaded object otherwise placed into the badObjects list. - object = _objectRepository.LoadObject(requiredObject); + auto newObject = _objectRepository.LoadObject(requiredObject); std::lock_guard guard(commonMutex); - if (object == nullptr) + if (newObject == nullptr) { badObjects.push_back(requiredObject->ObjectEntry); ReportObjectLoadProblem(&requiredObject->ObjectEntry); } else { - loadedObjects.push_back(object.get()); + object = newObject.get(); + newLoadedObjects.push_back(object); // Connect the ori to the registered object - _objectRepository.RegisterLoadedObject(requiredObject, object.get()); + _objectRepository.RegisterLoadedObject(requiredObject, std::move(newObject)); } } else { - // The object is already loaded, given that the new list will be used as the next loaded object list, - // we can move the element out safely. This is required as the resulting list must contain all loaded - // objects and not just the newly loaded ones. - std::lock_guard guard(commonMutex); - auto it = std::find_if(_loadedObjects.begin(), _loadedObjects.end(), [loadedObject](const auto& obj) { - return obj.get() == loadedObject; - }); - if (it != _loadedObjects.end()) - { - object = std::move(*it); - } + object = loadedObject; } } - objects[i] = std::move(object); + objects[i] = object; }); // Load objects - for (auto obj : loadedObjects) + for (auto obj : newLoadedObjects) { obj->Load(); } @@ -713,7 +696,7 @@ private: if (!badObjects.empty()) { // Unload all the new objects we loaded - for (auto object : loadedObjects) + for (auto object : newLoadedObjects) { UnloadObject(object); } @@ -722,28 +705,30 @@ private: if (outNewObjectsLoaded != nullptr) { - *outNewObjectsLoaded = loadedObjects.size(); + *outNewObjectsLoaded = newLoadedObjects.size(); } return objects; } - std::unique_ptr GetOrLoadObject(const ObjectRepositoryItem* ori) + Object* GetOrLoadObject(const ObjectRepositoryItem* ori) { - std::unique_ptr object; - auto loadedObject = ori->LoadedObject; - if (loadedObject == nullptr) - { - // Try to load object - object = _objectRepository.LoadObject(ori); - if (object != nullptr) - { - object->Load(); + auto* loadedObject = ori->LoadedObject.get(); + if (loadedObject != nullptr) + return loadedObject; - // Connect the ori to the registered object - _objectRepository.RegisterLoadedObject(ori, object.get()); - } + // Try to load object + auto object = _objectRepository.LoadObject(ori); + if (object != nullptr) + { + loadedObject = object.get(); + + object->Load(); + + // Connect the ori to the registered object + _objectRepository.RegisterLoadedObject(ori, std::move(object)); } - return object; + + return loadedObject; } void ResetTypeToRideEntryIndexMap() diff --git a/src/openrct2/object/ObjectRepository.cpp b/src/openrct2/object/ObjectRepository.cpp index e436e36563..ea19c419bf 100644 --- a/src/openrct2/object/ObjectRepository.cpp +++ b/src/openrct2/object/ObjectRepository.cpp @@ -266,18 +266,18 @@ public: } } - void RegisterLoadedObject(const ObjectRepositoryItem* ori, Object* object) override + void RegisterLoadedObject(const ObjectRepositoryItem* ori, std::unique_ptr&& object) override { ObjectRepositoryItem* item = &_items[ori->Id]; Guard::Assert(item->LoadedObject == nullptr, GUARD_LINE); - item->LoadedObject = object; + item->LoadedObject = std::move(object); } void UnregisterLoadedObject(const ObjectRepositoryItem* ori, Object* object) override { ObjectRepositoryItem* item = &_items[ori->Id]; - if (item->LoadedObject == object) + if (item->LoadedObject.get() == object) { item->LoadedObject = nullptr; } diff --git a/src/openrct2/object/ObjectRepository.h b/src/openrct2/object/ObjectRepository.h index 049fb54fe0..e6de1cce14 100644 --- a/src/openrct2/object/ObjectRepository.h +++ b/src/openrct2/object/ObjectRepository.h @@ -43,7 +43,7 @@ struct ObjectRepositoryItem std::string Name; std::vector Authors; std::vector Sources; - Object* LoadedObject{}; + std::shared_ptr LoadedObject{}; struct { uint8_t RideFlags; @@ -82,7 +82,7 @@ struct IObjectRepository [[nodiscard]] virtual const ObjectRepositoryItem* FindObject(const ObjectEntryDescriptor& oed) const abstract; [[nodiscard]] virtual std::unique_ptr LoadObject(const ObjectRepositoryItem* ori) abstract; - virtual void RegisterLoadedObject(const ObjectRepositoryItem* ori, Object* object) abstract; + virtual void RegisterLoadedObject(const ObjectRepositoryItem* ori, std::unique_ptr&& object) abstract; virtual void UnregisterLoadedObject(const ObjectRepositoryItem* ori, Object* object) abstract; virtual void AddObject(const rct_object_entry* objectEntry, const void* data, size_t dataSize) abstract; diff --git a/src/openrct2/object/SceneryGroupObject.cpp b/src/openrct2/object/SceneryGroupObject.cpp index aa6a8bfd20..166245d8b1 100644 --- a/src/openrct2/object/SceneryGroupObject.cpp +++ b/src/openrct2/object/SceneryGroupObject.cpp @@ -81,7 +81,7 @@ void SceneryGroupObject::UpdateEntryIndexes() if (ori->LoadedObject == nullptr) continue; - auto entryIndex = objectManager.GetLoadedObjectEntryIndex(ori->LoadedObject); + auto entryIndex = objectManager.GetLoadedObjectEntryIndex(ori->LoadedObject.get()); Guard::Assert(entryIndex != OBJECT_ENTRY_INDEX_NULL, GUARD_LINE); auto sceneryType = ori->ObjectEntry.GetSceneryType(); From a44f3017dc276843bb5f1bb2de9004054688d967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Fri, 3 Sep 2021 14:01:34 +0300 Subject: [PATCH 2/6] Move the log print and remove optional parameter --- src/openrct2/object/ObjectManager.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index c0bcc39faa..14902b118c 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -119,14 +119,17 @@ public: auto requiredObjects = GetRequiredObjects(entries, count); // Load the required objects - size_t numNewLoadedObjects = 0; - auto loadedObjects = LoadObjects(requiredObjects, &numNewLoadedObjects); + { + auto loadedObjects = LoadObjects(requiredObjects); + SetNewLoadedObjectList(std::move(loadedObjects)); + } - SetNewLoadedObjectList(std::move(loadedObjects)); + // Load defaults. LoadDefaultObjects(); + + // Update indices. UpdateSceneryGroupIndexes(); ResetTypeToRideEntryIndexMap(); - log_verbose("%u / %u new objects loaded", numNewLoadedObjects, requiredObjects.size()); } void UnloadObjects(const std::vector& entries) override @@ -644,7 +647,7 @@ private: } } - std::vector LoadObjects(std::vector& requiredObjects, size_t* outNewObjectsLoaded) + std::vector LoadObjects(std::vector& requiredObjects) { std::vector objects; std::vector newLoadedObjects; @@ -703,10 +706,7 @@ private: throw ObjectLoadException(std::move(badObjects)); } - if (outNewObjectsLoaded != nullptr) - { - *outNewObjectsLoaded = newLoadedObjects.size(); - } + log_verbose("%u / %u new objects loaded", newLoadedObjects.size(), requiredObjects.size()); return objects; } From 976c6148812f0ccf1ea9b43236e28631f37980df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Fri, 3 Sep 2021 14:08:17 +0300 Subject: [PATCH 3/6] Reduce duplicate code --- src/openrct2/object/ObjectManager.cpp | 42 ++++++++++++--------------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index 14902b118c..ef84127b3d 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -469,6 +469,18 @@ private: log_verbose("%u / %u objects unloaded", numObjectsUnloaded, totalObjectsLoaded); } + template static void UpdateSceneryGroupIndexes(Object* object) + { + auto* sceneryEntry = static_cast(object->GetLegacyData()); + sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + } + + template<> static void UpdateSceneryGroupIndexes(Object* object) + { + auto sgObject = dynamic_cast(object); + sgObject->UpdateEntryIndexes(); + } + void UpdateSceneryGroupIndexes() { for (auto& loadedObject : _loadedObjects) @@ -480,41 +492,23 @@ private: switch (loadedObject->GetObjectType()) { case ObjectType::SmallScenery: - { - auto* sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + UpdateSceneryGroupIndexes(loadedObject); break; - } case ObjectType::LargeScenery: - { - auto* sceneryEntry = static_cast(loadedObject->GetLegacyData()); - sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + UpdateSceneryGroupIndexes(loadedObject); break; - } case ObjectType::Walls: - { - auto* wallEntry = static_cast(loadedObject->GetLegacyData()); - wallEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + UpdateSceneryGroupIndexes(loadedObject); break; - } case ObjectType::Banners: - { - auto* bannerEntry = static_cast(loadedObject->GetLegacyData()); - bannerEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + UpdateSceneryGroupIndexes(loadedObject); break; - } case ObjectType::PathBits: - { - auto* pathBitEntry = static_cast(loadedObject->GetLegacyData()); - pathBitEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); + UpdateSceneryGroupIndexes(loadedObject); break; - } case ObjectType::SceneryGroup: - { - auto sgObject = dynamic_cast(loadedObject); - sgObject->UpdateEntryIndexes(); + UpdateSceneryGroupIndexes(loadedObject); break; - } default: // This switch only handles scenery ObjectTypes. break; From a118b1691254844852539e63fc48036e8315f78d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Fri, 3 Sep 2021 14:10:42 +0300 Subject: [PATCH 4/6] Remove unused functions --- src/openrct2/object/ObjectManager.cpp | 61 --------------------------- src/openrct2/object/ObjectManager.h | 1 - 2 files changed, 62 deletions(-) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index ef84127b3d..4a125b8ad1 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -534,60 +534,6 @@ private: return entryIndex; } - rct_object_entry* DuplicateObjectEntry(const rct_object_entry* original) - { - rct_object_entry* duplicate = Memory::Allocate(sizeof(rct_object_entry)); - duplicate->checksum = original->checksum; - strncpy(duplicate->name, original->name, 8); - duplicate->flags = original->flags; - return duplicate; - } - - std::vector GetInvalidObjects(const rct_object_entry* entries) override - { - std::vector invalidEntries; - invalidEntries.reserve(OBJECT_ENTRY_COUNT); - for (int32_t i = 0; i < OBJECT_ENTRY_COUNT; i++) - { - auto entry = entries[i]; - const ObjectRepositoryItem* ori = nullptr; - if (object_entry_is_empty(&entry)) - { - entry = {}; - continue; - } - - ori = _objectRepository.FindObject(&entry); - if (ori == nullptr) - { - if (entry.GetType() != ObjectType::ScenarioText) - { - invalidEntries.push_back(entry); - ReportMissingObject(&entry); - } - else - { - entry = {}; - continue; - } - } - else - { - auto* loadedObject = ori->LoadedObject.get(); - if (loadedObject == nullptr) - { - auto object = _objectRepository.LoadObject(ori); - if (object == nullptr) - { - invalidEntries.push_back(entry); - ReportObjectLoadProblem(&entry); - } - } - } - } - return invalidEntries; - } - std::vector GetRequiredObjects(const rct_object_entry* entries, size_t count) { std::vector requiredObjects; @@ -787,13 +733,6 @@ std::unique_ptr CreateObjectManager(IObjectRepository& objectRep return std::make_unique(objectRepository); } -Object* object_manager_get_loaded_object_by_index(size_t index) -{ - auto& objectManager = OpenRCT2::GetContext()->GetObjectManager(); - Object* loadedObject = objectManager.GetLoadedObject(index); - return loadedObject; -} - Object* object_manager_get_loaded_object(const ObjectEntryDescriptor& entry) { auto& objectManager = OpenRCT2::GetContext()->GetObjectManager(); diff --git a/src/openrct2/object/ObjectManager.h b/src/openrct2/object/ObjectManager.h index 1dbf7fc2a5..ed22926211 100644 --- a/src/openrct2/object/ObjectManager.h +++ b/src/openrct2/object/ObjectManager.h @@ -45,7 +45,6 @@ struct IObjectManager [[nodiscard]] std::unique_ptr CreateObjectManager(IObjectRepository& objectRepository); -[[nodiscard]] Object* object_manager_get_loaded_object_by_index(size_t index); [[nodiscard]] Object* object_manager_get_loaded_object(const ObjectEntryDescriptor& entry); [[nodiscard]] ObjectEntryIndex object_manager_get_loaded_object_entry_index(const Object* loadedObject); [[nodiscard]] ObjectEntryIndex object_manager_get_loaded_object_entry_index(const ObjectEntryDescriptor& entry); From cf0425d9538d05f194a45dc37cbb3bd738cdbf70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Fri, 3 Sep 2021 14:17:53 +0300 Subject: [PATCH 5/6] Fix building --- src/openrct2/object/ObjectManager.cpp | 15 ++++++--------- src/openrct2/object/ObjectManager.h | 1 - 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index 4a125b8ad1..acb487a00e 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -469,16 +469,10 @@ private: log_verbose("%u / %u objects unloaded", numObjectsUnloaded, totalObjectsLoaded); } - template static void UpdateSceneryGroupIndexes(Object* object) + template void UpdateSceneryGroupIndexes(Object* object) { auto* sceneryEntry = static_cast(object->GetLegacyData()); - sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(loadedObject); - } - - template<> static void UpdateSceneryGroupIndexes(Object* object) - { - auto sgObject = dynamic_cast(object); - sgObject->UpdateEntryIndexes(); + sceneryEntry->scenery_tab_id = GetPrimarySceneryGroupEntryIndex(object); } void UpdateSceneryGroupIndexes() @@ -507,8 +501,11 @@ private: UpdateSceneryGroupIndexes(loadedObject); break; case ObjectType::SceneryGroup: - UpdateSceneryGroupIndexes(loadedObject); + { + auto sgObject = dynamic_cast(loadedObject); + sgObject->UpdateEntryIndexes(); break; + } default: // This switch only handles scenery ObjectTypes. break; diff --git a/src/openrct2/object/ObjectManager.h b/src/openrct2/object/ObjectManager.h index ed22926211..a93b9f5c10 100644 --- a/src/openrct2/object/ObjectManager.h +++ b/src/openrct2/object/ObjectManager.h @@ -28,7 +28,6 @@ struct IObjectManager virtual Object* GetLoadedObject(ObjectType objectType, size_t index) abstract; virtual Object* GetLoadedObject(const ObjectEntryDescriptor& entry) abstract; virtual ObjectEntryIndex GetLoadedObjectEntryIndex(const Object* object) abstract; - virtual std::vector GetInvalidObjects(const rct_object_entry* entries) abstract; virtual Object* LoadObject(std::string_view identifier) abstract; virtual Object* LoadObject(const rct_object_entry* entry) abstract; From 6186b29675567fc085f71251a42ae867e50c9026 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Fri, 3 Sep 2021 16:26:13 +0300 Subject: [PATCH 6/6] Apply review suggestion --- src/openrct2/object/ObjectManager.cpp | 93 +++++++++++++-------------- 1 file changed, 44 insertions(+), 49 deletions(-) diff --git a/src/openrct2/object/ObjectManager.cpp b/src/openrct2/object/ObjectManager.cpp index acb487a00e..85e9871e6d 100644 --- a/src/openrct2/object/ObjectManager.cpp +++ b/src/openrct2/object/ObjectManager.cpp @@ -119,10 +119,7 @@ public: auto requiredObjects = GetRequiredObjects(entries, count); // Load the required objects - { - auto loadedObjects = LoadObjects(requiredObjects); - SetNewLoadedObjectList(std::move(loadedObjects)); - } + LoadObjects(requiredObjects); // Load defaults. LoadDefaultObjects(); @@ -162,7 +159,7 @@ public: void UnloadAll() override { - for (auto& object : _loadedObjects) + for (auto* object : _loadedObjects) { UnloadObject(object); } @@ -357,7 +354,7 @@ private: } if (slot) { - auto object = GetOrLoadObject(ori); + auto* object = GetOrLoadObject(ori); if (object != nullptr) { if (_loadedObjects.size() <= static_cast(*slot)) @@ -398,7 +395,7 @@ private: Guard::ArgumentNotNull(object, GUARD_LINE); auto result = std::numeric_limits().max(); - auto it = std::find_if(_loadedObjects.begin(), _loadedObjects.end(), [object](auto& obj) { return obj == object; }); + auto it = std::find(_loadedObjects.begin(), _loadedObjects.end(), object); if (it != _loadedObjects.end()) { result = std::distance(_loadedObjects.begin(), it); @@ -406,19 +403,6 @@ private: return result; } - void SetNewLoadedObjectList(std::vector&& newLoadedObjects) - { - if (newLoadedObjects.empty()) - { - UnloadAll(); - } - else - { - UnloadObjectsExcept(newLoadedObjects); - } - _loadedObjects = std::move(newLoadedObjects); - } - void UnloadObject(Object* object) { if (object == nullptr) @@ -453,16 +437,16 @@ private: // 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) + if (object == nullptr) + continue; + + totalObjectsLoaded++; + if (exceptSet.find(object) == exceptSet.end()) { - totalObjectsLoaded++; - if (exceptSet.find(object) == exceptSet.end()) - { - UnloadObject(object); - numObjectsUnloaded++; - } + UnloadObject(object); + numObjectsUnloaded++; } } @@ -477,7 +461,7 @@ private: void UpdateSceneryGroupIndexes() { - for (auto& loadedObject : _loadedObjects) + for (auto* loadedObject : _loadedObjects) { // The list can contain unused slots, skip them. if (loadedObject == nullptr) @@ -519,7 +503,7 @@ private: ObjectEntryIndex GetPrimarySceneryGroupEntryIndex(Object* loadedObject) { - auto sceneryObject = dynamic_cast(loadedObject); + auto* sceneryObject = dynamic_cast(loadedObject); const auto& primarySGEntry = sceneryObject->GetPrimarySceneryGroup(); Object* sgObject = GetLoadedObject(primarySGEntry); @@ -584,7 +568,7 @@ private: } } - std::vector LoadObjects(std::vector& requiredObjects) + void LoadObjects(std::vector& requiredObjects) { std::vector objects; std::vector newLoadedObjects; @@ -595,11 +579,11 @@ private: // Read objects std::mutex commonMutex; ParallelFor(requiredObjects, [this, &commonMutex, requiredObjects, &objects, &badObjects, &newLoadedObjects](size_t i) { - auto requiredObject = requiredObjects[i]; + auto* requiredObject = requiredObjects[i]; Object* object = nullptr; if (requiredObject != nullptr) { - auto loadedObject = requiredObject->LoadedObject.get(); + auto* loadedObject = requiredObject->LoadedObject.get(); if (loadedObject == nullptr) { // Object requires to be loaded, if the object successfully loads it will register it @@ -628,7 +612,7 @@ private: }); // Load objects - for (auto obj : newLoadedObjects) + for (auto* obj : newLoadedObjects) { obj->Load(); } @@ -636,15 +620,26 @@ private: if (!badObjects.empty()) { // Unload all the new objects we loaded - for (auto object : newLoadedObjects) + for (auto* object : newLoadedObjects) { UnloadObject(object); } throw ObjectLoadException(std::move(badObjects)); } + // Unload objects which are not in the required list. + if (objects.empty()) + { + UnloadAll(); + } + else + { + UnloadObjectsExcept(objects); + } + + _loadedObjects = std::move(objects); + log_verbose("%u / %u new objects loaded", newLoadedObjects.size(), requiredObjects.size()); - return objects; } Object* GetOrLoadObject(const ObjectRepositoryItem* ori) @@ -677,23 +672,23 @@ private: } // Build object lists - auto maxRideObjects = static_cast(object_entry_group_counts[EnumValue(ObjectType::Ride)]); + const auto maxRideObjects = static_cast(object_entry_group_counts[EnumValue(ObjectType::Ride)]); for (size_t i = 0; i < maxRideObjects; i++) { - auto rideObject = static_cast(GetLoadedObject(ObjectType::Ride, i)); - if (rideObject != nullptr) + auto* rideObject = static_cast(GetLoadedObject(ObjectType::Ride, i)); + if (rideObject == nullptr) + continue; + + const auto* entry = static_cast(rideObject->GetLegacyData()); + if (entry == nullptr) + continue; + + for (auto rideType : entry->ride_type) { - const auto entry = static_cast(rideObject->GetLegacyData()); - if (entry != nullptr) + if (rideType < _rideTypeToObjectMap.size()) { - for (auto rideType : entry->ride_type) - { - if (rideType < _rideTypeToObjectMap.size()) - { - auto& v = _rideTypeToObjectMap[rideType]; - v.push_back(static_cast(i)); - } - } + auto& v = _rideTypeToObjectMap[rideType]; + v.push_back(static_cast(i)); } } }