From 470a45c012d0fe6c3ee41dd8022e10d7b8dc89c3 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 27 Sep 2019 14:14:26 -0700 Subject: [PATCH] EntityItem::_owningAvatarID always AVATAR_SELF_ID for MyAvatar's entities --- interface/src/avatar/MyAvatar.cpp | 16 +++++++++---- .../src/EntityTreeRenderer.cpp | 10 ++++---- .../src/RenderableEntityItem.cpp | 5 ++-- .../entities/src/EntityEditPacketSender.cpp | 8 +++++-- libraries/entities/src/EntityItem.cpp | 23 +++++++++++++++++-- libraries/entities/src/EntityItem.h | 3 ++- .../entities/src/EntityScriptingInterface.cpp | 21 +++++++---------- libraries/entities/src/EntityTree.cpp | 5 ++-- libraries/entities/src/EntityTreeElement.cpp | 2 +- libraries/physics/src/EntityMotionState.cpp | 15 +++++++++--- libraries/physics/src/EntityMotionState.h | 2 +- 11 files changed, 73 insertions(+), 37 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index de6ae526b4..7aeb0aaa64 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -1585,7 +1585,12 @@ void MyAvatar::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFrom void MyAvatar::sanitizeAvatarEntityProperties(EntityItemProperties& properties) const { properties.setEntityHostType(entity::HostType::AVATAR); - properties.setOwningAvatarID(getID()); + + // Note: we store AVATAR_SELF_ID in EntityItem::_owningAvatarID and we usually + // store the actual sessionUUID in EntityItemProperties::_owningAvatarID (for JS + // consumption, for example). However at this context we are preparing properties + // for outgoing packet, in which case we use AVATAR_SELF_ID. + properties.setOwningAvatarID(AVATAR_SELF_ID); // there's no entity-server to tell us we're the simulation owner, so always set the // simulationOwner to the owningAvatarID and a high priority. @@ -4000,6 +4005,10 @@ float MyAvatar::getGravity() { void MyAvatar::setSessionUUID(const QUuid& sessionUUID) { QUuid oldSessionID = getSessionUUID(); Avatar::setSessionUUID(sessionUUID); + bool sendPackets = !DependencyManager::get()->getSessionUUID().isNull(); + if (!sendPackets) { + return; + } QUuid newSessionID = getSessionUUID(); if (newSessionID != oldSessionID) { auto treeRenderer = DependencyManager::get(); @@ -4009,7 +4018,6 @@ void MyAvatar::setSessionUUID(const QUuid& sessionUUID) { _avatarEntitiesLock.withReadLock([&] { avatarEntityIDs = _packedAvatarEntityData.keys(); }); - bool sendPackets = !DependencyManager::get()->getSessionUUID().isNull(); EntityEditPacketSender* packetSender = qApp->getEntityEditPacketSender(); entityTree->withWriteLock([&] { for (const auto& entityID : avatarEntityIDs) { @@ -4017,11 +4025,9 @@ void MyAvatar::setSessionUUID(const QUuid& sessionUUID) { if (!entity) { continue; } - // update OwningAvatarID so entity can be identified as "ours" later - entity->setOwningAvatarID(newSessionID); // NOTE: each attached AvatarEntity already have the correct updated parentID // via magic in SpatiallyNestable, hence we check against newSessionID - if (sendPackets && entity->getParentID() == newSessionID) { + if (entity->getParentID() == newSessionID) { // but when we have a real session and the AvatarEntity is parented to MyAvatar // we need to update the "packedAvatarEntityData" sent to the avatar-mixer // because it contains a stale parentID somewhere deep inside diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 52738bb6cd..6533d462a4 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -221,7 +221,7 @@ void EntityTreeRenderer::stopDomainAndNonOwnedEntities() { EntityItemPointer entityItem = getTree()->findEntityByEntityItemID(entityID); if (entityItem && !entityItem->getScript().isEmpty()) { - if (!(entityItem->isLocalEntity() || (entityItem->isAvatarEntity() && entityItem->getOwningAvatarID() == getTree()->getMyAvatarSessionUUID()))) { + if (!(entityItem->isLocalEntity() || entityItem->isMyAvatarEntity())) { if (_currentEntitiesInside.contains(entityID)) { _entitiesScriptEngine->callEntityScriptMethod(entityID, "leaveEntity"); } @@ -235,7 +235,6 @@ void EntityTreeRenderer::stopDomainAndNonOwnedEntities() { void EntityTreeRenderer::clearDomainAndNonOwnedEntities() { stopDomainAndNonOwnedEntities(); - auto sessionUUID = getTree()->getMyAvatarSessionUUID(); std::unordered_map savedEntities; std::unordered_set savedRenderables; // remove all entities from the scene @@ -244,7 +243,7 @@ void EntityTreeRenderer::clearDomainAndNonOwnedEntities() { for (const auto& entry : _entitiesInScene) { const auto& renderer = entry.second; const EntityItemPointer& entityItem = renderer->getEntity(); - if (!(entityItem->isLocalEntity() || (entityItem->isAvatarEntity() && entityItem->getOwningAvatarID() == sessionUUID))) { + if (!(entityItem->isLocalEntity() || entityItem->isMyAvatarEntity())) { fadeOutRenderable(renderer); } else { savedEntities[entry.first] = entry.second; @@ -256,6 +255,7 @@ void EntityTreeRenderer::clearDomainAndNonOwnedEntities() { _renderablesToUpdate = savedRenderables; _entitiesInScene = savedEntities; + auto sessionUUID = getTree()->getMyAvatarSessionUUID(); if (_layeredZones.clearDomainAndNonOwnedZones(sessionUUID)) { applyLayeredZones(); } @@ -678,7 +678,7 @@ void EntityTreeRenderer::leaveDomainAndNonOwnedEntities() { QSet currentEntitiesInsideToSave; foreach (const EntityItemID& entityID, _currentEntitiesInside) { EntityItemPointer entityItem = getTree()->findEntityByEntityItemID(entityID); - if (!(entityItem->isLocalEntity() || (entityItem->isAvatarEntity() && entityItem->getOwningAvatarID() == getTree()->getMyAvatarSessionUUID()))) { + if (!(entityItem->isLocalEntity() || entityItem->isMyAvatarEntity())) { emit leaveEntity(entityID); if (_entitiesScriptEngine) { _entitiesScriptEngine->callEntityScriptMethod(entityID, "leaveEntity"); @@ -1216,7 +1216,7 @@ bool EntityTreeRenderer::LayeredZones::clearDomainAndNonOwnedZones(const QUuid& auto it = begin(); while (it != end()) { auto zone = it->zone.lock(); - if (!zone || !(zone->isLocalEntity() || (zone->isAvatarEntity() && zone->getOwningAvatarID() == sessionUUID))) { + if (!zone || !(zone->isLocalEntity() || zone->isMyAvatarEntity())) { zonesChanged = true; it = erase(it); } else { diff --git a/libraries/entities-renderer/src/RenderableEntityItem.cpp b/libraries/entities-renderer/src/RenderableEntityItem.cpp index fb3d2f1bf5..0e5dab6524 100644 --- a/libraries/entities-renderer/src/RenderableEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableEntityItem.cpp @@ -43,6 +43,7 @@ const Transform& EntityRenderer::getModelTransform() const { void EntityRenderer::makeStatusGetters(const EntityItemPointer& entity, Item::Status::Getters& statusGetters) { auto nodeList = DependencyManager::get(); + // DANGER: nodeList->getSessionUUID() will return null id when not connected to domain. const QUuid& myNodeID = nodeList->getSessionUUID(); statusGetters.push_back([entity]() -> render::Item::Status::Value { @@ -103,9 +104,9 @@ void EntityRenderer::makeStatusGetters(const EntityItemPointer& entity, Item::St (unsigned char)render::Item::Status::Icon::HAS_ACTIONS); }); - statusGetters.push_back([entity, myNodeID] () -> render::Item::Status::Value { + statusGetters.push_back([entity] () -> render::Item::Status::Value { if (entity->isAvatarEntity()) { - if (entity->getOwningAvatarID() == myNodeID) { + if (entity->isMyAvatarEntity()) { return render::Item::Status::Value(1.0f, render::Item::Status::Value::GREEN, (unsigned char)render::Item::Status::Icon::ENTITY_HOST_TYPE); } else { diff --git a/libraries/entities/src/EntityEditPacketSender.cpp b/libraries/entities/src/EntityEditPacketSender.cpp index dbb3ab076e..aaaf7d645a 100644 --- a/libraries/entities/src/EntityEditPacketSender.cpp +++ b/libraries/entities/src/EntityEditPacketSender.cpp @@ -73,8 +73,12 @@ void EntityEditPacketSender::queueEditEntityMessage(PacketType type, if (properties.getEntityHostType() == entity::HostType::AVATAR) { if (!_myAvatar) { qCWarning(entities) << "Suppressing entity edit message: cannot send avatar entity edit with no myAvatar"; - } else if (properties.getOwningAvatarID() == _myAvatar->getID()) { - // this is an avatar-based entity --> update our avatar-data rather than sending to the entity-server + } else if (properties.getOwningAvatarID() == _myAvatar->getID() || properties.getOwningAvatarID() == AVATAR_SELF_ID) { + // this is a local avatar-entity --> update our avatar-data rather than sending to the entity-server + // Note: we store AVATAR_SELF_ID in EntityItem::_owningAvatarID and we usually + // store the actual sessionUUID in EntityItemProperties::_owningAvatarID. + // However at this context we check for both cases just in case. Really we just want to know + // where to route the data: entity-server or avatar-mixer. queueEditAvatarEntityMessage(entityTree, entityItemID); } else { qCWarning(entities) << "Suppressing entity edit message: cannot send avatar entity edit for another avatar"; diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 598ac17510..60a8cadf6c 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -1347,7 +1347,7 @@ EntityItemProperties EntityItem::getProperties(const EntityPropertyFlags& desire COPY_ENTITY_PROPERTY_TO_PROPERTIES(created, getCreated); COPY_ENTITY_PROPERTY_TO_PROPERTIES(lastEditedBy, getLastEditedBy); COPY_ENTITY_PROPERTY_TO_PROPERTIES(entityHostType, getEntityHostType); - COPY_ENTITY_PROPERTY_TO_PROPERTIES(owningAvatarID, getOwningAvatarID); + COPY_ENTITY_PROPERTY_TO_PROPERTIES(owningAvatarID, getOwningAvatarIDForProperties); COPY_ENTITY_PROPERTY_TO_PROPERTIES(queryAACube, getQueryAACube); COPY_ENTITY_PROPERTY_TO_PROPERTIES(canCastShadow, getCanCastShadow); COPY_ENTITY_PROPERTY_TO_PROPERTIES(isVisibleInSecondaryCamera, isVisibleInSecondaryCamera); @@ -3398,6 +3398,7 @@ void EntityItem::prepareForSimulationOwnershipBid(EntityItemProperties& properti properties.setSimulationOwner(Physics::getSessionUUID(), priority); setPendingOwnershipPriority(priority); + // ANDREW TODO: figure out if it would be OK to NOT bother set these properties properties.setEntityHostType(getEntityHostType()); properties.setOwningAvatarID(getOwningAvatarID()); setLastBroadcast(now); // for debug/physics status icons @@ -3409,9 +3410,27 @@ bool EntityItem::isWearable() const { } bool EntityItem::isMyAvatarEntity() const { - return _hostType == entity::HostType::AVATAR && Physics::getSessionUUID() == _owningAvatarID; + return _hostType == entity::HostType::AVATAR && AVATAR_SELF_ID == _owningAvatarID; }; +QUuid EntityItem::getOwningAvatarIDForProperties() const { + if (isMyAvatarEntity()) { + // NOTE: we always store AVATAR_SELF_ID for MyAvatar's avatar entities, + // however for EntityItemProperties to be consumed by outside contexts (e.g. JS) + // we use the actual "sessionUUID" which is conveniently cached in the Physics namespace + return Physics::getSessionUUID(); + } + return _owningAvatarID; +} + +void EntityItem::setOwningAvatarID(const QUuid& owningAvatarID) { + if (!owningAvatarID.isNull() && owningAvatarID == Physics::getSessionUUID()) { + _owningAvatarID = AVATAR_SELF_ID; + } else { + _owningAvatarID = owningAvatarID; + } +} + void EntityItem::addGrab(GrabPointer grab) { enableNoBootstrap(); SpatiallyNestable::addGrab(grab); diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 3274379ee9..bdccd466cb 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -514,7 +514,8 @@ public: // if this entity is an avatar entity, which avatar is it associated with? QUuid getOwningAvatarID() const { return _owningAvatarID; } - virtual void setOwningAvatarID(const QUuid& owningAvatarID) { _owningAvatarID = owningAvatarID; } + QUuid getOwningAvatarIDForProperties() const; + void setOwningAvatarID(const QUuid& owningAvatarID); virtual bool wantsHandControllerPointerEvents() const { return false; } virtual bool wantsKeyboardFocus() const { return false; } diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 7cfdc8a68d..803c1ac4ca 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -480,17 +480,11 @@ QUuid EntityScriptingInterface::addEntityInternal(const EntityItemProperties& pr _activityTracking.addedEntityCount++; - auto nodeList = DependencyManager::get(); - auto sessionID = nodeList->getSessionUUID(); - EntityItemProperties propertiesWithSimID = properties; propertiesWithSimID.setEntityHostType(entityHostType); if (entityHostType == entity::HostType::AVATAR) { - if (sessionID.isNull()) { - // null sessionID is unacceptable in this case - sessionID = AVATAR_SELF_ID; - } - propertiesWithSimID.setOwningAvatarID(sessionID); + // only allow adding our own avatar entities from script + propertiesWithSimID.setOwningAvatarID(AVATAR_SELF_ID); } else if (entityHostType == entity::HostType::LOCAL) { // For now, local entities are always collisionless // TODO: create a separate, local physics simulation that just handles local entities (and MyAvatar?) @@ -498,6 +492,8 @@ QUuid EntityScriptingInterface::addEntityInternal(const EntityItemProperties& pr } // the created time will be set in EntityTree::addEntity by recordCreationTime() + auto nodeList = DependencyManager::get(); + auto sessionID = nodeList->getSessionUUID(); propertiesWithSimID.setLastEditedBy(sessionID); bool scalesWithParent = propertiesWithSimID.getScalesWithParent(); @@ -805,7 +801,7 @@ QUuid EntityScriptingInterface::editEntity(const QUuid& id, const EntityItemProp return; } - if (entity->isAvatarEntity() && entity->getOwningAvatarID() != sessionID && entity->getOwningAvatarID() != AVATAR_SELF_ID) { + if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) { // don't edit other avatar's avatarEntities properties = EntityItemProperties(); return; @@ -978,10 +974,9 @@ void EntityScriptingInterface::deleteEntity(const QUuid& id) { _entityTree->withWriteLock([&] { EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); if (entity) { - auto nodeList = DependencyManager::get(); const QUuid myNodeID = nodeList->getSessionUUID(); - if (entity->isAvatarEntity() && entity->getOwningAvatarID() != myNodeID) { + if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) { // don't delete other avatar's avatarEntities shouldSendDeleteToServer = false; return; @@ -990,7 +985,7 @@ void EntityScriptingInterface::deleteEntity(const QUuid& id) { if (entity->getLocked()) { shouldSendDeleteToServer = false; } else { - // only delete local entities, server entities will round trip through the server filters + // only delete non-domain entities, domain entities will round trip through the server filters if (!entity->isDomainEntity() || _entityTree->isServerlessMode()) { shouldSendDeleteToServer = false; _entityTree->deleteEntity(entityID); @@ -1671,7 +1666,7 @@ bool EntityScriptingInterface::actionWorker(const QUuid& entityID, return; } - if (entity->isAvatarEntity() && entity->getOwningAvatarID() != myNodeID) { + if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) { return; } diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 6c12c6d019..6440f39f52 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -99,7 +99,7 @@ void EntityTree::eraseDomainAndNonOwnedEntities() { element->cleanupDomainAndNonOwnedEntities(); } - if (entity->isLocalEntity() || (entity->isAvatarEntity() && entity->getOwningAvatarID() == getMyAvatarSessionUUID())) { + if (entity->isLocalEntity() || entity->isMyAvatarEntity()) { savedEntities[entity->getEntityItemID()] = entity; } else { int32_t spaceIndex = entity->getSpaceIndex(); @@ -121,7 +121,7 @@ void EntityTree::eraseDomainAndNonOwnedEntities() { foreach (EntityItemWeakPointer entityItem, _needsParentFixup) { auto entity = entityItem.lock(); - if (entity && (entity->isLocalEntity() || (entity->isAvatarEntity() && entity->getOwningAvatarID() == getMyAvatarSessionUUID()))) { + if (entity && (entity->isLocalEntity() || entity->isMyAvatarEntity())) { needParentFixup.push_back(entityItem); } } @@ -688,6 +688,7 @@ void EntityTree::deleteEntities(QSet entityIDs, bool force, bool i // NOTE: callers must lock the tree before using this method DeleteEntityOperator theOperator(getThisPointer()); foreach(const EntityItemID& entityID, entityIDs) { + // ANDREW TODO: rejigger this logic to FIRST get entity THEN get element EntityTreeElementPointer containingElement = getContainingElement(entityID); if (!containingElement) { if (!ignoreWarnings) { diff --git a/libraries/entities/src/EntityTreeElement.cpp b/libraries/entities/src/EntityTreeElement.cpp index 60eaafc0dd..0096319081 100644 --- a/libraries/entities/src/EntityTreeElement.cpp +++ b/libraries/entities/src/EntityTreeElement.cpp @@ -705,7 +705,7 @@ void EntityTreeElement::cleanupDomainAndNonOwnedEntities() { withWriteLock([&] { EntityItems savedEntities; foreach(EntityItemPointer entity, _entityItems) { - if (!(entity->isLocalEntity() || (entity->isAvatarEntity() && entity->getOwningAvatarID() == getTree()->getMyAvatarSessionUUID()))) { + if (!(entity->isLocalEntity() || entity->isMyAvatarEntity())) { entity->preDelete(); entity->_element = NULL; } else { diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index e48f0603bd..2ec2eff7f9 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -62,7 +62,8 @@ EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItemPointer // rather than pass the legit shape pointer to the ObjectMotionState ctor above. setShape(shape); - if (_entity->isAvatarEntity() && _entity->getOwningAvatarID() != Physics::getSessionUUID()) { + // ANDREW TODO: maybe all non-domain entities should be unownable? or our avatar entities should have a special OwnershipState + if (_entity->isAvatarEntity() && !_entity->isMyAvatarEntity()) { // avatar entities are always thus, so we cache this fact in _ownershipState _ownershipState = EntityMotionState::OwnershipState::Unownable; } @@ -407,8 +408,8 @@ bool EntityMotionState::shouldSendUpdate(uint32_t simulationStep) { // NOTE: we expect _entity and _body to be valid in this context, since shouldSendUpdate() is only called // after doesNotNeedToSendUpdate() returns false and that call should return 'true' if _entity or _body are NULL. - // this case is prevented by setting _ownershipState to UNOWNABLE in EntityMotionState::ctor - assert(!(_entity->isAvatarEntity() && _entity->getOwningAvatarID() != Physics::getSessionUUID())); + // this case is prevented by setting _ownershipState to OwnershipState::Unownable in EntityMotionState::ctor + assert(!(_entity->isAvatarEntity() && !_entity->isMyAvatarEntity())); if (_entity->getTransitingWithAvatar()) { return false; @@ -747,6 +748,7 @@ bool EntityMotionState::shouldSendBid() const { // NOTE: this method is only ever called when the entity's simulation is NOT locally owned return _body->isActive() && (_region == workload::Region::R1) + // ANDREW TODO: make sure _ownershipState to Unownable on creation when applicable && _ownershipState != EntityMotionState::OwnershipState::Unownable && glm::max(glm::max(VOLUNTEER_SIMULATION_PRIORITY, _bumpedPriority), _entity->getScriptSimulationPriority()) >= _entity->getSimulationPriority() && !_entity->getLocked() @@ -768,6 +770,7 @@ uint8_t EntityMotionState::computeFinalBidPriority() const { } bool EntityMotionState::isLocallyOwned() const { + // ANDREW TODO: maybe also always return true for MyAvatar's avatar entities? return _entity->getSimulatorID() == Physics::getSessionUUID(); } @@ -795,6 +798,12 @@ void EntityMotionState::initForOwned() { _ownershipState = EntityMotionState::OwnershipState::LocallyOwned; } +void EntityMotionState::clearOwnershipState() { + if (_ownershipState != OwnershipState::Unownable) { + _ownershipState = OwnershipState::NotLocallyOwned; + } +} + void EntityMotionState::clearObjectVelocities() const { // If transform or velocities are flagged as dirty it means a network or scripted change // occured between the beginning and end of the stepSimulation() and we DON'T want to apply diff --git a/libraries/physics/src/EntityMotionState.h b/libraries/physics/src/EntityMotionState.h index 7456837777..be6f5c0658 100644 --- a/libraries/physics/src/EntityMotionState.h +++ b/libraries/physics/src/EntityMotionState.h @@ -107,7 +107,7 @@ protected: uint64_t getNextBidExpiry() const { return _nextBidExpiry; } void initForBid(); void initForOwned(); - void clearOwnershipState() { _ownershipState = OwnershipState::NotLocallyOwned; } + void clearOwnershipState(); void updateServerPhysicsVariables(); bool remoteSimulationOutOfSync(uint32_t simulationStep);