Merge pull request #10365 from sethalves/fix-dont-send-non-entity-edits-over-wire

Fix -- dont send non-entity edits to entity-server

interface/src/ui/overlays/Base3DOverlay.cpp

@@ -81,6 +81,10 @@ QVariantMap convertOverlayLocationFromScriptSemantics(const QVariantMap& propert
 void Base3DOverlay::setProperties(const QVariantMap& originalProperties) {
     QVariantMap properties = originalProperties;
 
+    if (properties["name"].isValid()) {
+        setName(properties["name"].toString());
+    }
+
     // carry over some legacy keys
     if (!properties["position"].isValid() && !properties["localPosition"].isValid()) {
         if (properties["p1"].isValid()) {
@@ -207,6 +211,9 @@ void Base3DOverlay::setProperties(const QVariantMap& originalProperties) {
 }
 
 QVariant Base3DOverlay::getProperty(const QString& property) {
+    if (property == "name") {
+        return _name;
+    }
     if (property == "position" || property == "start" || property == "p1" || property == "point") {
         return vec3toVariant(getPosition());
     }

interface/src/ui/overlays/Base3DOverlay.h

@@ -26,6 +26,9 @@ public:
     virtual OverlayID getOverlayID() const override { return OverlayID(getID().toString()); }
     void setOverlayID(OverlayID overlayID) override { setID(overlayID); }
 
+    virtual QString getName() const override { return QString("Overlay:") + _name; }
+    void setName(QString name) { _name = name; }
+
     // getters
     virtual bool is3D() const override { return true; }
 
@@ -74,6 +77,8 @@ protected:
     bool _drawInFront;
     bool _isAA;
     bool _isGrabbable { false };
+
+    QString _name;
 };
 
 #endif // hifi_Base3DOverlay_h

interface/src/ui/overlays/ModelOverlay.cpp

@@ -288,3 +288,10 @@ void ModelOverlay::locationChanged(bool tellPhysics) {
         _model->setTranslation(getPosition());
     }
 }
+
+QString ModelOverlay::getName() const {
+    if (_name != "") {
+        return QString("Overlay:") + getType() + ":" + _name;
+    }
+    return QString("Overlay:") + getType() + ":" + _url.toString();
+}

interface/src/ui/overlays/ModelOverlay.h

@@ -22,6 +22,8 @@ public:
     static QString const TYPE;
     virtual QString getType() const override { return TYPE; }
 
+    virtual QString getName() const override;
+
     ModelOverlay();
     ModelOverlay(const ModelOverlay* modelOverlay);
 

libraries/avatars/src/AvatarData.h

@@ -357,6 +357,8 @@ class AvatarData : public QObject, public SpatiallyNestable {
 
 public:
 
+    virtual QString getName() const override { return QString("Avatar:") + _displayName; }
+
     static const QString FRAME_NAME;
 
     static void fromFrame(const QByteArray& frameData, AvatarData& avatar, bool useFrameSkeleton = true);

libraries/entities/src/EntityItem.h

@@ -281,7 +281,7 @@ public:
     float getAngularDamping() const;
     void setAngularDamping(float value);
 
-    QString getName() const;
+    virtual QString getName() const override;
     void setName(const QString& value);
     QString getDebugName();
 

libraries/entities/src/EntityScriptingInterface.cpp

@@ -407,9 +407,11 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties&
     //     return QUuid();
     // }
 
+    bool entityFound { false };
     _entityTree->withReadLock([&] {
         EntityItemPointer 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 hasTerseUpdateChanges = properties.hasTerseUpdateChanges();
@@ -464,6 +466,27 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, const EntityItemProperties&
             });
         }
     });
+    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<SpatialParentFinder> parentFinder = DependencyManager::get<SpatialParentFinder>();
+        if (parentFinder) {
+            bool success;
+            auto nestableWP = parentFinder->find(id, success, static_cast<SpatialParentTree*>(_entityTree.get()));
+            if (success) {
+                auto nestable = nestableWP.lock();
+                if (nestable) {
+                    NestableType nestableType = nestable->getNestableType();
+                    if (nestableType == NestableType::Overlay || nestableType == NestableType::Avatar) {
+                        qCWarning(entities) << "attempted edit on non-entity: " << id << nestable->getName();
+                        return QUuid(); // null UUID to indicate failure
+                    }
+                }
+            }
+        }
+    }
+    // we queue edit packets even if we don't know about the entity.  This is to allow AC agents
+    // to edit entities they know only by ID.
     queueEntityMessage(PacketType::EntityEdit, entityID, properties);
     return id;
 }
@@ -1515,6 +1538,24 @@ bool EntityScriptingInterface::isChildOfParent(QUuid childID, QUuid parentID) {
     return isChild;
 }
 
+QString EntityScriptingInterface::getNestableType(QUuid id) {
+    QSharedPointer<SpatialParentFinder> parentFinder = DependencyManager::get<SpatialParentFinder>();
+    if (!parentFinder) {
+        return "unknown";
+    }
+    bool success;
+    SpatiallyNestableWeakPointer objectWP = parentFinder->find(id, success);
+    if (!success) {
+        return "unknown";
+    }
+    SpatiallyNestablePointer object = objectWP.lock();
+    if (!object) {
+        return "unknown";
+    }
+    NestableType nestableType = object->getNestableType();
+    return SpatiallyNestable::nestableTypeToString(nestableType);
+}
+
 QVector<QUuid> EntityScriptingInterface::getChildrenIDsOfJoint(const QUuid& parentID, int jointIndex) {
     QVector<QUuid> result;
     if (!_entityTree) {

libraries/entities/src/EntityScriptingInterface.h

@@ -304,6 +304,8 @@ public slots:
     Q_INVOKABLE QVector<QUuid> getChildrenIDsOfJoint(const QUuid& parentID, int jointIndex);
     Q_INVOKABLE bool isChildOfParent(QUuid childID, QUuid parentID);
 
+    Q_INVOKABLE QString getNestableType(QUuid id);
+
     Q_INVOKABLE QUuid getKeyboardFocusEntity() const;
     Q_INVOKABLE void setKeyboardFocusEntity(QUuid id);
 

libraries/entities/src/EntityTree.cpp

@@ -990,6 +990,17 @@ int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned c
                                                                                 entityItemID, properties);
             endDecode = usecTimestampNow();
 
+            EntityItemPointer existingEntity;
+            if (!isAdd) {
+                // search for the entity by EntityItemID
+                startLookup = usecTimestampNow();
+                existingEntity = findEntityByEntityItemID(entityItemID);
+                endLookup = usecTimestampNow();
+                if (!existingEntity) {
+                    // this is not an add-entity operation, and we don't know about the identified entity.
+                    validEditPacket = false;
+                }
+            }
 
             if (validEditPacket && !_entityScriptSourceWhitelist.isEmpty() && !properties.getScript().isEmpty()) {
                 bool passedWhiteList = false;
@@ -1036,12 +1047,6 @@ int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned c
             // If we got a valid edit packet, then it could be a new entity or it could be an update to
             // an existing entity... handle appropriately
             if (validEditPacket) {
-
-                // search for the entity by EntityItemID
-                startLookup = usecTimestampNow();
-                EntityItemPointer existingEntity = findEntityByEntityItemID(entityItemID);
-                endLookup = usecTimestampNow();
-                
                 startFilter = usecTimestampNow();
                 bool wasChanged = false;
                 // Having (un)lock rights bypasses the filter, unless it's a physics result.

libraries/shared/src/SpatiallyNestable.cpp

@@ -1138,3 +1138,17 @@ SpatiallyNestablePointer SpatiallyNestable::findByID(QUuid id, bool& success) {
     }
     return parentWP.lock();
 }
+
+
+QString SpatiallyNestable::nestableTypeToString(NestableType nestableType) {
+    switch(nestableType) {
+        case NestableType::Entity:
+            return "entity";
+        case NestableType::Avatar:
+            return "avatar";
+        case NestableType::Overlay:
+            return "overlay";
+        default:
+            return "unknown";
+    }
+}

libraries/shared/src/SpatiallyNestable.h

@@ -42,6 +42,8 @@ public:
     virtual const QUuid getID() const;
     virtual void setID(const QUuid& id);
 
+    virtual QString getName() const { return "SpatiallyNestable"; }
+
     virtual const QUuid getParentID() const;
     virtual void setParentID(const QUuid& parentID);
 
@@ -62,6 +64,8 @@ public:
     static glm::vec3 localToWorldAngularVelocity(const glm::vec3& angularVelocity,
                                                  const QUuid& parentID, int parentJointIndex, bool& success);
 
+    static QString nestableTypeToString(NestableType nestableType);
+
     // world frame
     virtual const Transform getTransform(bool& success, int depth = 0) const;
     virtual const Transform getTransform() const;

scripts/system/controllers/handControllerGrab.js

@@ -3881,6 +3881,7 @@ function MyController(hand) {
                 // we appear to be holding something and this script isn't in a state that would be holding something.
                 // unhook it.  if we previously took note of this entity's parent, put it back where it was.  This
                 // works around some problems that happen when more than one hand or avatar is passing something around.
+                var childType = Entities.getNestableType(childID);
                 if (_this.previousParentID[childID]) {
                     var previousParentID = _this.previousParentID[childID];
                     var previousParentJointIndex = _this.previousParentJointIndex[childID];
@@ -3898,7 +3899,7 @@ function MyController(hand) {
                     }
                     _this.previouslyUnhooked[childID] = now;
 
-                    if (Overlays.getProperty(childID, "grabbable")) {
+                    if (childType == "overlay" && Overlays.getProperty(childID, "grabbable")) {
                         // only auto-unhook overlays that were flagged as grabbable.  this avoids unhooking overlays
                         // used in tutorial.
                         Overlays.editOverlay(childID, {
@@ -3906,12 +3907,20 @@ function MyController(hand) {
                             parentJointIndex: previousParentJointIndex
                         });
                     }
-                    Entities.editEntity(childID, { parentID: previousParentID, parentJointIndex: previousParentJointIndex });
+                    if (childType == "entity") {
+                        Entities.editEntity(childID, {
+                            parentID: previousParentID,
+                            parentJointIndex: previousParentJointIndex
+                        });
+                    }
 
                 } else {
-                    Entities.editEntity(childID, { parentID: NULL_UUID });
-                    if (Overlays.getProperty(childID, "grabbable")) {
-                        Overlays.editOverlay(childID, { parentID: NULL_UUID });
+                    if (childType == "entity") {
+                        Entities.editEntity(childID, { parentID: NULL_UUID });
+                    } else if (childType == "overlay") {
+                        if (Overlays.getProperty(childID, "grabbable")) {
+                            Overlays.editOverlay(childID, { parentID: NULL_UUID });
+                        }
                     }
                 }
             }

scripts/system/libraries/WebTablet.js

@@ -275,7 +275,8 @@ WebTablet.prototype.getLocation = function() {
 };
 
 WebTablet.prototype.setHomeButtonTexture = function() {
-    Entities.editEntity(this.tabletEntityID, {textures: JSON.stringify({"tex.close": HOME_BUTTON_TEXTURE})});
+    // TODO - is this still needed?
+    // Entities.editEntity(this.tabletEntityID, {textures: JSON.stringify({"tex.close": HOME_BUTTON_TEXTURE})});
 };
 
 WebTablet.prototype.setURL = function (url) {
@@ -338,7 +339,8 @@ WebTablet.prototype.geometryChanged = function (geometry) {
 
         // compute position, rotation & parentJointIndex of the tablet
         this.calculateTabletAttachmentProperties(NO_HANDS, false, tabletProperties);
-        Entities.editEntity(this.tabletEntityID, tabletProperties);
+        // TODO -- is this still needed?
+        // Entities.editEntity(this.tabletEntityID, tabletProperties);
     }
 };
 
@@ -439,7 +441,8 @@ WebTablet.prototype.onHmdChanged = function () {
     var tabletProperties = {};
     // compute position, rotation & parentJointIndex of the tablet
     this.calculateTabletAttachmentProperties(NO_HANDS, false, tabletProperties);
-    Entities.editEntity(this.tabletEntityID, tabletProperties);
+    // TODO -- is this still needed?
+    // Entities.editEntity(this.tabletEntityID, tabletProperties);
 
     // Full scene FXAA should be disabled on the overlay when the tablet in desktop mode.
     // This should make the text more readable.
@@ -530,7 +533,8 @@ WebTablet.prototype.cameraModeChanged = function (newMode) {
         var tabletProperties = {};
         // compute position, rotation & parentJointIndex of the tablet
         self.calculateTabletAttachmentProperties(NO_HANDS, false, tabletProperties);
-        Entities.editEntity(self.tabletEntityID, tabletProperties);
+        // TODO -- is this still needed?
+        // Entities.editEntity(self.tabletEntityID, tabletProperties);
     }
 };