From 722c4acfb4e2770804576459b41d7f3c82684d84 Mon Sep 17 00:00:00 2001 From: maelstrom Date: Sun, 20 Apr 2025 18:50:27 +0200 Subject: [PATCH] fix(joints): snap crashing datamodel --- core/src/objects/base/instance.h | 3 ++ core/src/objects/datamodel.cpp | 80 +++--------------------------- core/src/objects/datamodel.h | 11 ++-- core/src/objects/jointsservice.cpp | 7 +++ core/src/objects/jointsservice.h | 5 ++ core/src/objects/snap.cpp | 66 +++++++++--------------- core/src/objects/snap.h | 8 +-- core/src/objects/workspace.cpp | 16 ++++++ editor/mainwindow.cpp | 2 +- editor/panes/propertiesview.cpp | 5 +- 10 files changed, 73 insertions(+), 130 deletions(-) diff --git a/core/src/objects/base/instance.h b/core/src/objects/base/instance.h index 34ea23a..3ff9f9c 100644 --- a/core/src/objects/base/instance.h +++ b/core/src/objects/base/instance.h @@ -41,6 +41,7 @@ struct InstanceType { typedef std::pair, std::string> _RefStatePropertyCell; class DescendantsIterator; +class Snap; // Base class for all instances in the data model // Note: enable_shared_from_this HAS to be public or else its field will not be populated @@ -58,6 +59,8 @@ private: bool ancestryContinuityCheck(std::optional> newParent); void updateAncestry(std::optional> child, std::optional> newParent); + + friend Snap; // This isn't ideal, but oh well protected: bool parentLocked = false; std::unique_ptr memberMap; diff --git a/core/src/objects/datamodel.cpp b/core/src/objects/datamodel.cpp index b245d40..d7cd6ce 100644 --- a/core/src/objects/datamodel.cpp +++ b/core/src/objects/datamodel.cpp @@ -193,83 +193,17 @@ std::shared_ptr DataModel::CloneModel() { // Clone services for (std::shared_ptr child : GetChildren()) { - // Special case: Instances parented to DataModel which are not services - if (!(child->GetClass()->flags & INSTANCE_SERVICE)) { - auto result = child->Clone(state); - if (result) - newModel->AddChild(result.value()); + auto result = child->Clone(state); + if (!result) continue; - } - DataModel::cloneService(newModel, std::dynamic_pointer_cast(child), state); - } - // Clone children of services - for (const auto& [className, service] : services) { - for (auto child : service->GetChildren()) { - auto result = child->Clone(state); - if (result) - service->AddChild(result.value()); + newModel->AddChild(result.value()); + + // Special case: Ignore instances parented to DataModel which are not services + if (child->GetClass()->flags & INSTANCE_SERVICE) { + newModel->services[child->GetClass()->className] = std::dynamic_pointer_cast(result.value()); } } return newModel; -} - -void DataModel::cloneService(std::shared_ptr target, std::shared_ptr service, RefState<_RefStatePropertyCell> state) { - // !!!!! THIS MAY CAUSE PROBLEMS !!!!! - // The way we are updating references means that their property onUpdate handler - // gets called immediately. They may expect certain invariants to be held such as workspace existing, while this - // is not the case. Currently, since this invariant is only used in the case of referring directly to workspace etc., - // this is fine, since GetService will avoid duplication problems. But here be dragons. - std::shared_ptr newInstance = target->GetService(service->GetClass()->className).expect(); - - // Copy properties - for (std::string property : service->GetProperties()) { - PropertyMeta meta = service->GetPropertyMeta(property).expect(); - - if (meta.flags & (PROP_READONLY | PROP_NOSAVE)) continue; - - // Update InstanceRef properties using map above - if (meta.type == &Data::InstanceRef::TYPE) { - std::weak_ptr refWeak = service->GetPropertyValue(property).expect().get(); - if (refWeak.expired()) continue; - - auto ref = refWeak.lock(); - auto remappedRef = state->remappedInstances[ref]; // TODO: I think this is okay? Maybe?? Add null check? - - if (remappedRef) { - // If the instance has already been remapped, set the new value - newInstance->SetPropertyValue(property, Data::InstanceRef(remappedRef)).expect(); - } else { - // Otheriise, queue this property to be updated later, and keep its current value - auto& refs = state->refsAwaitingRemap[ref]; - refs.push_back(std::make_pair(newInstance, property)); - state->refsAwaitingRemap[ref] = refs; - - newInstance->SetPropertyValue(property, Data::InstanceRef(ref)).expect(); - } - } else { - Data::Variant value = service->GetPropertyValue(property).expect(); - newInstance->SetPropertyValue(property, value).expect(); - } - } - - // Remap self - state->remappedInstances[service->shared_from_this()] = newInstance; - - // Add service prior to adding children, as children may expect service to already be parented to DataModel - target->AddChild(newInstance); - target->services[service->GetClass()->className] = std::dynamic_pointer_cast(newInstance); - - // Remap queued properties - for (std::pair, std::string> ref : state->refsAwaitingRemap[service->shared_from_this()]) { - ref.first->SetPropertyValue(ref.second, Data::InstanceRef(newInstance)).expect(); - } - - // Clone children - // for (std::shared_ptr child : service->GetChildren()) { - // std::optional> clonedChild = child->Clone(state); - // if (clonedChild) - // newInstance->AddChild(clonedChild.value()); - // } } \ No newline at end of file diff --git a/core/src/objects/datamodel.h b/core/src/objects/datamodel.h index 1511900..74987bc 100644 --- a/core/src/objects/datamodel.h +++ b/core/src/objects/datamodel.h @@ -53,17 +53,16 @@ public: result>, NoSuchService> FindService(std::string className); template - result, NoSuchService> GetService() { + std::shared_ptr GetService() { auto result = GetService(T::TYPE.className); - if (result.isError()) return result.error().value(); - return std::dynamic_pointer_cast(result.success().value()); + return std::dynamic_pointer_cast(result.expect("GetService() was called with a non-service instance type")); } template std::optional> FindService() { - auto result = FindService(T::TYPE.className); - if (result.isError()) return result.error().value(); - return std::dynamic_pointer_cast(result.success().value()); + auto result = FindService(T::TYPE.className).expect("FindService() was called with a non-service instance type"); + if (!result) return std::nullopt; + return std::dynamic_pointer_cast(result.value()); } // Saving/loading diff --git a/core/src/objects/jointsservice.cpp b/core/src/objects/jointsservice.cpp index 23c5eb6..fa3ac26 100644 --- a/core/src/objects/jointsservice.cpp +++ b/core/src/objects/jointsservice.cpp @@ -1,4 +1,5 @@ #include "jointsservice.h" +#include "workspace.h" const InstanceType JointsService::TYPE = { .super = &Instance::TYPE, @@ -21,3 +22,9 @@ void JointsService::InitService() { if (initialized) return; initialized = true; } + +std::optional> JointsService::jointWorkspace() { + if (!dataModel()) return std::nullopt; + + return dataModel().value()->FindService(); +} \ No newline at end of file diff --git a/core/src/objects/jointsservice.h b/core/src/objects/jointsservice.h index 3ccb3d4..577f245 100644 --- a/core/src/objects/jointsservice.h +++ b/core/src/objects/jointsservice.h @@ -2,7 +2,12 @@ #include "objects/base/service.h" +class Snap; class JointsService : public Service { +private: + std::optional> jointWorkspace(); + + friend Snap; protected: void InitService() override; bool initialized = false; diff --git a/core/src/objects/snap.cpp b/core/src/objects/snap.cpp index 8a3f2f9..13ce0c7 100644 --- a/core/src/objects/snap.cpp +++ b/core/src/objects/snap.cpp @@ -8,6 +8,7 @@ #include "part.h" #include #include +#include const InstanceType Snap::TYPE = { .super = &Instance::TYPE, @@ -52,66 +53,45 @@ Snap::~Snap() { } void Snap::OnAncestryChanged(std::optional>, std::optional>) { - // If the old workspace existed, and the new one differs, delete the current joint - if (this->joint && !this->oldWorkspace.expired() && (!workspace() || workspace().value() != this->oldWorkspace.lock())) { - // printf("Broke joint - Removed from workspace\n"); - oldJointWorkspace.lock()->physicsWorld->destroyJoint(this->joint); - this->joint = nullptr; - } + // Destroy and rebuild the joint, it's the simplest solution that actually works - // If the previous parent was JointsService, and now it isn't, delete the joint - if (this->joint && !oldParent.expired() && oldParent.lock()->GetClass() == &JointsService::TYPE && (!GetParent() || GetParent() != oldParent.lock())) { - // printf("Broke joint - Removed from JointsService\n"); - oldJointWorkspace.lock()->physicsWorld->destroyJoint(this->joint); - this->joint = nullptr; - } - - // If the new workspace exists, and the old one differs, create the joint - if (!this->joint && workspace() && (oldWorkspace.expired() || oldWorkspace.lock() != workspace().value())) { - // printf("Made joint - Added to workspace\n"); - buildJoint(); - } - - // If the new parent is JointsService and the previous wasn't, then create the joint - if (!this->joint && GetParent() && GetParent().value()->GetClass() == &JointsService::TYPE && (oldParent.expired() || GetParent() != oldParent.lock())) { - // printf("Made joint - Added to JointsService\n"); - buildJoint(); - } - - this->oldParent = !GetParent() ? std::weak_ptr() : GetParent().value(); - this->oldWorkspace = !workspace() ? std::weak_ptr() : workspace().value(); - this->oldJointWorkspace = !jointWorkspace() ? std::weak_ptr() : jointWorkspace().value(); + breakJoint(); + buildJoint(); } void Snap::onUpdated(std::string property) { - // We are not in the workspace, so we don't really care what values are currently set - if (!jointWorkspace()) return; - - // Workspace cannot have changed, so if the joint currently exists, it is in the present one - if (this->joint) - jointWorkspace().value()->physicsWorld->destroyJoint(this->joint); + // Destroy and rebuild the joint, if applicable + breakJoint(); buildJoint(); } void Snap::buildJoint() { - if (part0.expired() || part1.expired() || !jointWorkspace()) return; + // Only if both parts are set, are not the same part, are part of a workspace, and are part of the same workspace, we build the joint + if (part0.expired() || part1.expired() || part0.lock() == part1.lock() || !part0.lock()->workspace() || part0.lock()->workspace() != part1.lock()->workspace()) return; + + // Don't build the joint if we're not part of either a workspace or JointsService + if ((!GetParent() || GetParent().value()->GetClass() != &JointsService::TYPE) && !workspace()) return; + + std::shared_ptr workspace = part0.lock()->workspace().value(); + if (!workspace->physicsWorld) return; // Update Part1's rotation and cframe prior to creating the joint as reactphysics3d locks rotation based on how it // used to be rather than specifying an anchor rotation, so whatever. Data::CFrame newFrame = part0.lock()->cframe * (c1.Inverse() * c0); part1.lock()->cframe = newFrame; - jointWorkspace().value()->SyncPartPhysics(part1.lock()); + workspace->SyncPartPhysics(part1.lock()); rp::FixedJointInfo jointInfo(part0.lock()->rigidBody, part1.lock()->rigidBody, (c0.Inverse() * c1).Position()); - this->joint = dynamic_cast(jointWorkspace().value()->physicsWorld->createJoint(jointInfo)); + this->joint = dynamic_cast(workspace->physicsWorld->createJoint(jointInfo)); + jointWorkspace = workspace; } -std::optional> Snap::jointWorkspace() { - if (workspace()) return workspace(); +// !!! REMINDER: This has to be called manually when parts are destroyed/removed from the workspace, or joints will linger +void Snap::breakJoint() { + // If the joint doesn't exist, or its workspace expired (not our problem anymore), then no need to do anything + if (!this->joint || jointWorkspace.expired() || !jointWorkspace.lock()->physicsWorld) return; - if (GetParent() && GetParent().value()->GetClass() == &JointsService::TYPE) - return std::dynamic_pointer_cast(GetParent().value()->GetParent().value())->GetService(); - - return {}; + jointWorkspace.lock()->physicsWorld->destroyJoint(this->joint); + this->joint = nullptr; } \ No newline at end of file diff --git a/core/src/objects/snap.h b/core/src/objects/snap.h index c621009..2dd83ae 100644 --- a/core/src/objects/snap.h +++ b/core/src/objects/snap.h @@ -10,15 +10,11 @@ class Workspace; class Snap : public Instance { rp::FixedJoint* joint = nullptr; - std::weak_ptr oldParent; - // The actual workspace the joint is a part of - std::weak_ptr oldWorkspace; - // The pseudo-workspace the joint is a part of (including if parented to JointsService) - std::weak_ptr oldJointWorkspace; + // The workspace the joint was created in, if it exists + std::weak_ptr jointWorkspace; protected: void OnAncestryChanged(std::optional>, std::optional>) override; - std::optional> jointWorkspace(); void onUpdated(std::string property); void buildJoint(); void breakJoint(); diff --git a/core/src/objects/workspace.cpp b/core/src/objects/workspace.cpp index 5f31132..a743ef2 100644 --- a/core/src/objects/workspace.cpp +++ b/core/src/objects/workspace.cpp @@ -1,5 +1,7 @@ #include "workspace.h" #include "objects/base/instance.h" +#include "objects/jointsservice.h" +#include "objects/snap.h" #include "physics/util.h" #include @@ -47,6 +49,20 @@ void Workspace::InitService() { std::shared_ptr part = std::dynamic_pointer_cast(obj); this->SyncPartPhysics(part); } + + // Activate all joints + for (auto it = this->GetDescendantsStart(); it != this->GetDescendantsEnd(); it++) { + InstanceRef obj = *it; + if (obj->GetClass()->className != "Snap") continue; // TODO: Replace this with a .IsA call instead of comparing the class name directly + std::shared_ptr joint = std::dynamic_pointer_cast(obj); + joint->UpdateProperty("Part0"); + } + + for (auto obj : dataModel().value()->GetService()->GetChildren()) { + if (obj->GetClass()->className != "Snap") continue; // TODO: Replace this with a .IsA call instead of comparing the class name directly + std::shared_ptr joint = std::dynamic_pointer_cast(obj); + joint->UpdateProperty("Part0"); + } } void Workspace::SyncPartPhysics(std::shared_ptr part) { diff --git a/editor/mainwindow.cpp b/editor/mainwindow.cpp index 8c23efd..ca36d8b 100644 --- a/editor/mainwindow.cpp +++ b/editor/mainwindow.cpp @@ -177,7 +177,7 @@ MainWindow::MainWindow(QWidget *parent) snap->c1 = part0->cframe; // gWorkspace()->AddChild(snap); - gDataModel->GetService().expect()->AddChild(snap); + gDataModel->GetService()->AddChild(snap); } void MainWindow::closeEvent(QCloseEvent* evt) { diff --git a/editor/panes/propertiesview.cpp b/editor/panes/propertiesview.cpp index 383f856..c6fae3e 100644 --- a/editor/panes/propertiesview.cpp +++ b/editor/panes/propertiesview.cpp @@ -296,7 +296,7 @@ void PropertiesView::setSelected(std::optional instance) { PropertyMeta meta = inst->GetPropertyMeta(property).expect(); Data::Variant currentValue = inst->GetPropertyValue(property).expect(); - if (meta.type == &Data::CFrame::TYPE) continue; + // if (meta.type == &Data::CFrame::TYPE) continue; QTreeWidgetItem* item = new QTreeWidgetItem; item->setFlags(item->flags() | Qt::ItemIsEditable | Qt::ItemIsSelectable); @@ -311,6 +311,9 @@ void PropertiesView::setSelected(std::optional instance) { } else if (meta.type == &Data::Vector3::TYPE) { Data::Vector3 vector = currentValue.get(); item->setData(1, Qt::DisplayRole, QString::fromStdString(currentValue.ToString())); + } else if (meta.type == &Data::CFrame::TYPE) { + Data::Vector3 vector = currentValue.get().Position(); + item->setData(1, Qt::DisplayRole, QString::fromStdString(currentValue.ToString())); } else { item->setData(1, Qt::DisplayRole, QString::fromStdString(currentValue.ToString())); }