From c3f9663ab0addb14ed9e3c6d64fe249d7f255b9f Mon Sep 17 00:00:00 2001 From: Thijs Wenker Date: Tue, 31 Jan 2017 01:25:59 +0100 Subject: [PATCH 1/3] - Fix for simulation owned entities moving to 0,0,0 after cache reload - Fix for simulation priority, use SCRIPT_GRAB_SIMULATION_PRIORITY in EntityItem::grabSimulationOwnership() --- libraries/entities/src/EntityItem.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 64bc9fbd5a..52dad5e976 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -688,6 +688,14 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef somethingChanged = true; _simulationOwner.clearCurrentOwner(); } + } else if (newSimOwner.matchesValidID(myNodeID) && !(_dirtyFlags & Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_POKE) + && !(_dirtyFlags & Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_GRAB)) { + // entity-server tells us that we have simulation ownership while we never requested this for this EntityItem, + // this could happen when the user reloads the cache and entity tree. + _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; + somethingChanged = true; + _simulationOwner.clearCurrentOwner(); + weOwnSimulation = false; } else if (_simulationOwner.set(newSimOwner)) { _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; somethingChanged = true; @@ -1278,7 +1286,7 @@ void EntityItem::grabSimulationOwnership() { auto nodeList = DependencyManager::get(); if (_simulationOwner.matchesValidID(nodeList->getSessionUUID())) { // we already own it - _simulationOwner.promotePriority(SCRIPT_POKE_SIMULATION_PRIORITY); + _simulationOwner.promotePriority(SCRIPT_GRAB_SIMULATION_PRIORITY); } else { // we don't own it yet _simulationOwner.setPendingPriority(SCRIPT_GRAB_SIMULATION_PRIORITY, usecTimestampNow()); From 360899887775ae01049d57ab552f4cc83e6f479a Mon Sep 17 00:00:00 2001 From: Thijs Wenker Date: Tue, 31 Jan 2017 21:01:03 +0100 Subject: [PATCH 2/3] use dedicated bool rather than unreliable dirtyFlags to determine if the entityItem had ever bid for simulation ownership --- libraries/entities/src/EntityItem.cpp | 15 +++++++++++++-- libraries/entities/src/EntityItem.h | 7 ++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index d77fac131d..61f082c9b6 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -688,8 +688,7 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef somethingChanged = true; _simulationOwner.clearCurrentOwner(); } - } else if (newSimOwner.matchesValidID(myNodeID) && !(_dirtyFlags & Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_POKE) - && !(_dirtyFlags & Simulation::DIRTY_SIMULATION_OWNERSHIP_FOR_GRAB)) { + } else if (newSimOwner.matchesValidID(myNodeID) && !_hasBidOnSimulation) { // entity-server tells us that we have simulation ownership while we never requested this for this EntityItem, // this could happen when the user reloads the cache and entity tree. _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; @@ -1279,6 +1278,7 @@ void EntityItem::pokeSimulationOwnership() { // we don't own it yet _simulationOwner.setPendingPriority(SCRIPT_POKE_SIMULATION_PRIORITY, usecTimestampNow()); } + checkForFirstSimulationBid(_simulationOwner); } void EntityItem::grabSimulationOwnership() { @@ -1291,6 +1291,7 @@ void EntityItem::grabSimulationOwnership() { // we don't own it yet _simulationOwner.setPendingPriority(SCRIPT_GRAB_SIMULATION_PRIORITY, usecTimestampNow()); } + checkForFirstSimulationBid(_simulationOwner); } bool EntityItem::setProperties(const EntityItemProperties& properties) { @@ -1861,6 +1862,7 @@ void EntityItem::setSimulationOwner(const QUuid& id, quint8 priority) { qCDebug(entities) << "sim ownership for" << getDebugName() << "is now" << id << priority; } _simulationOwner.set(id, priority); + checkForFirstSimulationBid(_simulationOwner); } void EntityItem::setSimulationOwner(const SimulationOwner& owner) { @@ -1869,6 +1871,7 @@ void EntityItem::setSimulationOwner(const SimulationOwner& owner) { } _simulationOwner.set(owner); + checkForFirstSimulationBid(_simulationOwner); } void EntityItem::updateSimulationOwner(const SimulationOwner& owner) { @@ -1879,6 +1882,7 @@ void EntityItem::updateSimulationOwner(const SimulationOwner& owner) { if (_simulationOwner.set(owner)) { _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; } + checkForFirstSimulationBid(_simulationOwner); } void EntityItem::clearSimulationOwnership() { @@ -1895,6 +1899,7 @@ void EntityItem::clearSimulationOwnership() { void EntityItem::setPendingOwnershipPriority(quint8 priority, const quint64& timestamp) { _simulationOwner.setPendingPriority(priority, timestamp); + checkForFirstSimulationBid(_simulationOwner); } QString EntityItem::actionsToDebugString() { @@ -2152,6 +2157,12 @@ void EntityItem::setActionDataInternal(QByteArray actionData) { checkWaitingToRemove(); } +void EntityItem::checkForFirstSimulationBid(const SimulationOwner& simulationOwner) const { + if (!_hasBidOnSimulation && simulationOwner.matchesValidID(DependencyManager::get()->getSessionUUID())) { + _hasBidOnSimulation = true; + } +} + void EntityItem::serializeActions(bool& success, QByteArray& result) const { if (_objectActions.size() == 0) { success = true; diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index b203de203b..98a2a1e268 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -478,6 +478,8 @@ protected: const QByteArray getActionDataInternal() const; void setActionDataInternal(QByteArray actionData); + void checkForFirstSimulationBid(const SimulationOwner& simulationOwner) const; + virtual void locationChanged(bool tellPhysics = true) override; virtual void dimensionsChanged() override; @@ -586,6 +588,9 @@ protected: static quint64 _rememberDeletedActionTime; mutable QHash _previouslyDeletedActions; + // per entity keep state if it ever bid on simulation, so that we can ignore false simulation ownership + mutable bool _hasBidOnSimulation = false; + QUuid _sourceUUID; /// the server node UUID we came from bool _clientOnly { false }; @@ -594,7 +599,7 @@ protected: // physics related changes from the network to suppress any duplicates and make // sure redundant applications are idempotent glm::vec3 _lastUpdatedPositionValue; - glm::quat _lastUpdatedRotationValue; + glm::quat _lastUpdatedRotationValue; glm::vec3 _lastUpdatedVelocityValue; glm::vec3 _lastUpdatedAngularVelocityValue; glm::vec3 _lastUpdatedAccelerationValue; From 174a7ad5bdadc72f14875281889b26fb7706ac2b Mon Sep 17 00:00:00 2001 From: Thijs Wenker Date: Tue, 31 Jan 2017 22:54:58 +0100 Subject: [PATCH 3/3] Optimizations and style fixes from code review --- libraries/entities/src/EntityItem.cpp | 16 +++-------- libraries/entities/src/EntityItem.h | 27 +++++++++---------- .../entities/src/EntityScriptingInterface.cpp | 2 ++ libraries/physics/src/EntityMotionState.cpp | 2 ++ 4 files changed, 21 insertions(+), 26 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 61f082c9b6..3c10d0382c 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -1278,7 +1278,6 @@ void EntityItem::pokeSimulationOwnership() { // we don't own it yet _simulationOwner.setPendingPriority(SCRIPT_POKE_SIMULATION_PRIORITY, usecTimestampNow()); } - checkForFirstSimulationBid(_simulationOwner); } void EntityItem::grabSimulationOwnership() { @@ -1291,7 +1290,6 @@ void EntityItem::grabSimulationOwnership() { // we don't own it yet _simulationOwner.setPendingPriority(SCRIPT_GRAB_SIMULATION_PRIORITY, usecTimestampNow()); } - checkForFirstSimulationBid(_simulationOwner); } bool EntityItem::setProperties(const EntityItemProperties& properties) { @@ -1862,7 +1860,6 @@ void EntityItem::setSimulationOwner(const QUuid& id, quint8 priority) { qCDebug(entities) << "sim ownership for" << getDebugName() << "is now" << id << priority; } _simulationOwner.set(id, priority); - checkForFirstSimulationBid(_simulationOwner); } void EntityItem::setSimulationOwner(const SimulationOwner& owner) { @@ -1871,7 +1868,6 @@ void EntityItem::setSimulationOwner(const SimulationOwner& owner) { } _simulationOwner.set(owner); - checkForFirstSimulationBid(_simulationOwner); } void EntityItem::updateSimulationOwner(const SimulationOwner& owner) { @@ -1882,7 +1878,6 @@ void EntityItem::updateSimulationOwner(const SimulationOwner& owner) { if (_simulationOwner.set(owner)) { _dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID; } - checkForFirstSimulationBid(_simulationOwner); } void EntityItem::clearSimulationOwnership() { @@ -1899,7 +1894,10 @@ void EntityItem::clearSimulationOwnership() { void EntityItem::setPendingOwnershipPriority(quint8 priority, const quint64& timestamp) { _simulationOwner.setPendingPriority(priority, timestamp); - checkForFirstSimulationBid(_simulationOwner); +} + +void EntityItem::rememberHasSimulationOwnershipBid() const { + _hasBidOnSimulation = true; } QString EntityItem::actionsToDebugString() { @@ -2157,12 +2155,6 @@ void EntityItem::setActionDataInternal(QByteArray actionData) { checkWaitingToRemove(); } -void EntityItem::checkForFirstSimulationBid(const SimulationOwner& simulationOwner) const { - if (!_hasBidOnSimulation && simulationOwner.matchesValidID(DependencyManager::get()->getSessionUUID())) { - _hasBidOnSimulation = true; - } -} - void EntityItem::serializeActions(bool& success, QByteArray& result) const { if (_objectActions.size() == 0) { success = true; diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 98a2a1e268..e69195d53d 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -321,6 +321,7 @@ public: void updateSimulationOwner(const SimulationOwner& owner); void clearSimulationOwnership(); void setPendingOwnershipPriority(quint8 priority, const quint64& timestamp); + void rememberHasSimulationOwnershipBid() const; const QString& getMarketplaceID() const { return _marketplaceID; } void setMarketplaceID(const QString& value) { _marketplaceID = value; } @@ -478,8 +479,6 @@ protected: const QByteArray getActionDataInternal() const; void setActionDataInternal(QByteArray actionData); - void checkForFirstSimulationBid(const SimulationOwner& simulationOwner) const; - virtual void locationChanged(bool tellPhysics = true) override; virtual void dimensionsChanged() override; @@ -499,16 +498,16 @@ protected: mutable AABox _cachedAABox; mutable AACube _maxAACube; mutable AACube _minAACube; - mutable bool _recalcAABox = true; - mutable bool _recalcMinAACube = true; - mutable bool _recalcMaxAACube = true; + mutable bool _recalcAABox { true }; + mutable bool _recalcMinAACube { true }; + mutable bool _recalcMaxAACube { true }; float _localRenderAlpha; - float _density = ENTITY_ITEM_DEFAULT_DENSITY; // kg/m^3 + float _density { ENTITY_ITEM_DEFAULT_DENSITY }; // kg/m^3 // NOTE: _volumeMultiplier is used to allow some mass properties code exist in the EntityItem base class // rather than in all of the derived classes. If we ever collapse these classes to one we could do it a // different way. - float _volumeMultiplier = 1.0f; + float _volumeMultiplier { 1.0f }; glm::vec3 _gravity; glm::vec3 _acceleration; float _damping; @@ -518,7 +517,7 @@ protected: QString _script; /// the value of the script property QString _loadedScript; /// the value of _script when the last preload signal was sent - quint64 _scriptTimestamp{ ENTITY_ITEM_DEFAULT_SCRIPT_TIMESTAMP }; /// the script loaded property used for forced reload + quint64 _scriptTimestamp { ENTITY_ITEM_DEFAULT_SCRIPT_TIMESTAMP }; /// the script loaded property used for forced reload QString _serverScripts; /// keep track of time when _serverScripts property was last changed @@ -526,7 +525,7 @@ protected: /// the value of _scriptTimestamp when the last preload signal was sent // NOTE: on construction we want this to be different from _scriptTimestamp so we intentionally bump it - quint64 _loadedScriptTimestamp{ ENTITY_ITEM_DEFAULT_SCRIPT_TIMESTAMP + 1 }; + quint64 _loadedScriptTimestamp { ENTITY_ITEM_DEFAULT_SCRIPT_TIMESTAMP + 1 }; QString _collisionSoundURL; SharedSoundPointer _collisionSound; @@ -564,8 +563,8 @@ protected: uint32_t _dirtyFlags; // things that have changed from EXTERNAL changes (via script or packet) but NOT from simulation // these backpointers are only ever set/cleared by friends: - EntityTreeElementPointer _element = nullptr; // set by EntityTreeElement - void* _physicsInfo = nullptr; // set by EntitySimulation + EntityTreeElementPointer _element { nullptr }; // set by EntityTreeElement + void* _physicsInfo { nullptr }; // set by EntitySimulation bool _simulated; // set by EntitySimulation bool addActionInternal(EntitySimulationPointer simulation, EntityActionPointer action); @@ -582,14 +581,14 @@ protected: // are used to keep track of and work around this situation. void checkWaitingToRemove(EntitySimulationPointer simulation = nullptr); mutable QSet _actionsToRemove; - mutable bool _actionDataDirty = false; - mutable bool _actionDataNeedsTransmit = false; + mutable bool _actionDataDirty { false }; + mutable bool _actionDataNeedsTransmit { false }; // _previouslyDeletedActions is used to avoid an action being re-added due to server round-trip lag static quint64 _rememberDeletedActionTime; mutable QHash _previouslyDeletedActions; // per entity keep state if it ever bid on simulation, so that we can ignore false simulation ownership - mutable bool _hasBidOnSimulation = false; + mutable bool _hasBidOnSimulation { false }; QUuid _sourceUUID; /// the server node UUID we came from diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 6fb5d14329..85c3fc74f6 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -231,6 +231,7 @@ QUuid EntityScriptingInterface::addEntity(const EntityItemProperties& properties // and make note of it now, so we can act on it right away. propertiesWithSimID.setSimulationOwner(myNodeID, SCRIPT_POKE_SIMULATION_PRIORITY); entity->setSimulationOwner(myNodeID, SCRIPT_POKE_SIMULATION_PRIORITY); + entity->rememberHasSimulationOwnershipBid(); } entity->setLastBroadcast(usecTimestampNow()); @@ -444,6 +445,7 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties& // we make a bid for simulation ownership properties.setSimulationOwner(myNodeID, SCRIPT_POKE_SIMULATION_PRIORITY); entity->pokeSimulationOwnership(); + entity->rememberHasSimulationOwnershipBid(); } } if (properties.parentRelatedPropertyChanged() && entity->computePuffedQueryAACube()) { diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index b0bdc34b52..02cee9a03a 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -582,6 +582,8 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_ _nextOwnershipBid = now + USECS_BETWEEN_OWNERSHIP_BIDS; // copy _outgoingPriority into pendingPriority... _entity->setPendingOwnershipPriority(_outgoingPriority, now); + // don't forget to remember that we have made a bid + _entity->rememberHasSimulationOwnershipBid(); // ...then reset _outgoingPriority in preparation for the next frame _outgoingPriority = 0; } else if (_outgoingPriority != _entity->getSimulationPriority()) {