From f63610af2b8d7549d55a3c7e3f6afe2b623c8d57 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 26 Sep 2018 15:07:29 -0700 Subject: [PATCH 1/8] namechange: TerseUpdate --> TransformAndVelocities --- libraries/entities/src/EntityItem.cpp | 7 +++---- libraries/entities/src/EntityItem.h | 2 +- libraries/entities/src/EntityItemProperties.cpp | 9 ++++++--- libraries/entities/src/EntityItemProperties.h | 2 +- .../entities/src/EntityScriptingInterface.cpp | 14 +++++++------- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 6279f0bae0..48a1edeea2 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -673,7 +673,7 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef const QUuid& myNodeID = nodeList->getSessionUUID(); bool weOwnSimulation = _simulationOwner.matchesValidID(myNodeID); - // pack SimulationOwner and terse update properties near each other + // pack SimulationOwner, transform, and velocity properties near each other // NOTE: the server is authoritative for changes to simOwnerID so we always unpack ownership data // even when we would otherwise ignore the rest of the packet. @@ -1358,8 +1358,7 @@ EntityItemProperties EntityItem::getProperties(const EntityPropertyFlags& desire return properties; } -void EntityItem::getAllTerseUpdateProperties(EntityItemProperties& properties) const { - // a TerseUpdate includes the transform and its derivatives +void EntityItem::getTransformAndVelocityProperties(EntityItemProperties& properties) const { if (!properties._positionChanged) { properties._position = getLocalPosition(); } @@ -1419,7 +1418,7 @@ bool EntityItem::stillWaitingToTakeOwnership(uint64_t timestamp) const { bool EntityItem::setProperties(const EntityItemProperties& properties) { bool somethingChanged = false; - // these affect TerseUpdate properties + // these affect transform and velocity properties SET_ENTITY_PROPERTY_FROM_PROPERTIES(simulationOwner, setSimulationOwner); SET_ENTITY_PROPERTY_FROM_PROPERTIES(position, setPosition); SET_ENTITY_PROPERTY_FROM_PROPERTIES(rotation, setRotation); diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index e4063eac3b..d4acc10d9b 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -420,7 +420,7 @@ public: quint64 getLastEditedFromRemote() const { return _lastEditedFromRemote; } void updateLastEditedFromRemote() { _lastEditedFromRemote = usecTimestampNow(); } - void getAllTerseUpdateProperties(EntityItemProperties& properties) const; + void getTransformAndVelocityProperties(EntityItemProperties& properties) const; void flagForMotionStateChange() { _flags |= Simulation::DIRTY_MOTION_TYPE; } diff --git a/libraries/entities/src/EntityItemProperties.cpp b/libraries/entities/src/EntityItemProperties.cpp index dd5c2020c8..414380afc4 100644 --- a/libraries/entities/src/EntityItemProperties.cpp +++ b/libraries/entities/src/EntityItemProperties.cpp @@ -3244,9 +3244,12 @@ AABox EntityItemProperties::getAABox() const { return AABox(rotatedExtentsRelativeToRegistrationPoint); } -bool EntityItemProperties::hasTerseUpdateChanges() const { - // a TerseUpdate includes the transform and its derivatives - return _positionChanged || _velocityChanged || _rotationChanged || _angularVelocityChanged || _accelerationChanged; +bool EntityItemProperties::hasTransformOrVelocityChanges() const { + return _positionChanged ||_localPositionChanged + || _rotationChanged || _localRotationChanged + || _velocityChanged || _localVelocityChanged + || _angularVelocityChanged || _localAngularVelocityChanged + || _accelerationChanged; } bool EntityItemProperties::hasMiscPhysicsChanges() const { diff --git a/libraries/entities/src/EntityItemProperties.h b/libraries/entities/src/EntityItemProperties.h index 8527b471c3..df2e5423e6 100644 --- a/libraries/entities/src/EntityItemProperties.h +++ b/libraries/entities/src/EntityItemProperties.h @@ -349,7 +349,7 @@ public: void setCreated(QDateTime& v); - bool hasTerseUpdateChanges() const; + bool hasTransformOrVelocityChanges() const; bool hasMiscPhysicsChanges() const; void clearSimulationOwner(); diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index a8fce0d5fb..d2603bad47 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -597,20 +597,20 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& entityFound = true; // make sure the properties has a type, so that the encode can know which properties to include properties.setType(entity->getType()); - bool hasTerseUpdateChanges = properties.hasTerseUpdateChanges(); - bool hasPhysicsChanges = properties.hasMiscPhysicsChanges() || hasTerseUpdateChanges; + bool hasTransformOrVelocityChanges = properties.hasTransformOrVelocityChanges(); + bool hasPhysicsChanges = properties.hasMiscPhysicsChanges() || hasTransformOrVelocityChanges; if (_bidOnSimulationOwnership && hasPhysicsChanges) { auto nodeList = DependencyManager::get(); const QUuid myNodeID = nodeList->getSessionUUID(); if (entity->getSimulatorID() == myNodeID) { - // we think we already own the simulation, so make sure to send ALL TerseUpdate properties - if (hasTerseUpdateChanges) { - entity->getAllTerseUpdateProperties(properties); + // we think we already own the simulation, so make sure to send the full transform and all velocities + if (hasTransformOrVelocityChanges) { + entity->getTransformAndVelocityProperties(properties); } - // TODO: if we knew that ONLY TerseUpdate properties have changed in properties AND the object + // TODO: if we knew that ONLY transforms and velocities have changed in properties AND the object // is dynamic AND it is active in the physics simulation then we could chose to NOT queue an update - // and instead let the physics simulation decide when to send a terse update. This would remove + // and instead let the physics simulation decide when to send the update. This would remove // the "slide-no-rotate" glitch (and typical double-update) that we see during the "poke rolling // balls" test. However, even if we solve this problem we still need to provide a "slerp the visible // proxy toward the true physical position" feature to hide the final glitches in the remote watcher's From 21654ba77c0fe24bc5f5ba2108de754ad8340694 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 26 Sep 2018 15:29:21 -0700 Subject: [PATCH 2/8] cleanup access to sessionID --- .../entities/src/EntityScriptingInterface.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index d2603bad47..a668c806cb 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -242,13 +242,12 @@ QUuid EntityScriptingInterface::addEntity(const EntityItemProperties& properties _activityTracking.addedEntityCount++; auto nodeList = DependencyManager::get(); - auto sessionID = nodeList->getSessionUUID(); + const auto sessionID = nodeList->getSessionUUID(); EntityItemProperties propertiesWithSimID = properties; if (clientOnly) { - const QUuid myNodeID = sessionID; propertiesWithSimID.setClientOnly(clientOnly); - propertiesWithSimID.setOwningAvatarID(myNodeID); + propertiesWithSimID.setOwningAvatarID(sessionID); } propertiesWithSimID.setLastEditedBy(sessionID); @@ -530,8 +529,7 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& _activityTracking.editedEntityCount++; - auto nodeList = DependencyManager::get(); - auto sessionID = nodeList->getSessionUUID(); + const auto sessionID = DependencyManager::get()->getSessionUUID(); EntityItemProperties properties = scriptSideProperties; properties.setLastEditedBy(sessionID); @@ -550,7 +548,7 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& return; } - if (entity->getClientOnly() && entity->getOwningAvatarID() != nodeList->getSessionUUID()) { + if (entity->getClientOnly() && entity->getOwningAvatarID() != sessionID) { // don't edit other avatar's avatarEntities return; } @@ -600,10 +598,7 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& bool hasTransformOrVelocityChanges = properties.hasTransformOrVelocityChanges(); bool hasPhysicsChanges = properties.hasMiscPhysicsChanges() || hasTransformOrVelocityChanges; if (_bidOnSimulationOwnership && hasPhysicsChanges) { - auto nodeList = DependencyManager::get(); - const QUuid myNodeID = nodeList->getSessionUUID(); - - if (entity->getSimulatorID() == myNodeID) { + if (entity->getSimulatorID() == sessionID) { // we think we already own the simulation, so make sure to send the full transform and all velocities if (hasTransformOrVelocityChanges) { entity->getTransformAndVelocityProperties(properties); @@ -618,11 +613,11 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& if (entity->getSimulationPriority() < SCRIPT_POKE_SIMULATION_PRIORITY) { // we re-assert our simulation ownership at a higher priority - properties.setSimulationOwner(myNodeID, SCRIPT_POKE_SIMULATION_PRIORITY); + properties.setSimulationOwner(sessionID, SCRIPT_POKE_SIMULATION_PRIORITY); } } else { // we make a bid for simulation ownership - properties.setSimulationOwner(myNodeID, SCRIPT_POKE_SIMULATION_PRIORITY); + properties.setSimulationOwner(sessionID, SCRIPT_POKE_SIMULATION_PRIORITY); entity->setScriptSimulationPriority(SCRIPT_POKE_SIMULATION_PRIORITY); } } From 0b34524ba0cef85fb73b4a9cbb83632ae329f627 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 26 Sep 2018 16:09:44 -0700 Subject: [PATCH 3/8] minimimze time holding locks on tree in editEntity() --- .../entities/src/EntityScriptingInterface.cpp | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index a668c806cb..199e9eaaf8 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -541,8 +541,7 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& } // If we have a local entity tree set, then also update it. - bool updatedEntity = false; - _entityTree->withWriteLock([&] { + _entityTree->withReadLock([&] { EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); if (!entity) { return; @@ -575,7 +574,14 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& } properties.setClientOnly(entity->getClientOnly()); properties.setOwningAvatarID(entity->getOwningAvatarID()); - properties = convertPropertiesFromScriptSemantics(properties, properties.getScalesWithParent()); + }); + properties = convertPropertiesFromScriptSemantics(properties, properties.getScalesWithParent()); + + // TODO: avoid setting properties not allowed to be changed according to simulation ownership rules + + // done reading, start write + bool updatedEntity = false; + _entityTree->withWriteLock([&] { updatedEntity = _entityTree->updateEntity(entityID, properties); }); @@ -588,6 +594,7 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& // return QUuid(); // } + // done writing, send update bool entityFound { false }; _entityTree->withReadLock([&] { EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); @@ -641,7 +648,10 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& } } }); - } else { + } + }); + if (!entityFound) { + { // Sometimes ESS don't have the entity they are trying to edit in their local tree. In this case, // convertPropertiesFromScriptSemantics doesn't get called and local* edits will get dropped. // This is because, on the script side, "position" is in world frame, but in the network @@ -663,8 +673,6 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& properties.setDimensions(properties.getLocalDimensions()); } } - }); - if (!entityFound) { // we've made an edit to an entity we don't know about, or to a non-entity. If it's a known non-entity, // print a warning and don't send an edit packet to the entity-server. QSharedPointer parentFinder = DependencyManager::get(); From bb510792847ea44877962578d5135d130a54e8be Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 26 Sep 2018 17:09:45 -0700 Subject: [PATCH 4/8] move some bid-prep logic from EntityMotionState to EntityItem --- libraries/entities/src/EntityItem.cpp | 23 +++++++++++++++++++++ libraries/entities/src/EntityItem.h | 2 ++ libraries/physics/src/EntityMotionState.cpp | 22 ++------------------ 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 48a1edeea2..c4ef74a0d7 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -3255,3 +3255,26 @@ void EntityItem::setScriptHasFinishedPreload(bool value) { bool EntityItem::isScriptPreloadFinished() { return _scriptPreloadFinished; } + +void EntityItem::prepareForSimulationOwnershipBid(EntityItemProperties& properties, uint64_t now, uint8_t priority) { + if (dynamicDataNeedsTransmit()) { + setDynamicDataNeedsTransmit(false); + properties.setActionData(getDynamicData()); + } + + if (updateQueryAACube()) { + // due to parenting, the server may not know where something is in world-space, so include the bounding cube. + properties.setQueryAACube(getQueryAACube()); + } + + // set the LastEdited of the properties but NOT the entity itself + properties.setLastEdited(now); + + clearScriptSimulationPriority(); + properties.setSimulationOwner(Physics::getSessionUUID(), priority); + setPendingOwnershipPriority(priority); + + properties.setClientOnly(getClientOnly()); + properties.setOwningAvatarID(getOwningAvatarID()); + setLastBroadcast(now); // for debug/physics status icons +} diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index d4acc10d9b..2fa60e17cb 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -535,6 +535,8 @@ public: const GrabPropertyGroup& getGrabProperties() const { return _grabProperties; } + void prepareForSimulationOwnershipBid(EntityItemProperties& properties, uint64_t now, uint8_t priority); + signals: void requestRenderUpdate(); void spaceUpdate(std::pair data); diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index 8162bf4e18..c920665279 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -502,36 +502,18 @@ void EntityMotionState::sendBid(OctreeEditPacketSender* packetSender, uint32_t s properties.setVelocity(linearVelocity); properties.setAcceleration(_entity->getAcceleration()); properties.setAngularVelocity(angularVelocity); - if (_entity->dynamicDataNeedsTransmit()) { - _entity->setDynamicDataNeedsTransmit(false); - properties.setActionData(_entity->getDynamicData()); - } - - if (_entity->updateQueryAACube()) { - // due to parenting, the server may not know where something is in world-space, so include the bounding cube. - properties.setQueryAACube(_entity->getQueryAACube()); - } - - // set the LastEdited of the properties but NOT the entity itself - quint64 now = usecTimestampNow(); - properties.setLastEdited(now); // we don't own the simulation for this entity yet, but we're sending a bid for it + quint64 now = usecTimestampNow(); uint8_t finalBidPriority = computeFinalBidPriority(); - _entity->clearScriptSimulationPriority(); - properties.setSimulationOwner(Physics::getSessionUUID(), finalBidPriority); - _entity->setPendingOwnershipPriority(finalBidPriority); + _entity->prepareForSimulationOwnershipBid(properties, now, finalBidPriority); EntityTreeElementPointer element = _entity->getElement(); EntityTreePointer tree = element ? element->getTree() : nullptr; - properties.setClientOnly(_entity->getClientOnly()); - properties.setOwningAvatarID(_entity->getOwningAvatarID()); - EntityItemID id(_entity->getID()); EntityEditPacketSender* entityPacketSender = static_cast(packetSender); entityPacketSender->queueEditEntityMessage(PacketType::EntityPhysics, tree, id, properties); - _entity->setLastBroadcast(now); // for debug/physics status icons // NOTE: we don't descend to children for ownership bid. Instead, if we win ownership of the parent // then in sendUpdate() we'll walk descendents and send updates for their QueryAACubes if necessary. From ce81ab1f9de75fd6fd85d1a7284eade6e70a7c6d Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 3 Oct 2018 14:41:21 -0700 Subject: [PATCH 5/8] don't edit restricted properties unless sim ownership possible --- .../entities/src/EntityItemProperties.cpp | 50 ++++++ libraries/entities/src/EntityItemProperties.h | 4 + .../entities/src/EntityScriptingInterface.cpp | 148 +++++++++--------- 3 files changed, 128 insertions(+), 74 deletions(-) diff --git a/libraries/entities/src/EntityItemProperties.cpp b/libraries/entities/src/EntityItemProperties.cpp index 414380afc4..6921bc0bdd 100644 --- a/libraries/entities/src/EntityItemProperties.cpp +++ b/libraries/entities/src/EntityItemProperties.cpp @@ -3258,6 +3258,56 @@ bool EntityItemProperties::hasMiscPhysicsChanges() const { _compoundShapeURLChanged || _dynamicChanged || _collisionlessChanged || _collisionMaskChanged; } +bool EntityItemProperties::hasSimulationRestrictedChanges() const { + return _positionChanged || _localPositionChanged + || _rotationChanged || _localRotationChanged + || _velocityChanged || _localVelocityChanged + || _angularVelocityChanged || _localAngularVelocityChanged + || _accelerationChanged + || _parentIDChanged || _parentJointIndexChanged; +} + +void EntityItemProperties::copySimulationRestrictedProperties(const EntityItemPointer& entity) { + if (!_parentIDChanged) { + setParentID(entity->getParentID()); + } + if (!_parentJointIndexChanged) { + setParentJointIndex(entity->getParentJointIndex()); + } + if (!_localPositionChanged && !_positionChanged) { + setPosition(entity->getWorldPosition()); + } + if (!_localRotationChanged && !_rotationChanged) { + setRotation(entity->getWorldOrientation()); + } + if (!_localVelocityChanged && !_velocityChanged) { + setVelocity(entity->getWorldVelocity()); + } + if (!_localAngularVelocityChanged && !_angularVelocityChanged) { + setAngularVelocity(entity->getWorldAngularVelocity()); + } + if (!_accelerationChanged) { + setAcceleration(entity->getAcceleration()); + } + if (!_localDimensionsChanged && !_dimensionsChanged) { + setDimensions(entity->getScaledDimensions()); + } +} + +void EntityItemProperties::clearSimulationRestrictedProperties() { + _positionChanged = false; + _localPositionChanged = false; + _rotationChanged = false; + _localRotationChanged = false; + _velocityChanged = false; + _localVelocityChanged = false; + _angularVelocityChanged = false; + _localAngularVelocityChanged = false; + _accelerationChanged = false; + _parentIDChanged = false; + _parentJointIndexChanged = false; +} + void EntityItemProperties::clearSimulationOwner() { _simulationOwner.clear(); _simulationOwnerChanged = true; diff --git a/libraries/entities/src/EntityItemProperties.h b/libraries/entities/src/EntityItemProperties.h index df2e5423e6..d351a9f092 100644 --- a/libraries/entities/src/EntityItemProperties.h +++ b/libraries/entities/src/EntityItemProperties.h @@ -352,6 +352,10 @@ public: bool hasTransformOrVelocityChanges() const; bool hasMiscPhysicsChanges() const; + bool hasSimulationRestrictedChanges() const; + void copySimulationRestrictedProperties(const EntityItemPointer& entity); + void clearSimulationRestrictedProperties(); + void clearSimulationOwner(); void setSimulationOwner(const QUuid& id, uint8_t priority); void setSimulationOwner(const QByteArray& data); diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 199e9eaaf8..e745bf0874 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -174,6 +174,7 @@ EntityItemProperties convertPropertiesToScriptSemantics(const EntityItemProperti } +// TODO: this method looks expensive and should take properties by reference, update it, and return void EntityItemProperties convertPropertiesFromScriptSemantics(const EntityItemProperties& scriptSideProperties, bool scalesWithParent) { // convert position and rotation properties from world-space to local, unless localPosition and localRotation @@ -532,54 +533,80 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& const auto sessionID = DependencyManager::get()->getSessionUUID(); EntityItemProperties properties = scriptSideProperties; - properties.setLastEditedBy(sessionID); EntityItemID entityID(id); if (!_entityTree) { + properties.setLastEditedBy(sessionID); queueEntityMessage(PacketType::EntityEdit, entityID, properties); return id; } - // If we have a local entity tree set, then also update it. + EntityItemPointer entity(nullptr); + SimulationOwner simulationOwner; _entityTree->withReadLock([&] { - EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); + // make a copy of entity for local logic outside of tree lock + entity = _entityTree->findEntityByEntityItemID(entityID); if (!entity) { return; } if (entity->getClientOnly() && entity->getOwningAvatarID() != sessionID) { // don't edit other avatar's avatarEntities + properties = EntityItemProperties(); return; } + // make a copy of simulationOwner for local logic outside of tree lock + simulationOwner = entity->getSimulationOwner(); + }); - if (scriptSideProperties.parentRelatedPropertyChanged()) { - // All of parentID, parentJointIndex, position, rotation are needed to make sense of any of them. - // If any of these changed, pull any missing properties from the entity. - - if (!scriptSideProperties.parentIDChanged()) { - properties.setParentID(entity->getParentID()); - } - if (!scriptSideProperties.parentJointIndexChanged()) { - properties.setParentJointIndex(entity->getParentJointIndex()); - } - if (!scriptSideProperties.localPositionChanged() && !scriptSideProperties.positionChanged()) { - properties.setPosition(entity->getWorldPosition()); - } - if (!scriptSideProperties.localRotationChanged() && !scriptSideProperties.rotationChanged()) { - properties.setRotation(entity->getWorldOrientation()); - } - if (!scriptSideProperties.localDimensionsChanged() && !scriptSideProperties.dimensionsChanged()) { - properties.setDimensions(entity->getScaledDimensions()); + if (entity) { + if (properties.hasSimulationRestrictedChanges()) { + if (_bidOnSimulationOwnership) { + if (simulationOwner.getID() == sessionID) { + // we own the simulation --> copy ALL restricted properties + properties.copySimulationRestrictedProperties(entity); + //if (ourBidPriority != currentBidPriority) { + // TODO: figure out how to change simulation ownership priority + //} + } else { + // we don't own the simulation + uint8_t desiredPriority = entity->getScriptSimulationPriority(); + if (desiredPriority < simulationOwner.getPriority()) { + // we don't desire to own it --> clear restricted properties + properties.clearSimulationRestrictedProperties(); + } else { + // we want to own it --> copy ALL restricted properties + properties.copySimulationRestrictedProperties(entity); + // and also store our simulation + properties.setSimulationOwner(sessionID, desiredPriority); + } + } + } else if (!simulationOwner.getID().isNull()) { + // someone owns this but not us + // clear restricted properties + properties.clearSimulationRestrictedProperties(); } + // clear the cached simulationPriority level + entity->setScriptSimulationPriority(0); } + + // set these to make EntityItemProperties::getScalesWithParent() work correctly properties.setClientOnly(entity->getClientOnly()); properties.setOwningAvatarID(entity->getOwningAvatarID()); - }); + + // make sure the properties has a type, so that the encode can know which properties to include + properties.setType(entity->getType()); + } else if (_bidOnSimulationOwnership) { + // bail when simulation participants don't know about entity + return QUuid(); + } + // TODO: it is possible there is no remaining useful changes in properties and we should bail early. + // How to check for this cheaply? + properties = convertPropertiesFromScriptSemantics(properties, properties.getScalesWithParent()); + properties.setLastEditedBy(sessionID); - // TODO: avoid setting properties not allowed to be changed according to simulation ownership rules - - // done reading, start write + // done reading and modifying properties --> start write bool updatedEntity = false; _entityTree->withWriteLock([&] { updatedEntity = _entityTree->updateEntity(entityID, properties); @@ -594,64 +621,37 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& // return QUuid(); // } + bool hasQueryAACubeRelatedChanges = properties.queryAACubeRelatedPropertyChanged(); // done writing, send update - bool entityFound { false }; _entityTree->withReadLock([&] { - EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); + // find the entity again: maybe it was removed since we last found it + entity = _entityTree->findEntityByEntityItemID(entityID); if (entity) { - entityFound = true; - // make sure the properties has a type, so that the encode can know which properties to include - properties.setType(entity->getType()); - bool hasTransformOrVelocityChanges = properties.hasTransformOrVelocityChanges(); - bool hasPhysicsChanges = properties.hasMiscPhysicsChanges() || hasTransformOrVelocityChanges; - if (_bidOnSimulationOwnership && hasPhysicsChanges) { - if (entity->getSimulatorID() == sessionID) { - // we think we already own the simulation, so make sure to send the full transform and all velocities - if (hasTransformOrVelocityChanges) { - entity->getTransformAndVelocityProperties(properties); - } - // TODO: if we knew that ONLY transforms and velocities have changed in properties AND the object - // is dynamic AND it is active in the physics simulation then we could chose to NOT queue an update - // and instead let the physics simulation decide when to send the update. This would remove - // the "slide-no-rotate" glitch (and typical double-update) that we see during the "poke rolling - // balls" test. However, even if we solve this problem we still need to provide a "slerp the visible - // proxy toward the true physical position" feature to hide the final glitches in the remote watcher's - // simulation. + uint64_t now = usecTimestampNow(); + entity->setLastBroadcast(now); - if (entity->getSimulationPriority() < SCRIPT_POKE_SIMULATION_PRIORITY) { - // we re-assert our simulation ownership at a higher priority - properties.setSimulationOwner(sessionID, SCRIPT_POKE_SIMULATION_PRIORITY); - } - } else { - // we make a bid for simulation ownership - properties.setSimulationOwner(sessionID, SCRIPT_POKE_SIMULATION_PRIORITY); - entity->setScriptSimulationPriority(SCRIPT_POKE_SIMULATION_PRIORITY); - } - } - if (properties.queryAACubeRelatedPropertyChanged()) { + if (hasQueryAACubeRelatedChanges) { properties.setQueryAACube(entity->getQueryAACube()); - } - entity->setLastBroadcast(usecTimestampNow()); - properties.setLastEdited(entity->getLastEdited()); - // if we've moved an entity with children, check/update the queryAACube of all descendents and tell the server - // if they've changed. - entity->forEachDescendant([&](SpatiallyNestablePointer descendant) { - if (descendant->getNestableType() == NestableType::Entity) { - if (descendant->updateQueryAACube()) { - EntityItemPointer entityDescendant = std::static_pointer_cast(descendant); - EntityItemProperties newQueryCubeProperties; - newQueryCubeProperties.setQueryAACube(descendant->getQueryAACube()); - newQueryCubeProperties.setLastEdited(properties.getLastEdited()); - queueEntityMessage(PacketType::EntityEdit, descendant->getID(), newQueryCubeProperties); - entityDescendant->setLastBroadcast(usecTimestampNow()); + // if we've moved an entity with children, check/update the queryAACube of all descendents and tell the server + // if they've changed. + entity->forEachDescendant([&](SpatiallyNestablePointer descendant) { + if (descendant->getNestableType() == NestableType::Entity) { + if (descendant->updateQueryAACube()) { + EntityItemPointer entityDescendant = std::static_pointer_cast(descendant); + EntityItemProperties newQueryCubeProperties; + newQueryCubeProperties.setQueryAACube(descendant->getQueryAACube()); + newQueryCubeProperties.setLastEdited(properties.getLastEdited()); + queueEntityMessage(PacketType::EntityEdit, descendant->getID(), newQueryCubeProperties); + entityDescendant->setLastBroadcast(now); + } } - } - }); + }); + } } }); - if (!entityFound) { - { + if (!entity) { + if (hasQueryAACubeRelatedChanges) { // Sometimes ESS don't have the entity they are trying to edit in their local tree. In this case, // convertPropertiesFromScriptSemantics doesn't get called and local* edits will get dropped. // This is because, on the script side, "position" is in world frame, but in the network From 1a685d240960cb1cd559343e6fa3c3e9a554a0cd Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 4 Oct 2018 12:57:16 -0700 Subject: [PATCH 6/8] set script-triggerd sim ownership priority correctly --- interface/src/AvatarBookmarks.cpp | 6 +++-- libraries/entities/src/EntityItem.cpp | 7 ++++-- libraries/entities/src/EntityItem.h | 2 +- .../entities/src/EntityScriptingInterface.cpp | 24 +++++++++++-------- 4 files changed, 24 insertions(+), 15 deletions(-) diff --git a/interface/src/AvatarBookmarks.cpp b/interface/src/AvatarBookmarks.cpp index 5c79bedc9a..631ea94b49 100644 --- a/interface/src/AvatarBookmarks.cpp +++ b/interface/src/AvatarBookmarks.cpp @@ -78,8 +78,10 @@ void addAvatarEntities(const QVariantList& avatarEntities) { } entity->setLastBroadcast(usecTimestampNow()); - // since we're creating this object we will immediately volunteer to own its simulation - entity->setScriptSimulationPriority(VOLUNTEER_SIMULATION_PRIORITY); + if (entityProperties.getDynamic()) { + // since we're creating a dynamic object we volunteer immediately to own its simulation + entity->upgradeScriptSimulationPriority(VOLUNTEER_SIMULATION_PRIORITY); + } entityProperties.setLastEdited(entity->getLastEdited()); } else { qCDebug(entities) << "AvatarEntitiesBookmark failed to add new Entity to local Octree"; diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index c4ef74a0d7..36ffc68fd3 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -1382,8 +1382,11 @@ void EntityItem::getTransformAndVelocityProperties(EntityItemProperties& propert properties._accelerationChanged = true; } -void EntityItem::setScriptSimulationPriority(uint8_t priority) { - uint8_t newPriority = stillHasGrabActions() ? glm::max(priority, SCRIPT_GRAB_SIMULATION_PRIORITY) : priority; +void EntityItem::upgradeScriptSimulationPriority(uint8_t priority) { + uint8_t newPriority = glm::max(priority, _scriptSimulationPriority); + if (newPriority < SCRIPT_GRAB_SIMULATION_PRIORITY && stillHasGrabActions()) { + newPriority = SCRIPT_GRAB_SIMULATION_PRIORITY; + } if (newPriority != _scriptSimulationPriority) { // set the dirty flag to trigger a bid or ownership update markDirtyFlags(Simulation::DIRTY_SIMULATION_OWNERSHIP_PRIORITY); diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 2fa60e17cb..c49017b2e0 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -325,7 +325,7 @@ public: // TODO: move this "ScriptSimulationPriority" and "PendingOwnership" stuff into EntityMotionState // but first would need to do some other cleanup. In the meantime these live here as "scratch space" // to allow libs that don't know about each other to communicate. - void setScriptSimulationPriority(uint8_t priority); + void upgradeScriptSimulationPriority(uint8_t priority); void clearScriptSimulationPriority(); uint8_t getScriptSimulationPriority() const { return _scriptSimulationPriority; } void setPendingOwnershipPriority(uint8_t priority); diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index e745bf0874..73b9a58fc1 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -290,7 +290,7 @@ bool EntityScriptingInterface::addLocalEntityCopy(EntityItemProperties& properti entity->setLastBroadcast(usecTimestampNow()); // since we're creating this object we will immediately volunteer to own its simulation - entity->setScriptSimulationPriority(VOLUNTEER_SIMULATION_PRIORITY); + entity->upgradeScriptSimulationPriority(VOLUNTEER_SIMULATION_PRIORITY); properties.setLastEdited(entity->getLastEdited()); } else { qCDebug(entities) << "script failed to add new Entity to local Octree"; @@ -569,16 +569,20 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& // TODO: figure out how to change simulation ownership priority //} } else { - // we don't own the simulation + // we don't own the simulation but think we would like to + // --> flag the object for simulation ownership at at least POKE + // (the bid for simulation ownership (if any) will be sent by the physics simulation) + entity->upgradeScriptSimulationPriority(SCRIPT_POKE_SIMULATION_PRIORITY); + uint8_t desiredPriority = entity->getScriptSimulationPriority(); if (desiredPriority < simulationOwner.getPriority()) { - // we don't desire to own it --> clear restricted properties + // the priority at which we'd like to own it is not high enough + // --> assume failure and clear all restricted property changes properties.clearSimulationRestrictedProperties(); } else { - // we want to own it --> copy ALL restricted properties + // the priority at which we'd like to own it is high enough to win. + // --> assume success and copy ALL restricted properties properties.copySimulationRestrictedProperties(entity); - // and also store our simulation - properties.setSimulationOwner(sessionID, desiredPriority); } } } else if (!simulationOwner.getID().isNull()) { @@ -587,7 +591,7 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& properties.clearSimulationRestrictedProperties(); } // clear the cached simulationPriority level - entity->setScriptSimulationPriority(0); + entity->upgradeScriptSimulationPriority(0); } // set these to make EntityItemProperties::getScalesWithParent() work correctly @@ -1452,7 +1456,7 @@ QUuid EntityScriptingInterface::addAction(const QString& actionTypeString, } action->setIsMine(true); success = entity->addAction(simulation, action); - entity->setScriptSimulationPriority(SCRIPT_GRAB_SIMULATION_PRIORITY); + entity->upgradeScriptSimulationPriority(SCRIPT_GRAB_SIMULATION_PRIORITY); return false; // Physics will cause a packet to be sent, so don't send from here. }); if (success) { @@ -1468,7 +1472,7 @@ bool EntityScriptingInterface::updateAction(const QUuid& entityID, const QUuid& return actionWorker(entityID, [&](EntitySimulationPointer simulation, EntityItemPointer entity) { bool success = entity->updateAction(simulation, actionID, arguments); if (success) { - entity->setScriptSimulationPriority(SCRIPT_GRAB_SIMULATION_PRIORITY); + entity->upgradeScriptSimulationPriority(SCRIPT_GRAB_SIMULATION_PRIORITY); } return success; }); @@ -1482,7 +1486,7 @@ bool EntityScriptingInterface::deleteAction(const QUuid& entityID, const QUuid& success = entity->removeAction(simulation, actionID); if (success) { // reduce from grab to poke - entity->setScriptSimulationPriority(SCRIPT_POKE_SIMULATION_PRIORITY); + entity->upgradeScriptSimulationPriority(SCRIPT_POKE_SIMULATION_PRIORITY); } return false; // Physics will cause a packet to be sent, so don't send from here. }); From 9bfa4a5267da8a75ffdc9eba9ab296f3b911b1a7 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 4 Oct 2018 13:55:29 -0700 Subject: [PATCH 7/8] remove unhelpful/wrong TODO comment --- libraries/entities/src/EntityScriptingInterface.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 73b9a58fc1..850997457b 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -565,9 +565,6 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& if (simulationOwner.getID() == sessionID) { // we own the simulation --> copy ALL restricted properties properties.copySimulationRestrictedProperties(entity); - //if (ourBidPriority != currentBidPriority) { - // TODO: figure out how to change simulation ownership priority - //} } else { // we don't own the simulation but think we would like to // --> flag the object for simulation ownership at at least POKE From 6558d539e44481207caed644bcc977f4e6f95997 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Mon, 8 Oct 2018 11:19:37 -0700 Subject: [PATCH 8/8] adjust simulation bid priority by props changed --- libraries/entities/src/EntityItemProperties.cpp | 14 ++++++++++++++ libraries/entities/src/EntityItemProperties.h | 1 + .../entities/src/EntityScriptingInterface.cpp | 6 +++--- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/libraries/entities/src/EntityItemProperties.cpp b/libraries/entities/src/EntityItemProperties.cpp index 6921bc0bdd..c90d1f6635 100644 --- a/libraries/entities/src/EntityItemProperties.cpp +++ b/libraries/entities/src/EntityItemProperties.cpp @@ -3326,6 +3326,20 @@ void EntityItemProperties::setSimulationOwner(const QByteArray& data) { } } +uint8_t EntityItemProperties::computeSimulationBidPriority() const { + uint8_t priority = 0; + if (_parentIDChanged || _parentJointIndexChanged) { + // we need higher simulation ownership priority to chang parenting info + priority = SCRIPT_GRAB_SIMULATION_PRIORITY; + } else if ( _positionChanged || _localPositionChanged + || _rotationChanged || _localRotationChanged + || _velocityChanged || _localVelocityChanged + || _angularVelocityChanged || _localAngularVelocityChanged) { + priority = SCRIPT_POKE_SIMULATION_PRIORITY; + } + return priority; +} + QList EntityItemProperties::listChangedProperties() { QList out; if (containsPositionChange()) { diff --git a/libraries/entities/src/EntityItemProperties.h b/libraries/entities/src/EntityItemProperties.h index d351a9f092..ae2c402d22 100644 --- a/libraries/entities/src/EntityItemProperties.h +++ b/libraries/entities/src/EntityItemProperties.h @@ -360,6 +360,7 @@ public: void setSimulationOwner(const QUuid& id, uint8_t priority); void setSimulationOwner(const QByteArray& data); void setSimulationPriority(uint8_t priority) { _simulationOwner.setPriority(priority); } + uint8_t computeSimulationBidPriority() const; void setActionDataDirty() { _actionDataChanged = true; } diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 850997457b..f8b22fdbae 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -562,14 +562,14 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& if (entity) { if (properties.hasSimulationRestrictedChanges()) { if (_bidOnSimulationOwnership) { + // flag for simulation ownership, or upgrade existing ownership priority + // (actual bids for simulation ownership are sent by the PhysicalEntitySimulation) + entity->upgradeScriptSimulationPriority(properties.computeSimulationBidPriority()); if (simulationOwner.getID() == sessionID) { // we own the simulation --> copy ALL restricted properties properties.copySimulationRestrictedProperties(entity); } else { // we don't own the simulation but think we would like to - // --> flag the object for simulation ownership at at least POKE - // (the bid for simulation ownership (if any) will be sent by the physics simulation) - entity->upgradeScriptSimulationPriority(SCRIPT_POKE_SIMULATION_PRIORITY); uint8_t desiredPriority = entity->getScriptSimulationPriority(); if (desiredPriority < simulationOwner.getPriority()) {