From 5e2f7204b45539b23d3fbd79f3b390f24da6aabd Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Wed, 1 Jul 2015 10:29:42 -0700 Subject: [PATCH] responses to code review --- assignment-client/src/AssignmentAction.cpp | 46 +++++++++++++++++++++ assignment-client/src/AssignmentAction.h | 20 ++++----- interface/src/InterfaceActionFactory.cpp | 6 +-- interface/src/devices/TV3DManager.cpp | 1 - interface/src/ui/ApplicationCompositor.cpp | 1 - interface/src/ui/AvatarInputs.cpp | 1 - interface/src/ui/Stats.cpp | 14 ------- libraries/entities/src/EntityItem.cpp | 43 +++++++++---------- libraries/entities/src/EntityItem.h | 6 +-- libraries/entities/src/SimulationOwner.h | 2 + libraries/networking/src/PacketHeaders.cpp | 2 +- libraries/networking/src/PacketHeaders.h | 3 +- libraries/physics/src/EntityMotionState.cpp | 14 +++---- libraries/physics/src/EntityMotionState.h | 4 +- libraries/physics/src/ObjectAction.h | 2 +- libraries/shared/src/QVariantGLM.h | 4 +- 16 files changed, 100 insertions(+), 69 deletions(-) diff --git a/assignment-client/src/AssignmentAction.cpp b/assignment-client/src/AssignmentAction.cpp index 9b2cf94ba2..6cb3c06312 100644 --- a/assignment-client/src/AssignmentAction.cpp +++ b/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."; +} diff --git a/assignment-client/src/AssignmentAction.h b/assignment-client/src/AssignmentAction.h index b49e8aa609..42c9fedecc 100644 --- a/assignment-client/src/AssignmentAction.h +++ b/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 QVariantMap getArguments() { assert(false); return QVariantMap(); } + virtual bool updateArguments(QVariantMap arguments); + 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 void setPosition(glm::vec3 position) { assert(false); } - virtual glm::quat getRotation() { assert(false); return glm::quat(); } - virtual void setRotation(glm::quat rotation) { assert(false); } - virtual glm::vec3 getLinearVelocity() { assert(false); return glm::vec3(0.0f); } - virtual void setLinearVelocity(glm::vec3 linearVelocity) { assert(false); } - virtual glm::vec3 getAngularVelocity() { assert(false); return glm::vec3(0.0f); } - virtual void setAngularVelocity(glm::vec3 angularVelocity) { assert(false); } + virtual glm::vec3 getPosition(); + virtual void setPosition(glm::vec3 position); + virtual glm::quat getRotation(); + virtual void setRotation(glm::quat rotation); + virtual glm::vec3 getLinearVelocity(); + virtual void setLinearVelocity(glm::vec3 linearVelocity); + virtual glm::vec3 getAngularVelocity(); + virtual void setAngularVelocity(glm::vec3 angularVelocity); bool _active; EntityItemWeakPointer _ownerEntity; diff --git a/interface/src/InterfaceActionFactory.cpp b/interface/src/InterfaceActionFactory.cpp index 8b47b597fa..02f8c8728d 100644 --- a/interface/src/InterfaceActionFactory.cpp +++ b/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; - ds >> id; + serializedArgumentStream >> type; + serializedArgumentStream >> id; EntityActionPointer action = interfaceActionFactory(type, id, ownerEntity); diff --git a/interface/src/devices/TV3DManager.cpp b/interface/src/devices/TV3DManager.cpp index b00e9f74f4..e945b5e79a 100644 --- a/interface/src/devices/TV3DManager.cpp +++ b/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(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); diff --git a/interface/src/ui/ApplicationCompositor.cpp b/interface/src/ui/ApplicationCompositor.cpp index f6fc86cff0..4c3a2a44c1 100644 --- a/interface/src/ui/ApplicationCompositor.cpp +++ b/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); } diff --git a/interface/src/ui/AvatarInputs.cpp b/interface/src/ui/AvatarInputs.cpp index 42d9f75403..019deb9690 100644 --- a/interface/src/ui/AvatarInputs.cpp +++ b/interface/src/ui/AvatarInputs.cpp @@ -67,7 +67,6 @@ void AvatarInputs::update() { AI_UPDATE(cameraMuted, Menu::getInstance()->isOptionChecked(MenuOption::MuteFaceTracking)); auto audioIO = DependencyManager::get(); - // 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; diff --git a/interface/src/ui/Stats.cpp b/interface/src/ui/Stats.cpp index 1bfc4c8d61..93515929cc 100644 --- a/interface/src/ui/Stats.cpp +++ b/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); diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index d809ac4cd4..09672b07c4 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/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; - if (actionData.size() == 0) { + _serializedActions = actionData; + if (_serializedActions.size() == 0) { return; } QVector serializedActions; - QDataStream ds(actionData); - ds >> serializedActions; + QDataStream serializedActionsStream(actionData); + serializedActionsStream >> serializedActions; // Keep track of which actions got added or updated by the new actionData QSet updated; foreach(QByteArray serializedAction, serializedActions) { - QDataStream dsForAction(serializedAction); + QDataStream serializedActionStream(serializedAction); EntityActionType actionType; QUuid actionID; - dsForAction >> actionType; - dsForAction >> actionID; + serializedActionStream >> actionType; + 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); - ds << serializedActions; + QDataStream serializedActionsStream(&result, QIODevice::WriteOnly); + serializedActionsStream << serializedActions; - if (result.size() >= _maxActionDataSize) { + if (result.size() >= _maxActionsDataSize) { return false; } - _actionData = result; + _serializedActions = result; return true; } const QByteArray EntityItem::getActionData() const { - serializeActionData(); - return _actionData; + serializeActions(); + return _serializedActions; } QVariantMap EntityItem::getActionArguments(const QUuid& actionID) { diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 880a666d3f..aedcc0f8eb 100644 --- a/libraries/entities/src/EntityItem.h +++ b/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 _objectActions; - static int _maxActionDataSize; - mutable QByteArray _actionData; + static int _maxActionsDataSize; + mutable QByteArray _serializedActions; }; #endif // hifi_EntityItem_h diff --git a/libraries/entities/src/SimulationOwner.h b/libraries/entities/src/SimulationOwner.h index 325be54e62..a848ad6e84 100644 --- a/libraries/entities/src/SimulationOwner.h +++ b/libraries/entities/src/SimulationOwner.h @@ -18,6 +18,8 @@ #include #include +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. diff --git a/libraries/networking/src/PacketHeaders.cpp b/libraries/networking/src/PacketHeaders.cpp index 1a579cb6c9..42ee9f3025 100644 --- a/libraries/networking/src/PacketHeaders.cpp +++ b/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: diff --git a/libraries/networking/src/PacketHeaders.h b/libraries/networking/src/PacketHeaders.h index e30daac328..0283ba3796 100644 --- a/libraries/networking/src/PacketHeaders.h +++ b/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 = 34; +const PacketVersion VERSION_ENTITIES_HAVE_SIMULATION_OWNER_AND_ACTIONS_OVER_WIRE = 33; #endif // hifi_PacketHeaders_h diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index 3c2111fca0..a975d21c4d 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/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 diff --git a/libraries/physics/src/EntityMotionState.h b/libraries/physics/src/EntityMotionState.h index 3f427d42e2..009c931dba 100644 --- a/libraries/physics/src/EntityMotionState.h +++ b/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 diff --git a/libraries/physics/src/ObjectAction.h b/libraries/physics/src/ObjectAction.h index 8119657abe..0e982aaacf 100644 --- a/libraries/physics/src/ObjectAction.h +++ b/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; } diff --git a/libraries/shared/src/QVariantGLM.h b/libraries/shared/src/QVariantGLM.h index cef625465f..922aa7c4aa 100644 --- a/libraries/shared/src/QVariantGLM.h +++ b/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::quat& g); +QVariantMap glmToQMap(const glm::vec3& glmVector); +QVariantMap glmToQMap(const glm::quat& glmQuat); glm::vec3 qListToGlmVec3(const QVariant q); glm::quat qListToGlmQuat(const QVariant q);