responses to code review

assignment-client/src/AssignmentAction.cpp

@@ -35,3 +35,49 @@ QByteArray AssignmentAction::serialize() {
 void AssignmentAction::deserialize(QByteArray serializedArguments) {
     _data = serializedArguments;
 }
+
+bool AssignmentAction::updateArguments(QVariantMap arguments) {
+    qDebug() << "UNEXPECTED -- AssignmentAction::updateArguments called in assignment-client.";
+    return false;
+}
+
+QVariantMap AssignmentAction::getArguments() {
+    qDebug() << "UNEXPECTED -- AssignmentAction::getArguments called in assignment-client.";
+    return QVariantMap();
+}
+
+glm::vec3 AssignmentAction::getPosition() {
+    qDebug() << "UNEXPECTED -- AssignmentAction::getPosition called in assignment-client.";
+    return glm::vec3(0.0f);
+}
+
+void AssignmentAction::setPosition(glm::vec3 position) {
+    qDebug() << "UNEXPECTED -- AssignmentAction::setPosition called in assignment-client.";
+}
+
+glm::quat AssignmentAction::getRotation() {
+    qDebug() << "UNEXPECTED -- AssignmentAction::getRotation called in assignment-client.";
+    return glm::quat();
+}
+
+void AssignmentAction::setRotation(glm::quat rotation) {
+    qDebug() << "UNEXPECTED -- AssignmentAction::setRotation called in assignment-client.";
+}
+
+glm::vec3 AssignmentAction::getLinearVelocity() {
+    qDebug() << "UNEXPECTED -- AssignmentAction::getLinearVelocity called in assignment-client.";
+    return glm::vec3(0.0f);
+}
+
+void AssignmentAction::setLinearVelocity(glm::vec3 linearVelocity) {
+    qDebug() << "UNEXPECTED -- AssignmentAction::setLinearVelocity called in assignment-client.";
+}
+
+glm::vec3 AssignmentAction::getAngularVelocity() {
+    qDebug() << "UNEXPECTED -- AssignmentAction::getAngularVelocity called in assignment-client.";
+    return glm::vec3(0.0f);
+}
+
+void AssignmentAction::setAngularVelocity(glm::vec3 angularVelocity) {
+    qDebug() << "UNEXPECTED -- AssignmentAction::setAngularVelocity called in assignment-client.";
+}

assignment-client/src/AssignmentAction.h

@@ -29,8 +29,8 @@ public:
     virtual void removeFromSimulation(EntitySimulation* simulation) const;
     virtual const EntityItemWeakPointer getOwnerEntity() const { return _ownerEntity; }
     virtual void setOwnerEntity(const EntityItemPointer ownerEntity) { _ownerEntity = ownerEntity; }
-    virtual bool updateArguments(QVariantMap arguments) { assert(false); return false; }
+    virtual bool updateArguments(QVariantMap arguments);
-    virtual QVariantMap getArguments() { assert(false); return QVariantMap(); }
+    virtual QVariantMap getArguments();
 
     virtual QByteArray serialize();
     virtual void deserialize(QByteArray serializedArguments);
@@ -41,14 +41,14 @@ private:
     QByteArray _data;
 
 protected:
-    virtual glm::vec3 getPosition() { assert(false); return glm::vec3(0.0f); }
+    virtual glm::vec3 getPosition();
-    virtual void setPosition(glm::vec3 position) { assert(false); }
+    virtual void setPosition(glm::vec3 position);
-    virtual glm::quat getRotation() { assert(false); return glm::quat(); }
+    virtual glm::quat getRotation();
-    virtual void setRotation(glm::quat rotation) { assert(false); }
+    virtual void setRotation(glm::quat rotation);
-    virtual glm::vec3 getLinearVelocity() { assert(false); return glm::vec3(0.0f); }
+    virtual glm::vec3 getLinearVelocity();
-    virtual void setLinearVelocity(glm::vec3 linearVelocity) { assert(false); }
+    virtual void setLinearVelocity(glm::vec3 linearVelocity);
-    virtual glm::vec3 getAngularVelocity() { assert(false); return glm::vec3(0.0f); }
+    virtual glm::vec3 getAngularVelocity();
-    virtual void setAngularVelocity(glm::vec3 angularVelocity) { assert(false); }
+    virtual void setAngularVelocity(glm::vec3 angularVelocity);
 
     bool _active;
     EntityItemWeakPointer _ownerEntity;

interface/src/InterfaceActionFactory.cpp

@@ -55,12 +55,12 @@ EntityActionPointer InterfaceActionFactory::factory(EntitySimulation* simulation
 EntityActionPointer InterfaceActionFactory::factoryBA(EntitySimulation* simulation,
                                                       EntityItemPointer ownerEntity,
                                                       QByteArray data) {
-    QDataStream ds(data);
+    QDataStream serializedArgumentStream(data);
     EntityActionType type;
     QUuid id;
 
-    ds >> type;
+    serializedArgumentStream >> type;
-    ds >> id;
+    serializedArgumentStream >> id;
 
     EntityActionPointer action = interfaceActionFactory(type, id, ownerEntity);
 

interface/src/devices/TV3DManager.cpp

@@ -109,7 +109,6 @@ void TV3DManager::display(RenderArgs* renderArgs, Camera& whichCamera) {
         glScissor(portalX, portalY, portalW, portalH);
 
         glm::mat4 projection = glm::frustum<float>(eye.left, eye.right, eye.bottom, eye.top, nearZ, farZ);
-        // float fov = atanf(1.0f / projection[1][1]);
         projection = glm::translate(projection, vec3(eye.modelTranslation, 0, 0));
         eyeCamera.setProjection(projection);
 

interface/src/ui/ApplicationCompositor.cpp

@@ -234,7 +234,6 @@ void ApplicationCompositor::displayOverlayTexture(RenderArgs* renderArgs) {
     model.setScale(vec3(mouseSize, 1.0f));
     batch.setModelTransform(model);
     bindCursorTexture(batch);
-    // vec4 reticleColor = { RETICLE_COLOR[0], RETICLE_COLOR[1], RETICLE_COLOR[2], 1.0f };
     geometryCache->renderUnitQuad(batch, vec4(1));
     renderArgs->_context->render(batch);
 }

interface/src/ui/AvatarInputs.cpp

@@ -67,7 +67,6 @@ void AvatarInputs::update() {
     AI_UPDATE(cameraMuted, Menu::getInstance()->isOptionChecked(MenuOption::MuteFaceTracking));
 
     auto audioIO = DependencyManager::get<AudioClient>();
-    // const float CLIPPING_INDICATOR_TIME = 1.0f;
     const float AUDIO_METER_AVERAGING = 0.5;
     const float LOG2 = log(2.0f);
     const float METER_LOUDNESS_SCALE = 2.8f / 5.0f;

interface/src/ui/Stats.cpp

@@ -136,7 +136,6 @@ void Stats::updateStats() {
         unsigned long totalPingOctree = 0;
         int octreeServerCount = 0;
         int pingOctreeMax = 0;
-        // int pingVoxel;
         nodeList->eachNode([&](const SharedNodePointer& node) {
             // TODO: this should also support entities
             if (node->getType() == NodeType::EntityServer) {
@@ -147,19 +146,6 @@ void Stats::updateStats() {
                 }
             }
         });
-
-        // if (octreeServerCount) {
-        //     pingVoxel = totalPingOctree / octreeServerCount;
-        // }
-
-        //STAT_UPDATE(entitiesPing, pingVoxel);
-        //if (_expanded) {
-        //    QString voxelMaxPing;
-        //    if (pingVoxel >= 0) {  // Average is only meaningful if pingVoxel is valid.
-        //        voxelMaxPing = QString("Voxel max ping: %1").arg(pingOctreeMax);
-        //    } else {
-        //        voxelMaxPing = QString("Voxel max ping: --");
-        //    }
     } else {
         // -2 causes the QML to hide the ping column
         STAT_UPDATE(audioPing, -2);

libraries/entities/src/EntityItem.cpp

@@ -34,7 +34,7 @@ const quint64 DEFAULT_SIMULATOR_CHANGE_LOCKOUT_PERIOD = (quint64)(0.2f * USECS_P
 const quint64 MAX_SIMULATOR_CHANGE_LOCKOUT_PERIOD = 2 * USECS_PER_SECOND;
 
 bool EntityItem::_sendPhysicsUpdates = true;
-int EntityItem::_maxActionDataSize = 800;
+int EntityItem::_maxActionsDataSize = 800;
 
 EntityItem::EntityItem(const EntityItemID& entityItemID) :
     _type(EntityTypes::Unknown),
@@ -1421,7 +1421,7 @@ bool EntityItem::addAction(EntitySimulation* simulation, EntityActionPointer act
     _objectActions[actionID] = action;
 
     simulation->addAction(action);
-    bool success = serializeActionData();
+    bool success = serializeActions();
     if (!success) {
         removeAction(simulation, actionID);
     }
@@ -1435,7 +1435,7 @@ bool EntityItem::updateAction(EntitySimulation* simulation, const QUuid& actionI
     EntityActionPointer action = _objectActions[actionID];
     bool success = action->updateArguments(arguments);
     if (success) {
-        success = serializeActionData();
+        success = serializeActions();
     }
     return success;
 }
@@ -1446,7 +1446,7 @@ bool EntityItem::removeAction(EntitySimulation* simulation, const QUuid& actionI
         _objectActions.remove(actionID);
         action->setOwnerEntity(nullptr);
         action->removeFromSimulation(simulation);
-        return serializeActionData();
+        return serializeActions();
     }
     return false;
 }
@@ -1460,33 +1460,34 @@ bool EntityItem::clearActions(EntitySimulation* simulation) {
         action->setOwnerEntity(nullptr);
         action->removeFromSimulation(simulation);
     }
-    _actionData = QByteArray();
+    // empty _serializedActions means no actions for the EntityItem
+    _serializedActions = QByteArray();
     return true;
 }
 
 void EntityItem::setActionData(QByteArray actionData) {
     // it would be nice to take this shortcut, but a previous add may have failed
-    // if (_actionData == actionData) {
+    // if (_serializedActions == actionData) {
     //     return;
     // }
-    _actionData = actionData;
+    _serializedActions = actionData;
-    if (actionData.size() == 0) {
+    if (_serializedActions.size() == 0) {
         return;
     }
 
     QVector<QByteArray> serializedActions;
-    QDataStream ds(actionData);
+    QDataStream serializedActionsStream(actionData);
-    ds >> serializedActions;
+    serializedActionsStream >> serializedActions;
 
     // Keep track of which actions got added or updated by the new actionData
     QSet<QUuid> updated;
 
     foreach(QByteArray serializedAction, serializedActions) {
-        QDataStream dsForAction(serializedAction);
+        QDataStream serializedActionStream(serializedAction);
         EntityActionType actionType;
         QUuid actionID;
-        dsForAction >> actionType;
+        serializedActionStream >> actionType;
-        dsForAction >> actionID;
+        serializedActionStream >> actionID;
         updated << actionID;
 
         if (_objectActions.contains(actionID)) {
@@ -1526,9 +1527,9 @@ void EntityItem::setActionData(QByteArray actionData) {
     }
 }
 
-bool EntityItem::serializeActionData() const {
+bool EntityItem::serializeActions() const {
     if (_objectActions.size() == 0) {
-        _actionData = QByteArray();
+        _serializedActions = QByteArray();
         return true;
     }
 
@@ -1543,20 +1544,20 @@ bool EntityItem::serializeActionData() const {
     }
 
     QByteArray result;
-    QDataStream ds(&result, QIODevice::WriteOnly);
+    QDataStream serializedActionsStream(&result, QIODevice::WriteOnly);
-    ds << serializedActions;
+    serializedActionsStream << serializedActions;
 
-    if (result.size() >= _maxActionDataSize) {
+    if (result.size() >= _maxActionsDataSize) {
         return false;
     }
 
-    _actionData = result;
+    _serializedActions = result;
     return true;
 }
 
 const QByteArray EntityItem::getActionData() const {
-    serializeActionData();
+    serializeActions();
-    return _actionData;
+    return _serializedActions;
 }
 
 QVariantMap EntityItem::getActionArguments(const QUuid& actionID) {

libraries/entities/src/EntityItem.h

@@ -468,10 +468,10 @@ protected:
     void* _physicsInfo = nullptr; // set by EntitySimulation
     bool _simulated; // set by EntitySimulation
 
-    bool serializeActionData() const;
+    bool serializeActions() const;
     QHash<QUuid, EntityActionPointer> _objectActions;
-    static int _maxActionDataSize;
+    static int _maxActionsDataSize;
-    mutable QByteArray _actionData;
+    mutable QByteArray _serializedActions;
 };
 
 #endif // hifi_EntityItem_h

libraries/entities/src/SimulationOwner.h

@@ -18,6 +18,8 @@
 #include <SharedUtil.h>
 #include <UUID.h>
 
+const quint8 NO_PRORITY = 0x00;
+
 // Simulation observers will bid to simulate unowned active objects at the lowest possible priority
 // which is VOLUNTEER.  If the server accepts a VOLUNTEER bid it will automatically bump it 
 // to RECRUIT priority so that other volunteers don't accidentally take over.

libraries/networking/src/PacketHeaders.cpp

@@ -73,7 +73,7 @@ PacketVersion versionForPacketType(PacketType packetType) {
         case PacketTypeEntityAdd:
         case PacketTypeEntityEdit:
         case PacketTypeEntityData:
-            return VERSION_ENTITIES_HAVE_SIMULATION_OWNER;
+            return VERSION_ENTITIES_HAVE_SIMULATION_OWNER_AND_ACTIONS_OVER_WIRE;
         case PacketTypeEntityErase:
             return 2;
         case PacketTypeAudioStreamStats:

libraries/networking/src/PacketHeaders.h

@@ -186,7 +186,6 @@ const PacketVersion VERSION_ENTITIES_LINE_POINTS = 29;
 const PacketVersion VERSION_ENTITIES_FACE_CAMERA = 30;
 const PacketVersion VERSION_ENTITIES_SCRIPT_TIMESTAMP = 31;
 const PacketVersion VERSION_ENTITIES_SCRIPT_TIMESTAMP_FIX = 32;
-const PacketVersion VERSION_ACTIONS_OVER_WIRE = 33;
+const PacketVersion VERSION_ENTITIES_HAVE_SIMULATION_OWNER_AND_ACTIONS_OVER_WIRE = 33;
-const PacketVersion VERSION_ENTITIES_HAVE_SIMULATION_OWNER = 34;
 
 #endif // hifi_PacketHeaders_h

libraries/physics/src/EntityMotionState.cpp

@@ -113,12 +113,12 @@ void EntityMotionState::handleEasyChanges(uint32_t flags, PhysicsEngine* engine)
             flags &= ~EntityItem::DIRTY_PHYSICS_ACTIVATION;
             // hint to Bullet that the object is deactivating
             _body->setActivationState(WANTS_DEACTIVATION);
-            _outgoingPriority = 0;
+            _outgoingPriority = NO_PRORITY;
         } else  {
             _nextOwnershipBid = usecTimestampNow() + USECS_BETWEEN_OWNERSHIP_BIDS;
             if (engine->getSessionID() == _entity->getSimulatorID() || _entity->getSimulationPriority() > _outgoingPriority) {
                 // we own the simulation or our priority looses to remote
-                _outgoingPriority = 0;
+                _outgoingPriority = NO_PRORITY;
             }
         }
     }
@@ -238,7 +238,7 @@ bool EntityMotionState::isCandidateForOwnership(const QUuid& sessionID) const {
         return false;
     }
     assert(entityTreeIsLocked());
-    return _outgoingPriority > 0 || sessionID == _entity->getSimulatorID();
+    return _outgoingPriority != NO_PRORITY || sessionID == _entity->getSimulatorID();
 }
 
 bool EntityMotionState::remoteSimulationOutOfSync(uint32_t simulationStep) {
@@ -361,10 +361,10 @@ bool EntityMotionState::shouldSendUpdate(uint32_t simulationStep, const QUuid& s
 
     if (_entity->getSimulatorID() != sessionID) {
         // we don't own the simulation, but maybe we should...
-        if (_outgoingPriority > 0) {
+        if (_outgoingPriority != NO_PRORITY) {
             if (_outgoingPriority < _entity->getSimulationPriority()) {
                 // our priority looses to remote, so we don't bother to bid
-                _outgoingPriority = 0;
+                _outgoingPriority = NO_PRORITY;
                 return false;
             }
             return usecTimestampNow() > _nextOwnershipBid;
@@ -465,7 +465,7 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, const Q
             // we own the simulation but the entity has stopped, so we tell the server that we're clearing simulatorID
             // but we remember that we do still own it...  and rely on the server to tell us that we don't
             properties.clearSimulationOwner();
-            _outgoingPriority = 0;
+            _outgoingPriority = NO_PRORITY;
         }
         // else the ownership is not changing so we don't bother to pack it
     } else {
@@ -514,7 +514,7 @@ quint8 EntityMotionState::getSimulationPriority() const {
     if (_entity) {
         return _entity->getSimulationPriority();
     }
-    return 0;
+    return NO_PRORITY;
 }
 
 // virtual

libraries/physics/src/EntityMotionState.h

@@ -115,9 +115,9 @@ protected:
     float _measuredDeltaTime;
 
     quint8 _accelerationNearlyGravityCount;
-    quint64 _nextOwnershipBid = 0;
+    quint64 _nextOwnershipBid = NO_PRORITY;
     uint32_t _loopsWithoutOwner;
-    quint8 _outgoingPriority = 0;
+    quint8 _outgoingPriority = NO_PRORITY;
 };
 
 #endif // hifi_EntityMotionState_h

libraries/physics/src/ObjectAction.h

@@ -32,7 +32,7 @@ public:
     const QUuid& getID() const { return _id; }
     virtual EntityActionType getType() { assert(false); return ACTION_TYPE_NONE; }
     virtual void removeFromSimulation(EntitySimulation* simulation) const;
-    virtual const EntityItemWeakPointer getOwnerEntity() const { return _ownerEntity; }
+    virtual EntityItemWeakPointer getOwnerEntity() const { return _ownerEntity; }
     virtual void setOwnerEntity(const EntityItemPointer ownerEntity) { _ownerEntity = ownerEntity; }
 
     virtual bool updateArguments(QVariantMap arguments) { return false; }

libraries/shared/src/QVariantGLM.h

@@ -21,8 +21,8 @@ QVariantList glmToQList(const glm::vec3& g);
 QVariantList glmToQList(const glm::quat& g);
 QVariantList rgbColorToQList(rgbColor& v);
 
-QVariantMap glmToQMap(const glm::vec3& g);
+QVariantMap glmToQMap(const glm::vec3& glmVector);
-QVariantMap glmToQMap(const glm::quat& g);
+QVariantMap glmToQMap(const glm::quat& glmQuat);
 
 glm::vec3 qListToGlmVec3(const QVariant q);
 glm::quat qListToGlmQuat(const QVariant q);