From 4d4b97fe5947631b904df7aca8ec1a056d1fdfd6 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 26 Jun 2015 22:30:06 -0700 Subject: [PATCH] add priority promotion to reduce volunteer races also fix priority inheritance from chained collisions --- libraries/entities/src/EntityItem.cpp | 36 +++++-------------- .../entities/src/EntityItemProperties.cpp | 2 -- libraries/entities/src/EntityItemProperties.h | 1 + libraries/entities/src/EntityPropertyFlags.h | 2 +- .../entities/src/EntityScriptingInterface.cpp | 8 +++-- libraries/entities/src/EntityTree.cpp | 13 ++++--- libraries/entities/src/SimulationOwner.cpp | 2 -- libraries/octree/src/OctreePacketData.h | 2 -- libraries/physics/src/EntityMotionState.cpp | 6 ++-- 9 files changed, 26 insertions(+), 46 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 09cdb0a776..587b822013 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -349,12 +349,6 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef return 0; } - // if this bitstream indicates that this node is the simulation owner, ignore any physics-related updates. - glm::vec3 savePosition = getPosition(); - glm::quat saveRotation = getRotation(); - glm::vec3 saveVelocity = _velocity; - glm::vec3 saveAngularVelocity = _angularVelocity; - int originalLength = bytesLeftToRead; // TODO: figure out a way to avoid the big deep copy below. QByteArray originalDataBuffer((const char*)data, originalLength); // big deep copy! @@ -550,17 +544,14 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef dataAt += bytes; bytesRead += bytes; - if (overwriteLocalData) { - if (_simulationOwner.set(newSimOwner)) { - _dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID; - } + if (_simulationOwner.set(newSimOwner)) { + _dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID; } } - { + { // When we own the simulation we don't accept updates to the entity's transform/velocities + // but since we're using macros below we have to temporarily modify overwriteLocalData. auto nodeList = DependencyManager::get(); bool weOwnIt = _simulationOwner.matchesValidID(nodeList->getSessionUUID()); - // When we own the simulation we don't accept updates to the entity's transform/velocities - // but since we're using macros below we have to temporarily modify overwriteLocalData. bool oldOverwrite = overwriteLocalData; overwriteLocalData = overwriteLocalData && !weOwnIt; READ_ENTITY_PROPERTY(PROP_POSITION, glm::vec3, updatePosition); @@ -665,15 +656,8 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef auto nodeList = DependencyManager::get(); const QUuid& myNodeID = nodeList->getSessionUUID(); if (overwriteLocalData) { - if (_simulationOwner.matchesValidID(myNodeID)) { - // we own the simulation, so we keep our transform+velocities and remove any related dirty flags - // rather than accept the values in the packet - setPosition(savePosition); - setRotation(saveRotation); - _velocity = saveVelocity; - _angularVelocity = saveAngularVelocity; - _dirtyFlags &= ~(EntityItem::DIRTY_TRANSFORM | EntityItem::DIRTY_VELOCITIES); - } else { + if (!_simulationOwner.matchesValidID(myNodeID)) { + _lastSimulated = now; } } @@ -988,6 +972,7 @@ bool EntityItem::setProperties(const EntityItemProperties& properties) { bool somethingChanged = false; // these affect TerseUpdate properties + SET_ENTITY_PROPERTY_FROM_PROPERTIES(simulationOwner, setSimulationOwner); SET_ENTITY_PROPERTY_FROM_PROPERTIES(position, updatePosition); SET_ENTITY_PROPERTY_FROM_PROPERTIES(rotation, updateRotation); SET_ENTITY_PROPERTY_FROM_PROPERTIES(velocity, updateVelocity); @@ -1009,7 +994,6 @@ bool EntityItem::setProperties(const EntityItemProperties& properties) { SET_ENTITY_PROPERTY_FROM_PROPERTIES(collisionsWillMove, updateCollisionsWillMove); SET_ENTITY_PROPERTY_FROM_PROPERTIES(created, updateCreated); SET_ENTITY_PROPERTY_FROM_PROPERTIES(lifetime, updateLifetime); - SET_ENTITY_PROPERTY_FROM_PROPERTIES(simulationOwner, setSimulationOwner); // non-simulation properties below SET_ENTITY_PROPERTY_FROM_PROPERTIES(script, setScript); @@ -1392,10 +1376,6 @@ void EntityItem::setSimulationOwner(const SimulationOwner& owner) { _simulationOwner.set(owner); } -void EntityItem::promoteSimulationPriority(quint8 priority) { - _simulationOwner.promotePriority(priority); -} - void EntityItem::updateSimulatorID(const QUuid& value) { if (_simulationOwner.setID(value)) { _dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID; @@ -1404,7 +1384,7 @@ void EntityItem::updateSimulatorID(const QUuid& value) { void EntityItem::clearSimulationOwnership() { _simulationOwner.clear(); - // don't bother setting the DIRTY_SIMULATOR_ID flag because clearSimulatorOwnership() + // 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 //_dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID; diff --git a/libraries/entities/src/EntityItemProperties.cpp b/libraries/entities/src/EntityItemProperties.cpp index 1de875be92..ae37d61e5f 100644 --- a/libraries/entities/src/EntityItemProperties.cpp +++ b/libraries/entities/src/EntityItemProperties.cpp @@ -706,7 +706,6 @@ bool EntityItemProperties::encodeEntityEditPacket(PacketType command, EntityItem // PROP_PAGED_PROPERTY, // PROP_CUSTOM_PROPERTIES_INCLUDED, - // adebug TODO: convert this to use APPEND_ENTITY_PROPERTY(P,V) macro? if (requestedProperties.getHasProperty(PROP_SIMULATION_OWNER)) { LevelDetails propertyLevel = packetData->startLevel(); successPropertyFits = packetData->appendValue(properties._simulationOwner.toByteArray()); @@ -977,7 +976,6 @@ bool EntityItemProperties::decodeEntityEditPacket(const unsigned char* data, int dataAt += propertyFlags.getEncodedLength(); processedBytes += propertyFlags.getEncodedLength(); - // adebug TODO: convert this to use READ_ENTITY_PROPERTY_TO_PROPERTIES macro? if (propertyFlags.getHasProperty(PROP_SIMULATION_OWNER)) { QByteArray fromBuffer; int bytes = OctreePacketData::unpackDataFromBytes(dataAt, fromBuffer); diff --git a/libraries/entities/src/EntityItemProperties.h b/libraries/entities/src/EntityItemProperties.h index f84e897331..32190664e4 100644 --- a/libraries/entities/src/EntityItemProperties.h +++ b/libraries/entities/src/EntityItemProperties.h @@ -210,6 +210,7 @@ public: void clearSimulationOwner(); void setSimulationOwner(const QUuid& id, uint8_t priority); + void promoteSimulationPriority(quint8 priority) { _simulationOwner.promotePriority(priority); } private: QUuid _id; diff --git a/libraries/entities/src/EntityPropertyFlags.h b/libraries/entities/src/EntityPropertyFlags.h index 163870c225..be4e818a4d 100644 --- a/libraries/entities/src/EntityPropertyFlags.h +++ b/libraries/entities/src/EntityPropertyFlags.h @@ -121,10 +121,10 @@ enum EntityPropertyList { // used by hyperlinks PROP_HREF, PROP_DESCRIPTION, - PROP_SIMULATION_OWNER, PROP_FACE_CAMERA, PROP_SCRIPT_TIMESTAMP, + PROP_SIMULATION_OWNER, //////////////////////////////////////////////////////////////////////////////////////////////////// // ATTENTION: add new properties ABOVE this line diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 8fdbe479e6..c3ba03315a 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -161,9 +161,11 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, EntityItemProperties proper // proxy toward the true physical position" feature to hide the final glitches in the remote watcher's // simulation. - // we re-assert our simulation ownership - properties.setSimulationOwner(myNodeID, - glm::max(entity->getSimulationPriority(), SCRIPT_EDIT_SIMULATION_PRIORITY)); + if (entity->getSimulationPriority() < SCRIPT_EDIT_SIMULATION_PRIORITY) { + // we re-assert our simulation ownership at a higher priority + properties.setSimulationOwner(myNodeID, + glm::max(entity->getSimulationPriority(), SCRIPT_EDIT_SIMULATION_PRIORITY)); + } } else { // we make a bid for simulation ownership properties.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY); diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 3f38e18a9e..3d5878784a 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -162,7 +162,12 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI // else: We assume the sender really did believe it was the simulation owner when it sent } else if (submittedID == senderID) { // the sender is trying to take or continue ownership - if (entity->getSimulatorID().isNull() || entity->getSimulatorID() == senderID) { + if (entity->getSimulatorID().isNull()) { + // the sender it taking ownership + properties.promoteSimulationPriority(RECRUIT_SIMULATION_PRIORITY); + simulationBlocked = false; + } else if (entity->getSimulatorID() == senderID) { + // the sender is asserting ownership simulationBlocked = false; } else { // the sender is trying to steal ownership from another simulator @@ -180,6 +185,8 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI // the entire update is suspect --> ignore it return false; } + } else { + simulationBlocked = senderID != entity->getSimulatorID(); } if (simulationBlocked) { // squash ownership and physics-related changes. @@ -208,10 +215,6 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI _simulation->lock(); _simulation->changeEntity(entity); _simulation->unlock(); - // always promote volunteer priority - if (entity->getSimulationPriority() == VOLUNTEER_SIMULATION_PRIORITY) { - entity->promoteSimulationPriority(RECRUIT_SIMULATION_PRIORITY); - } } } else { // normally the _simulation clears ALL updateFlags, but since there is none we do it explicitly diff --git a/libraries/entities/src/SimulationOwner.cpp b/libraries/entities/src/SimulationOwner.cpp index eb5f8b6ecd..d6957873e2 100644 --- a/libraries/entities/src/SimulationOwner.cpp +++ b/libraries/entities/src/SimulationOwner.cpp @@ -67,9 +67,7 @@ bool SimulationOwner::setID(const QUuid& id) { _id = id; updateExpiry(); if (_id.isNull()) { - // when id is null we clear everything _priority = 0; - _expiry = 0; } return true; } diff --git a/libraries/octree/src/OctreePacketData.h b/libraries/octree/src/OctreePacketData.h index 20d3db72c8..0ef5039fb0 100644 --- a/libraries/octree/src/OctreePacketData.h +++ b/libraries/octree/src/OctreePacketData.h @@ -249,8 +249,6 @@ public: static int unpackDataFromBytes(const unsigned char* dataBytes, QVector& result); static int unpackDataFromBytes(const unsigned char* dataBytes, QByteArray& result); - QByteArray getRawData() const { return QByteArray((const char*)_uncompressed, _bytesInUse); } // adebug - private: /// appends raw bytes, might fail if byte would cause packet to be too large bool append(const unsigned char* data, int length); diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index 884eccb609..b690e4f40a 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -118,7 +118,7 @@ void EntityMotionState::handleEasyChanges(uint32_t flags, PhysicsEngine* engine) } } if (flags & EntityItem::DIRTY_SIMULATOR_OWNERSHIP) { - // also known as "bid for ownership with SCRIPT priority" + // (DIRTY_SIMULATOR_OWNERSHIP really means "we should bid for ownership with SCRIPT priority") // we're manipulating this object directly via script, so we artificially // manipulate the logic to trigger an immediate bid for ownership setOutgoingPriority(SCRIPT_EDIT_SIMULATION_PRIORITY); @@ -449,6 +449,7 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, const Q // we own the simulation but the entity has stopped, so we tell the server that we're clearing simulatorID // but we remember that we do still own it... and rely on the server to tell us that we don't properties.clearSimulationOwner(); + _outgoingPriority = 0; } // else the ownership is not changing so we don't bother to pack it } else { @@ -512,8 +513,7 @@ QUuid EntityMotionState::getSimulatorID() const { // virtual void EntityMotionState::bump(quint8 priority) { if (_entity) { - quint8 inheritedPriority = VOLUNTEER_SIMULATION_PRIORITY; - setOutgoingPriority(inheritedPriority); + setOutgoingPriority(glm::max(VOLUNTEER_SIMULATION_PRIORITY, --priority)); } }