From 7ed35399e9e6405f511102c46e268374ccdd9145 Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Thu, 2 Jun 2016 18:01:23 -0700 Subject: [PATCH 1/2] make the application of redundant physics update idempotent to improve action glitches --- libraries/entities/src/EntityItem.cpp | 95 ++++++++++++++++++++++----- libraries/entities/src/EntityItem.h | 16 +++++ 2 files changed, 95 insertions(+), 16 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index dc5fbf7d8e..64b6a2c655 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -687,15 +687,80 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef } } { // 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 && !weOwnSimulation; - READ_ENTITY_PROPERTY(PROP_POSITION, glm::vec3, updatePositionFromNetwork); - READ_ENTITY_PROPERTY(PROP_ROTATION, glm::quat, updateRotationFromNetwork); - READ_ENTITY_PROPERTY(PROP_VELOCITY, glm::vec3, updateVelocityFromNetwork); - READ_ENTITY_PROPERTY(PROP_ANGULAR_VELOCITY, glm::vec3, updateAngularVelocityFromNetwork); - READ_ENTITY_PROPERTY(PROP_ACCELERATION, glm::vec3, setAcceleration); - overwriteLocalData = oldOverwrite; + // we also want to ignore any duplicate packets that have the same "recently updated" values + // as a packet we've already recieved. This is because we want multiple edits of the same + // information to be idempotent, but if we applied new physics properties we'd resimulation + // with small differences in results. + + // Because the regular streaming property "setters" only have access to the new value, we've + // made these lambdas that can access other details about the previous updates to suppress + // any duplicates. + + // Note: duplicate packets are expected and not wrong. They may be sent for any number of + // reasons and the contract is that the client handles them in an idempotent manner. + auto lastEdited = lastEditedFromBufferAdjusted; + auto customUpdatePositionFromNetwork = [this, lastEdited, overwriteLocalData, weOwnSimulation](glm::vec3 value){ + bool simulationChanged = lastEdited > _lastUpdatedPositionTimestamp; + bool valueChanged = value != _lastUpdatedPositionValue; + bool shouldUpdate = overwriteLocalData && !weOwnSimulation && simulationChanged && valueChanged; + if (shouldUpdate) { + updatePositionFromNetwork(value); + _lastUpdatedPositionTimestamp = lastEdited; + _lastUpdatedPositionValue = value; + } + }; + + auto customUpdateRotationFromNetwork = [this, lastEdited, overwriteLocalData, weOwnSimulation](glm::quat value){ + bool simulationChanged = lastEdited > _lastUpdatedRotationTimestamp; + bool valueChanged = value != _lastUpdatedRotationValue; + bool shouldUpdate = overwriteLocalData && !weOwnSimulation && simulationChanged && valueChanged; + if (shouldUpdate) { + updateRotationFromNetwork(value); + _lastUpdatedRotationTimestamp = lastEdited; + _lastUpdatedRotationValue = value; + } + }; + + auto customUpdateVelocityFromNetwork = [this, lastEdited, overwriteLocalData, weOwnSimulation](glm::vec3 value){ + bool simulationChanged = lastEdited > _lastUpdatedVelocityTimestamp; + bool valueChanged = value != _lastUpdatedVelocityValue; + bool shouldUpdate = overwriteLocalData && !weOwnSimulation && simulationChanged && valueChanged; + if (shouldUpdate) { + updateVelocityFromNetwork(value); + _lastUpdatedVelocityTimestamp = lastEdited; + _lastUpdatedVelocityValue = value; + } + }; + + auto customUpdateAngularVelocityFromNetwork = [this, lastEdited, overwriteLocalData, weOwnSimulation](glm::vec3 value){ + bool simulationChanged = lastEdited > _lastUpdatedAngularVelocityTimestamp; + bool valueChanged = value != _lastUpdatedAngularVelocityValue; + bool shouldUpdate = overwriteLocalData && !weOwnSimulation && simulationChanged && valueChanged; + if (shouldUpdate) { + updateAngularVelocityFromNetwork(value); + _lastUpdatedAngularVelocityTimestamp = lastEdited; + _lastUpdatedAngularVelocityValue = value; + } + }; + + auto customSetAcceleration = [this, lastEdited, overwriteLocalData, weOwnSimulation](glm::vec3 value){ + bool simulationChanged = lastEdited > _lastUpdatedAccelerationTimestamp; + bool valueChanged = value != _lastUpdatedAccelerationValue; + bool shouldUpdate = overwriteLocalData && !weOwnSimulation && simulationChanged && valueChanged; + if (shouldUpdate) { + setAcceleration(value); + _lastUpdatedAccelerationTimestamp = lastEdited; + _lastUpdatedAccelerationValue = value; + } + }; + + READ_ENTITY_PROPERTY(PROP_POSITION, glm::vec3, customUpdatePositionFromNetwork); + READ_ENTITY_PROPERTY(PROP_ROTATION, glm::quat, customUpdateRotationFromNetwork); + READ_ENTITY_PROPERTY(PROP_VELOCITY, glm::vec3, customUpdateVelocityFromNetwork); + READ_ENTITY_PROPERTY(PROP_ANGULAR_VELOCITY, glm::vec3, customUpdateAngularVelocityFromNetwork); + READ_ENTITY_PROPERTY(PROP_ACCELERATION, glm::vec3, customSetAcceleration); + + } READ_ENTITY_PROPERTY(PROP_DIMENSIONS, glm::vec3, updateDimensions); @@ -922,13 +987,11 @@ void EntityItem::simulate(const quint64& now) { qCDebug(entities) << " ********** EntityItem::simulate() .... SETTING _lastSimulated=" << _lastSimulated; #endif - if (!hasActions()) { - if (!stepKinematicMotion(timeElapsed)) { - // this entity is no longer moving - // flag it to transition from KINEMATIC to STATIC - _dirtyFlags |= Simulation::DIRTY_MOTION_TYPE; - setAcceleration(Vectors::ZERO); - } + if (!stepKinematicMotion(timeElapsed)) { + // this entity is no longer moving + // flag it to transition from KINEMATIC to STATIC + _dirtyFlags |= Simulation::DIRTY_MOTION_TYPE; + setAcceleration(Vectors::ZERO); } _lastSimulated = now; } diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 2c6dc9b74d..96297a09ce 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -550,6 +550,22 @@ protected: bool _clientOnly { false }; QUuid _owningAvatarID; + + // physics related changes from the network to suppress any duplicates and make + // sure redundant applications are idempotent + glm::vec3 _lastUpdatedPositionValue; + glm::quat _lastUpdatedRotationValue; + glm::vec3 _lastUpdatedVelocityValue; + glm::vec3 _lastUpdatedAngularVelocityValue; + glm::vec3 _lastUpdatedAccelerationValue; + + quint64 _lastUpdatedPositionTimestamp = 0; + quint64 _lastUpdatedRotationTimestamp = 0; + quint64 _lastUpdatedVelocityTimestamp = 0; + quint64 _lastUpdatedAngularVelocityTimestamp = 0; + quint64 _lastUpdatedAccelerationTimestamp = 0; + + }; #endif // hifi_EntityItem_h From 1075ba1599160728f35ea40c4616873f95ebf636 Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Fri, 3 Jun 2016 08:01:09 -0700 Subject: [PATCH 2/2] CR feedback --- libraries/entities/src/EntityItem.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 96297a09ce..4a691462ab 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -559,11 +559,11 @@ protected: glm::vec3 _lastUpdatedAngularVelocityValue; glm::vec3 _lastUpdatedAccelerationValue; - quint64 _lastUpdatedPositionTimestamp = 0; - quint64 _lastUpdatedRotationTimestamp = 0; - quint64 _lastUpdatedVelocityTimestamp = 0; - quint64 _lastUpdatedAngularVelocityTimestamp = 0; - quint64 _lastUpdatedAccelerationTimestamp = 0; + quint64 _lastUpdatedPositionTimestamp { 0 }; + quint64 _lastUpdatedRotationTimestamp { 0 }; + quint64 _lastUpdatedVelocityTimestamp { 0 }; + quint64 _lastUpdatedAngularVelocityTimestamp { 0 }; + quint64 _lastUpdatedAccelerationTimestamp { 0 }; };