From ba226b9cb4ada063cbfd1d2b4a8fc9f26f878854 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Tue, 3 May 2016 13:40:05 -0700 Subject: [PATCH 1/4] promote priority immediately when already owned --- libraries/entities/src/EntityItem.cpp | 21 ++++++++++++++++++++- libraries/entities/src/EntityItem.h | 4 ++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index dc017f81e6..efe01d3d82 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -655,7 +655,10 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef if (wantTerseEditLogging() && _simulationOwner != newSimOwner) { qCDebug(entities) << "sim ownership for" << getDebugName() << "is now" << newSimOwner; } - if (_simulationOwner.set(newSimOwner)) { + if (weOwnSimulation && newSimOwner.getID().isNull() && _simulationOwner.getPriority() != 0) { + // entity-server is trying to clear our ownership, probably at our own request, but we've + // changed our minds since we sent it, therefore we ignore the ownerhip part of this update. + } else if (_simulationOwner.set(newSimOwner)) { _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; somethingChanged = true; // recompute weOwnSimulation so that if this is the packet that tells use we are the owner, @@ -1112,6 +1115,22 @@ void EntityItem::getAllTerseUpdateProperties(EntityItemProperties& properties) c properties._accelerationChanged = true; } +void EntityItem::pokeSimulationOwnership() { + _dirtyFlags |= Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_POKE; + auto nodeList = DependencyManager::get(); + if (_simulationOwner.matchesValidID(nodeList->getSessionUUID())) { + _simulationOwner.promotePriority(SCRIPT_POKE_SIMULATION_PRIORITY); + } +} + +void EntityItem::grabSimulationOwnership() { + _dirtyFlags |= Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_GRAB; + auto nodeList = DependencyManager::get(); + if (_simulationOwner.matchesValidID(nodeList->getSessionUUID())) { + _simulationOwner.promotePriority(SCRIPT_POKE_SIMULATION_PRIORITY); + } +} + bool EntityItem::setProperties(const EntityItemProperties& properties) { bool somethingChanged = false; diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 742ab337d0..661b25bbe8 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -373,8 +373,8 @@ public: void getAllTerseUpdateProperties(EntityItemProperties& properties) const; - void pokeSimulationOwnership() { _dirtyFlags |= Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_POKE; } - void grabSimulationOwnership() { _dirtyFlags |= Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_GRAB; } + void pokeSimulationOwnership(); + void grabSimulationOwnership(); void flagForMotionStateChange() { _dirtyFlags |= Simulation::DIRTY_MOTION_TYPE; } bool addAction(EntitySimulation* simulation, EntityActionPointer action); From 2c9608489f70ec36bdafc86c10ecc051cefc398d Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Wed, 4 May 2016 10:01:20 -0700 Subject: [PATCH 2/4] fix recursive mutex in Entites.callEntityMethod() --- libraries/entities/src/EntityScriptingInterface.cpp | 4 ++-- libraries/entities/src/EntityScriptingInterface.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 9a4539ea9f..093fa73ace 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -415,12 +415,12 @@ void EntityScriptingInterface::deleteEntity(QUuid id) { } void EntityScriptingInterface::setEntitiesScriptEngine(EntitiesScriptEngineProvider* engine) { - std::lock_guard lock(_entitiesScriptEngineLock); + std::lock_guard lock(_entitiesScriptEngineLock); _entitiesScriptEngine = engine; } void EntityScriptingInterface::callEntityMethod(QUuid id, const QString& method, const QStringList& params) { - std::lock_guard lock(_entitiesScriptEngineLock); + std::lock_guard lock(_entitiesScriptEngineLock); if (_entitiesScriptEngine) { EntityItemID entityID{ id }; _entitiesScriptEngine->callEntityScriptMethod(entityID, method, params); diff --git a/libraries/entities/src/EntityScriptingInterface.h b/libraries/entities/src/EntityScriptingInterface.h index ca6e6fd9ac..e5f913dbf8 100644 --- a/libraries/entities/src/EntityScriptingInterface.h +++ b/libraries/entities/src/EntityScriptingInterface.h @@ -215,7 +215,7 @@ private: EntityTreePointer _entityTree; - std::mutex _entitiesScriptEngineLock; + std::recursive_mutex _entitiesScriptEngineLock; EntitiesScriptEngineProvider* _entitiesScriptEngine { nullptr }; bool _bidOnSimulationOwnership { false }; From be9a572fb0fca6944600ca976eb5fc12a9f382af Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 4 May 2016 16:50:42 -0700 Subject: [PATCH 3/4] fix grab motion glitch on long-ping connections --- libraries/entities/src/EntityItem.cpp | 41 +++++++++++++++--- libraries/entities/src/EntityItem.h | 1 + libraries/entities/src/SimulationOwner.cpp | 48 +++++++++++++++++++-- libraries/entities/src/SimulationOwner.h | 21 ++++++--- libraries/physics/src/EntityMotionState.cpp | 8 +++- 5 files changed, 101 insertions(+), 18 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index efe01d3d82..bdedbbce77 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -655,14 +655,32 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef if (wantTerseEditLogging() && _simulationOwner != newSimOwner) { qCDebug(entities) << "sim ownership for" << getDebugName() << "is now" << newSimOwner; } - if (weOwnSimulation && newSimOwner.getID().isNull() && _simulationOwner.getPriority() != 0) { - // entity-server is trying to clear our ownership, probably at our own request, but we've - // changed our minds since we sent it, therefore we ignore the ownerhip part of this update. + if (weOwnSimulation) { + if (newSimOwner.getID().isNull() && !_simulationOwner.pendingRelease(lastEditedFromBufferAdjusted)) { + // entity-server is trying to clear our ownership (probably at our own request) + // but we actually want to own it, therefore we ignore this clear event + // and pretend that we own it (we assume we'll recover it soon) + } else if (_simulationOwner.set(newSimOwner)) { + _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; + somethingChanged = true; + // recompute weOwnSimulation for later + weOwnSimulation = _simulationOwner.matchesValidID(myNodeID); + } + } else if (newSimOwner.getID().isNull() && _simulationOwner.pendingTake(lastEditedFromBufferAdjusted)) { + // entity-server is trying to clear someone else's ownership + // but we want to own it, therefore we ignore this clear event + // and pretend that we own it (we assume we'll get it soon) + weOwnSimulation = true; + if (!_simulationOwner.isNull()) { + // someone else really did own it + _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; + somethingChanged = true; + _simulationOwner.clearCurrentOwner(); + } } else if (_simulationOwner.set(newSimOwner)) { _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; somethingChanged = true; - // recompute weOwnSimulation so that if this is the packet that tells use we are the owner, - // we ignore the physics changes from this packet. + // recompute weOwnSimulation for later weOwnSimulation = _simulationOwner.matchesValidID(myNodeID); } } @@ -1119,7 +1137,11 @@ void EntityItem::pokeSimulationOwnership() { _dirtyFlags |= Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_POKE; auto nodeList = DependencyManager::get(); if (_simulationOwner.matchesValidID(nodeList->getSessionUUID())) { + // we already own it _simulationOwner.promotePriority(SCRIPT_POKE_SIMULATION_PRIORITY); + } else { + // we don't own it yet + _simulationOwner.setPendingPriority(SCRIPT_POKE_SIMULATION_PRIORITY, usecTimestampNow()); } } @@ -1127,7 +1149,11 @@ void EntityItem::grabSimulationOwnership() { _dirtyFlags |= Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_GRAB; auto nodeList = DependencyManager::get(); if (_simulationOwner.matchesValidID(nodeList->getSessionUUID())) { + // we already own it _simulationOwner.promotePriority(SCRIPT_POKE_SIMULATION_PRIORITY); + } else { + // we don't own it yet + _simulationOwner.setPendingPriority(SCRIPT_GRAB_SIMULATION_PRIORITY, usecTimestampNow()); } } @@ -1665,11 +1691,14 @@ void EntityItem::clearSimulationOwnership() { _simulationOwner.clear(); // don't bother setting the DIRTY_SIMULATOR_ID flag because clearSimulationOwnership() - // is only ever called entity-server-side and the flags are only used client-side + // is only ever called on the entity-server and the flags are only used client-side //_dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; } +void EntityItem::setPendingOwnershipPriority(quint8 priority, const quint64& timestamp) { + _simulationOwner.setPendingPriority(priority, timestamp); +} bool EntityItem::addAction(EntitySimulation* simulation, EntityActionPointer action) { bool result; diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 661b25bbe8..ecb9800e70 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -302,6 +302,7 @@ public: QUuid getSimulatorID() const { return _simulationOwner.getID(); } void updateSimulationOwner(const SimulationOwner& owner); void clearSimulationOwnership(); + void setPendingOwnershipPriority(quint8 priority, const quint64& timestamp); const QString& getMarketplaceID() const { return _marketplaceID; } void setMarketplaceID(const QString& value) { _marketplaceID = value; } diff --git a/libraries/entities/src/SimulationOwner.cpp b/libraries/entities/src/SimulationOwner.cpp index 2db9f2a122..43bece93ae 100644 --- a/libraries/entities/src/SimulationOwner.cpp +++ b/libraries/entities/src/SimulationOwner.cpp @@ -16,12 +16,30 @@ #include +const quint8 PENDING_STATE_NOTHING = 0; +const quint8 PENDING_STATE_TAKE = 1; +const quint8 PENDING_STATE_RELEASE = 2; + // static const int SimulationOwner::NUM_BYTES_ENCODED = NUM_BYTES_RFC4122_UUID + 1; +SimulationOwner::SimulationOwner() : + _id(), + _expiry(0), + _pendingTimestamp(0), + _priority(0), + _pendingPriority(0), + _pendingState(PENDING_STATE_NOTHING) +{ +} -SimulationOwner::SimulationOwner(const SimulationOwner& other) - : _id(other._id), _priority(other._priority), _expiry(other._expiry) { +SimulationOwner::SimulationOwner(const QUuid& id, quint8 priority) : + _id(id), + _expiry(0), + _pendingTimestamp(0), + _priority(priority), + _pendingPriority(0) +{ } QByteArray SimulationOwner::toByteArray() const { @@ -42,8 +60,11 @@ bool SimulationOwner::fromByteArray(const QByteArray& data) { void SimulationOwner::clear() { _id = QUuid(); - _priority = 0; _expiry = 0; + _pendingTimestamp = 0; + _priority = 0; + _pendingPriority = 0; + _pendingState = PENDING_STATE_NOTHING; } void SimulationOwner::setPriority(quint8 priority) { @@ -53,7 +74,6 @@ void SimulationOwner::setPriority(quint8 priority) { void SimulationOwner::promotePriority(quint8 priority) { if (priority > _priority) { _priority = priority; - updateExpiry(); } } @@ -81,11 +101,31 @@ bool SimulationOwner::set(const SimulationOwner& owner) { return setID(owner._id) || oldPriority != _priority; } +void SimulationOwner::setPendingPriority(quint8 priority, const quint64& timestamp) { + _pendingPriority = priority; + _pendingTimestamp = timestamp; + _pendingState = (_pendingPriority == 0) ? PENDING_STATE_RELEASE : PENDING_STATE_TAKE; +} + void SimulationOwner::updateExpiry() { const quint64 OWNERSHIP_LOCKOUT_EXPIRY = USECS_PER_SECOND / 5; _expiry = usecTimestampNow() + OWNERSHIP_LOCKOUT_EXPIRY; } +bool SimulationOwner::pendingRelease(const quint64& timestamp) { + return _pendingPriority == 0 && _pendingState == PENDING_STATE_RELEASE && _pendingTimestamp > timestamp; +} + +bool SimulationOwner::pendingTake(const quint64& timestamp) { + return _pendingPriority > 0 && _pendingState == PENDING_STATE_TAKE && _pendingTimestamp > timestamp; +} + +void SimulationOwner::clearCurrentOwner() { + _id = QUuid(); + _expiry = 0; + _priority = 0; +} + // NOTE: eventually this code will be moved into unit tests // static debug void SimulationOwner::test() { diff --git a/libraries/entities/src/SimulationOwner.h b/libraries/entities/src/SimulationOwner.h index 242d0eb272..1afec426d7 100644 --- a/libraries/entities/src/SimulationOwner.h +++ b/libraries/entities/src/SimulationOwner.h @@ -37,13 +37,12 @@ class SimulationOwner { public: static const int NUM_BYTES_ENCODED; - SimulationOwner() : _id(), _priority(0), _expiry(0) {} - SimulationOwner(const QUuid& id, quint8 priority) : _id(id), _priority(priority), _expiry(0) {} - SimulationOwner(const SimulationOwner& other); + SimulationOwner(); + SimulationOwner(const QUuid& id, quint8 priority); const QUuid& getID() const { return _id; } - quint8 getPriority() const { return _priority; } const quint64& getExpiry() const { return _expiry; } + quint8 getPriority() const { return _priority; } QByteArray toByteArray() const; bool fromByteArray(const QByteArray& data); @@ -57,6 +56,7 @@ public: bool setID(const QUuid& id); bool set(const QUuid& id, quint8 priority); bool set(const SimulationOwner& owner); + void setPendingPriority(quint8 priority, const quint64& timestamp); bool isNull() const { return _id.isNull(); } bool matchesValidID(const QUuid& id) const { return _id == id && !_id.isNull(); } @@ -65,6 +65,10 @@ public: bool hasExpired() const { return usecTimestampNow() > _expiry; } + bool pendingRelease(const quint64& timestamp); // return true if valid pending RELEASE + bool pendingTake(const quint64& timestamp); // return true if valid pending TAKE + void clearCurrentOwner(); + bool operator>=(quint8 priority) const { return _priority >= priority; } bool operator==(const SimulationOwner& other) { return (_id == other._id && _priority == other._priority); } @@ -77,9 +81,12 @@ public: static void test(); private: - QUuid _id; - quint8 _priority; - quint64 _expiry; + QUuid _id; // owner + quint64 _expiry; // time when ownership can transition at equal priority + quint64 _pendingTimestamp; // time when pending update was set + quint8 _priority; // priority of current owner + quint8 _pendingPriority; // priority of pendingTake + quint8 _pendingState; // NOTHING, TAKE, or RELEASE }; diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index fbec7cd3e0..db76b1727c 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -519,12 +519,17 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_ // but we remember we do still own it... and rely on the server to tell us we don't properties.clearSimulationOwner(); _outgoingPriority = 0; + _entity->setPendingOwnershipPriority(_outgoingPriority, now); } else if (Physics::getSessionUUID() != _entity->getSimulatorID()) { // we don't own the simulation for this entity yet, but we're sending a bid for it + quint8 bidPriority = glm::max(_outgoingPriority, VOLUNTEER_SIMULATION_PRIORITY); properties.setSimulationOwner(Physics::getSessionUUID(), glm::max(_outgoingPriority, VOLUNTEER_SIMULATION_PRIORITY)); _nextOwnershipBid = now + USECS_BETWEEN_OWNERSHIP_BIDS; - _outgoingPriority = 0; // reset outgoing priority whenever we bid + // copy _outgoingPriority into pendingPriority... + _entity->setPendingOwnershipPriority(_outgoingPriority, now); + // ...then reset _outgoingPriority in preparation for the next frame + _outgoingPriority = 0; } else if (_outgoingPriority != _entity->getSimulationPriority()) { // we own the simulation but our desired priority has changed if (_outgoingPriority == 0) { @@ -534,6 +539,7 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_ // we just need to change the priority properties.setSimulationOwner(Physics::getSessionUUID(), _outgoingPriority); } + _entity->setPendingOwnershipPriority(_outgoingPriority, now); } EntityItemID id(_entity->getID()); From 3202a723b8e4d091ce425d0da685bee5569cefe8 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 5 May 2016 08:54:09 -0700 Subject: [PATCH 4/4] remove warning about unused variable --- libraries/physics/src/EntityMotionState.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index db76b1727c..f0539110d3 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -523,8 +523,7 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_ } else if (Physics::getSessionUUID() != _entity->getSimulatorID()) { // we don't own the simulation for this entity yet, but we're sending a bid for it quint8 bidPriority = glm::max(_outgoingPriority, VOLUNTEER_SIMULATION_PRIORITY); - properties.setSimulationOwner(Physics::getSessionUUID(), - glm::max(_outgoingPriority, VOLUNTEER_SIMULATION_PRIORITY)); + properties.setSimulationOwner(Physics::getSessionUUID(), bidPriority); _nextOwnershipBid = now + USECS_BETWEEN_OWNERSHIP_BIDS; // copy _outgoingPriority into pendingPriority... _entity->setPendingOwnershipPriority(_outgoingPriority, now);