From 87d7c334002997ea54a10670009b0a8a49b0a436 Mon Sep 17 00:00:00 2001 From: Jonas Karlsson Date: Tue, 16 Jan 2024 11:23:23 +0100 Subject: Fix onBodyContact being called on removed nodes Since collision callbacks happen in the physx simulation thread we need to store these callbacks. Otherwise, if an object is deleted in the same frame a 'onBodyContact' signal is enqueued and a crash will happen. Therefore we save these contact callbacks and run them at the end of the physics frame when we know if the objects are deleted or not. We move the mutex in QPhysicsWorld::deregisterNode to cover also the decoupling of the physics node since this node can be read asynchronously in the 'onContact' physx callback. In the 'onContact' callback we check if the node is removed _before_ dereferencing it and checking 'm_backendObject'. Fixes: QTBUG-121033 Pick-to: 6.7 6.6 6.5 Change-Id: I2412502baa6ede1897a4d1377e8c52a8339471d2 Reviewed-by: Andy Nichols --- src/quick3dphysics/physxnode/qphysxworld.cpp | 9 +- src/quick3dphysics/qphysicsworld.cpp | 38 +++++++- src/quick3dphysics/qphysicsworld_p.h | 15 +++ tests/auto/CMakeLists.txt | 1 + tests/auto/callback_create_delete_node/Box.qml | 21 +++++ .../callback_create_delete_node/CMakeLists.txt | 22 +++++ .../tst_callback_create_delete_node.cpp | 21 +++++ .../tst_callback_create_delete_node.qml | 104 +++++++++++++++++++++ 8 files changed, 226 insertions(+), 5 deletions(-) create mode 100644 tests/auto/callback_create_delete_node/Box.qml create mode 100644 tests/auto/callback_create_delete_node/CMakeLists.txt create mode 100644 tests/auto/callback_create_delete_node/tst_callback_create_delete_node.cpp create mode 100644 tests/auto/callback_create_delete_node/tst_callback_create_delete_node.qml diff --git a/src/quick3dphysics/physxnode/qphysxworld.cpp b/src/quick3dphysics/physxnode/qphysxworld.cpp index 28b8f4b..e1d1a81 100644 --- a/src/quick3dphysics/physxnode/qphysxworld.cpp +++ b/src/quick3dphysics/physxnode/qphysxworld.cpp @@ -91,8 +91,9 @@ public: QAbstractPhysicsNode *other = static_cast(pairHeader.actors[1]->userData); - if (!trigger || !other || !trigger->m_backendObject || !other->m_backendObject - || world->isNodeRemoved(trigger) || world->isNodeRemoved(other)) + if (!trigger || !other || world->isNodeRemoved(trigger) + || world->isNodeRemoved(other) || !trigger->m_backendObject + || !other->m_backendObject) continue; const bool triggerReceive = @@ -130,9 +131,9 @@ public: } if (triggerReceive) - trigger->registerContact(other, positions, impulses, normals); + world->registerContact(other, trigger, positions, impulses, normals); if (otherReceive) - other->registerContact(trigger, positions, impulses, normalsInverted); + world->registerContact(trigger, other, positions, impulses, normalsInverted); } } }; diff --git a/src/quick3dphysics/qphysicsworld.cpp b/src/quick3dphysics/qphysicsworld.cpp index b07ec55..0790a83 100644 --- a/src/quick3dphysics/qphysicsworld.cpp +++ b/src/quick3dphysics/qphysicsworld.cpp @@ -371,18 +371,39 @@ void QPhysicsWorld::deregisterNode(QAbstractPhysicsNode *physicsNode) { for (auto world : worldManager.worlds) { world->m_newPhysicsNodes.removeAll(physicsNode); + QMutexLocker locker(&world->m_removedPhysicsNodesMutex); if (physicsNode->m_backendObject) { Q_ASSERT(physicsNode->m_backendObject->frontendNode == physicsNode); physicsNode->m_backendObject->frontendNode = nullptr; physicsNode->m_backendObject->isRemoved = true; physicsNode->m_backendObject = nullptr; } - QMutexLocker locker(&world->m_removedPhysicsNodesMutex); world->m_removedPhysicsNodes.insert(physicsNode); } worldManager.orphanNodes.removeAll(physicsNode); } +void QPhysicsWorld::registerContact(QAbstractPhysicsNode *sender, QAbstractPhysicsNode *receiver, + const QVector &positions, + const QVector &impulses, + const QVector &normals) +{ + // Since collision callbacks happen in the physx simulation thread we need + // to store these callbacks. Otherwise, if an object is deleted in the same + // frame a 'onBodyContact' signal is enqueued and a crash will happen. + // Therefore we save these contact callbacks and run them at the end of the + // physics frame when we know if the objects are deleted or not. + + BodyContact contact; + contact.sender = sender; + contact.receiver = receiver; + contact.positions = positions; + contact.impulses = impulses; + contact.normals = normals; + + m_registeredContacts.push_back(contact); +} + QPhysicsWorld::QPhysicsWorld(QObject *parent) : QObject(parent) { m_inDesignStudio = !qEnvironmentVariableIsEmpty("QML_PUPPET_MODE"); @@ -1161,6 +1182,7 @@ void QPhysicsWorld::initPhysics() void QPhysicsWorld::frameFinished(float deltaTime) { matchOrphanNodes(); + emitContactCallbacks(); cleanupRemovedNodes(); for (auto *node : std::as_const(m_newPhysicsNodes)) { auto *body = node->createPhysXBackend(); @@ -1192,6 +1214,7 @@ void QPhysicsWorld::frameFinishedDesignStudio() { // Note sure if this is needed but do it anyway matchOrphanNodes(); + emitContactCallbacks(); cleanupRemovedNodes(); // Ignore new physics nodes, we find them from the scene node anyway m_newPhysicsNodes.clear(); @@ -1274,6 +1297,19 @@ void QPhysicsWorld::findPhysicsNodes() } } +void QPhysicsWorld::emitContactCallbacks() +{ + for (const QPhysicsWorld::BodyContact &contact : m_registeredContacts) { + if (m_removedPhysicsNodes.contains(contact.sender) + || m_removedPhysicsNodes.contains(contact.receiver)) + continue; + contact.receiver->registerContact(contact.sender, contact.positions, contact.impulses, + contact.normals); + } + + m_registeredContacts.clear(); +} + physx::PxPhysics *QPhysicsWorld::getPhysics() { return StaticPhysXObjects::getReference().physics; diff --git a/src/quick3dphysics/qphysicsworld_p.h b/src/quick3dphysics/qphysicsworld_p.h index 173a566..56e58c5 100644 --- a/src/quick3dphysics/qphysicsworld_p.h +++ b/src/quick3dphysics/qphysicsworld_p.h @@ -112,6 +112,10 @@ public: static void registerNode(QAbstractPhysicsNode *physicsNode); static void deregisterNode(QAbstractPhysicsNode *physicsNode); + void registerContact(QAbstractPhysicsNode *sender, QAbstractPhysicsNode *receiver, + const QVector &positions, const QVector &impulses, + const QVector &normals); + Q_REVISION(6, 5) QQuick3DNode *viewport() const; void setHasIndividualDebugDraw(); physx::PxControllerManager *controllerManager(); @@ -167,6 +171,16 @@ private: void disableDebugDraw(); void matchOrphanNodes(); void findPhysicsNodes(); + void emitContactCallbacks(); + + struct BodyContact + { + QAbstractPhysicsNode *sender = nullptr; + QAbstractPhysicsNode *receiver = nullptr; + QVector positions; + QVector impulses; + QVector normals; + }; struct DebugModelHolder { @@ -213,6 +227,7 @@ private: m_collisionShapeDebugModels; QSet m_removedPhysicsNodes; QMutex m_removedPhysicsNodesMutex; + QList m_registeredContacts; QVector3D m_gravity = QVector3D(0.f, -981.f, 0.f); float m_typicalLength = 100.f; // 100 cm diff --git a/tests/auto/CMakeLists.txt b/tests/auto/CMakeLists.txt index f605dc5..142cdeb 100644 --- a/tests/auto/CMakeLists.txt +++ b/tests/auto/CMakeLists.txt @@ -1,4 +1,5 @@ add_subdirectory(callback) +add_subdirectory(callback_create_delete_node) add_subdirectory(changescene) add_subdirectory(character) add_subdirectory(character_remove) diff --git a/tests/auto/callback_create_delete_node/Box.qml b/tests/auto/callback_create_delete_node/Box.qml new file mode 100644 index 0000000..a8137b3 --- /dev/null +++ b/tests/auto/callback_create_delete_node/Box.qml @@ -0,0 +1,21 @@ +// Copyright (C) 2024 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR BSD-3-Clause + +import QtQuick +import QtQuick3D +import QtQuick3D.Physics + +DynamicRigidBody { + Model { + source: "#Cube" + materials: PrincipledMaterial { + baseColor: "red" + } + } + + sendContactReports: true + receiveContactReports: true + onBodyContact: (body, positions, impulses, normals) => {} + + collisionShapes: BoxShape {} +} diff --git a/tests/auto/callback_create_delete_node/CMakeLists.txt b/tests/auto/callback_create_delete_node/CMakeLists.txt new file mode 100644 index 0000000..2cc871e --- /dev/null +++ b/tests/auto/callback_create_delete_node/CMakeLists.txt @@ -0,0 +1,22 @@ +# Copyright (C) 2024 The Qt Company Ltd. +# SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0 + +set(PROJECT_NAME "test_auto_callback_create_delete_node") + +qt_internal_add_test(${PROJECT_NAME} + GUI + QMLTEST + SOURCES + tst_callback_create_delete_node.cpp + LIBRARIES + Qt::Core + Qt::Qml + TESTDATA + tst_callback_create_delete_node.qml + Box.qml + BUILTIN_TESTDATA +) + +if(QT_BUILD_STANDALONE_TESTS) + qt_import_qml_plugins(${PROJECT_NAME}) +endif() diff --git a/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.cpp b/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.cpp new file mode 100644 index 0000000..1e4324f --- /dev/null +++ b/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.cpp @@ -0,0 +1,21 @@ +// Copyright (C) 2024 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0 + +#include +class tst_callback_create_delete_node : public QObject +{ + Q_OBJECT +private slots: + void skiptest() { QSKIP("This test will fail, skipping."); }; +}; +int main(int argc, char **argv) +{ + if (!qEnvironmentVariableIsEmpty("QEMU_LD_PREFIX")) { + qWarning("This test is unstable on QEMU, so it will be skipped."); + tst_callback_create_delete_node skip; + return QTest::qExec(&skip, argc, argv); + } + QTEST_SET_MAIN_SOURCE_PATH + return quick_test_main(argc, argv, "tst_callback_create_delete_node", QUICK_TEST_SOURCE_DIR); +} +#include "tst_callback_create_delete_node.moc" diff --git a/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.qml b/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.qml new file mode 100644 index 0000000..c43db93 --- /dev/null +++ b/tests/auto/callback_create_delete_node/tst_callback_create_delete_node.qml @@ -0,0 +1,104 @@ +// Copyright (C) 2024 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR BSD-3-Clause + +// Tests that removing and adding objects with active contact callbacks +// does not crash. QTBUG-121033 + +import QtCore +import QtTest +import QtQuick3D +import QtQuick3D.Physics +import QtQuick3D.Physics.Helpers +import QtQuick + +Item { + width: 800 + height: 600 + visible: true + + PhysicsWorld { + scene: viewport.scene + } + + View3D { + id: viewport + width: parent.width + height: parent.height + focus: true + + environment: SceneEnvironment { + antialiasingMode: SceneEnvironment.MSAA + backgroundMode: SceneEnvironment.Color + clearColor: "#f0f0f0" + } + + PerspectiveCamera { + id: camera + position: Qt.vector3d(-400, 500, 1000) + eulerRotation: Qt.vector3d(-20, -20, 0) + clipFar: 5000 + clipNear: 1 + } + + DirectionalLight { + eulerRotation: Qt.vector3d(-45, 45, 0) + } + + Node { + id: shapeSpawner + property var instancesBoxes: [] + property var boxComponent: Qt.createComponent("Box.qml") + property int numSpawns: 0 + + function createStack() { + let size = 10 + + for (var i = 0; i < 3; i++) { + for (var j = 0; j < 3; j++) { + let center = Qt.vector3d(j*100, 100*i, 0) + let box = boxComponent.incubateObject(shapeSpawner, { + "position": center, + }) + instancesBoxes.push(box) + } + } + + numSpawns = numSpawns + 1; + } + + function reset() { + // Only run method if previous stack has been created fully + for (var i = 0; i < instancesBoxes.length; i++) + if (!instancesBoxes[i].object) + return + + instancesBoxes.forEach(box => { + box.object.collisionShapes = [] + box.object.destroy() + }) + instancesBoxes = [] + + shapeSpawner.createStack() + } + } + } + + FrameAnimation { + property int frame: 0 + running: true + onTriggered: { + frame = frame + 1; + if (frame % 2 == 0) { + shapeSpawner.reset() + } + } + } + + TestCase { + name: "100 cycles" + when: shapeSpawner.numSpawns > 100 + function triggered() {} + } + +} + -- cgit v1.2.3