From e8a6acd65b8e91294b0015bb671c53d85e0b9115 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Wed, 1 Jul 2015 10:42:04 -0700 Subject: [PATCH] responses to code review --- assignment-client/src/AssignmentAction.h | 2 +- .../entities/src/EntityActionInterface.h | 2 +- libraries/entities/src/EntityItem.cpp | 4 +- libraries/physics/src/PhysicsEngine.cpp | 58 ++++++++++--------- 4 files changed, 35 insertions(+), 31 deletions(-) diff --git a/assignment-client/src/AssignmentAction.h b/assignment-client/src/AssignmentAction.h index 42c9fedecc..cd72c1f277 100644 --- a/assignment-client/src/AssignmentAction.h +++ b/assignment-client/src/AssignmentAction.h @@ -27,7 +27,7 @@ public: const QUuid& getID() const { return _id; } virtual EntityActionType getType() { return _type; } 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); virtual QVariantMap getArguments(); diff --git a/libraries/entities/src/EntityActionInterface.h b/libraries/entities/src/EntityActionInterface.h index 599ddc35c2..5693e1fb6f 100644 --- a/libraries/entities/src/EntityActionInterface.h +++ b/libraries/entities/src/EntityActionInterface.h @@ -35,7 +35,7 @@ public: virtual EntityActionType getType() { assert(false); return ACTION_TYPE_NONE; } virtual void removeFromSimulation(EntitySimulation* simulation) const = 0; - virtual const EntityItemWeakPointer getOwnerEntity() const = 0; + virtual EntityItemWeakPointer getOwnerEntity() const = 0; virtual void setOwnerEntity(const EntityItemPointer ownerEntity) = 0; virtual bool updateArguments(QVariantMap arguments) = 0; virtual QVariantMap getArguments() = 0; diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 09672b07c4..b184a3cad6 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -541,7 +541,7 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef dataAt += propertyFlags.getEncodedLength(); bytesRead += propertyFlags.getEncodedLength(); - if (args.bitstreamVersion >= VERSION_ENTITIES_HAVE_SIMULATION_OWNER) { + if (args.bitstreamVersion >= VERSION_ENTITIES_HAVE_SIMULATION_OWNER_AND_ACTIONS_OVER_WIRE) { // pack SimulationOwner and terse update properties near each other // NOTE: the server is authoritative for changes to simOwnerID so we always unpack ownership data @@ -611,7 +611,7 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef READ_ENTITY_PROPERTY(PROP_LOCKED, bool, setLocked); READ_ENTITY_PROPERTY(PROP_USER_DATA, QString, setUserData); - if (args.bitstreamVersion < VERSION_ENTITIES_HAVE_SIMULATION_OWNER) { + if (args.bitstreamVersion < VERSION_ENTITIES_HAVE_SIMULATION_OWNER_AND_ACTIONS_OVER_WIRE) { // this code for when there is only simulatorID and no simulation priority // we always accept the server's notion of simulatorID, so we fake overwriteLocalData as true diff --git a/libraries/physics/src/PhysicsEngine.cpp b/libraries/physics/src/PhysicsEngine.cpp index f533bba7c5..022468633f 100644 --- a/libraries/physics/src/PhysicsEngine.cpp +++ b/libraries/physics/src/PhysicsEngine.cpp @@ -142,7 +142,7 @@ void PhysicsEngine::addObject(ObjectMotionState* motionState) { motionState->getAndClearIncomingDirtyFlags(); } - + void PhysicsEngine::removeObject(ObjectMotionState* object) { // wake up anything touching this object bump(object); @@ -172,7 +172,7 @@ void PhysicsEngine::deleteObjects(SetOfMotionStates& objects) { for (auto object : objects) { btRigidBody* body = object->getRigidBody(); removeObject(object); - + // NOTE: setRigidBody() modifies body->m_userPointer so we should clear the MotionState's body BEFORE deleting it. object->setRigidBody(nullptr); body->setMotionState(nullptr); @@ -263,22 +263,26 @@ void PhysicsEngine::doOwnershipInfection(const btCollisionObject* objectA, const const btCollisionObject* characterObject = _characterController ? _characterController->getCollisionObject() : nullptr; - ObjectMotionState* a = static_cast(objectA->getUserPointer()); - ObjectMotionState* b = static_cast(objectB->getUserPointer()); + ObjectMotionState* motionStateA = static_cast(objectA->getUserPointer()); + ObjectMotionState* motionStateB = static_cast(objectB->getUserPointer()); - if (b && ((a && a->getSimulatorID() == _sessionID && !objectA->isStaticObject()) || (objectA == characterObject))) { - // NOTE: we might own the simulation of a kinematic object (A) + if (motionStateB && + ((motionStateA && motionStateA->getSimulatorID() == _sessionID && !objectA->isStaticObject()) || + (objectA == characterObject))) { + // NOTE: we might own the simulation of a kinematic object (A) // but we don't claim ownership of kinematic objects (B) based on collisions here. - if (!objectB->isStaticOrKinematicObject() && b->getSimulatorID() != _sessionID) { - quint8 priority = a ? a->getSimulationPriority() : PERSONAL_SIMULATION_PRIORITY; - b->bump(priority); + if (!objectB->isStaticOrKinematicObject() && motionStateB->getSimulatorID() != _sessionID) { + quint8 priority = motionStateA ? motionStateA->getSimulationPriority() : PERSONAL_SIMULATION_PRIORITY; + motionStateB->bump(priority); } - } else if (a && ((b && b->getSimulatorID() == _sessionID && !objectB->isStaticObject()) || (objectB == characterObject))) { - // SIMILARLY: we might own the simulation of a kinematic object (B) + } else if (motionStateA && + ((motionStateB && motionStateB->getSimulatorID() == _sessionID && !objectB->isStaticObject()) || + (objectB == characterObject))) { + // SIMILARLY: we might own the simulation of a kinematic object (B) // but we don't claim ownership of kinematic objects (A) based on collisions here. - if (!objectA->isStaticOrKinematicObject() && a->getSimulatorID() != _sessionID) { - quint8 priority = b ? b->getSimulationPriority() : PERSONAL_SIMULATION_PRIORITY; - a->bump(priority); + if (!objectA->isStaticOrKinematicObject() && motionStateA->getSimulatorID() != _sessionID) { + quint8 priority = motionStateB ? motionStateB->getSimulationPriority() : PERSONAL_SIMULATION_PRIORITY; + motionStateA->bump(priority); } } } @@ -292,13 +296,13 @@ void PhysicsEngine::updateContactMap() { for (int i = 0; i < numManifolds; ++i) { btPersistentManifold* contactManifold = _collisionDispatcher->getManifoldByIndexInternal(i); if (contactManifold->getNumContacts() > 0) { - // TODO: require scripts to register interest in callbacks for specific objects + // TODO: require scripts to register interest in callbacks for specific objects // so we can filter out most collision events right here. const btCollisionObject* objectA = static_cast(contactManifold->getBody0()); const btCollisionObject* objectB = static_cast(contactManifold->getBody1()); if (!(objectA->isActive() || objectB->isActive())) { - // both objects are inactive so stop tracking this contact, + // both objects are inactive so stop tracking this contact, // which will eventually trigger a CONTACT_EVENT_TYPE_END continue; } @@ -328,24 +332,24 @@ CollisionEvents& PhysicsEngine::getCollisionEvents() { ContactInfo& contact = contactItr->second; ContactEventType type = contact.computeType(_numContactFrames); if(type != CONTACT_EVENT_TYPE_CONTINUE || _numSubsteps % CONTINUE_EVENT_FILTER_FREQUENCY == 0) { - ObjectMotionState* A = static_cast(contactItr->first._a); - ObjectMotionState* B = static_cast(contactItr->first._b); - glm::vec3 velocityChange = (A ? A->getObjectLinearVelocityChange() : glm::vec3(0.0f)) + - (B ? B->getObjectLinearVelocityChange() : glm::vec3(0.0f)); + ObjectMotionState* motionStateA = static_cast(contactItr->first._a); + ObjectMotionState* motionStateB = static_cast(contactItr->first._b); + glm::vec3 velocityChange = (motionStateA ? motionStateA->getObjectLinearVelocityChange() : glm::vec3(0.0f)) + + (motionStateB ? motionStateB->getObjectLinearVelocityChange() : glm::vec3(0.0f)); - if (A && A->getType() == MOTIONSTATE_TYPE_ENTITY) { - QUuid idA = A->getObjectID(); + if (motionStateA && motionStateA->getType() == MOTIONSTATE_TYPE_ENTITY) { + QUuid idA = motionStateA->getObjectID(); QUuid idB; - if (B && B->getType() == MOTIONSTATE_TYPE_ENTITY) { - idB = B->getObjectID(); + if (motionStateB && motionStateB->getType() == MOTIONSTATE_TYPE_ENTITY) { + idB = motionStateB->getObjectID(); } glm::vec3 position = bulletToGLM(contact.getPositionWorldOnB()) + _originOffset; glm::vec3 penetration = bulletToGLM(contact.distance * contact.normalWorldOnB); _collisionEvents.push_back(Collision(type, idA, idB, position, penetration, velocityChange)); - } else if (B && B->getType() == MOTIONSTATE_TYPE_ENTITY) { - QUuid idB = B->getObjectID(); + } else if (motionStateB && motionStateB->getType() == MOTIONSTATE_TYPE_ENTITY) { + QUuid idB = motionStateB->getObjectID(); glm::vec3 position = bulletToGLM(contact.getPositionWorldOnA()) + _originOffset; - // NOTE: we're flipping the order of A and B (so that the first objectID is never NULL) + // NOTE: we're flipping the order of A and B (so that the first objectID is never NULL) // hence we must negate the penetration. glm::vec3 penetration = - bulletToGLM(contact.distance * contact.normalWorldOnB); _collisionEvents.push_back(Collision(type, idB, QUuid(), position, penetration, velocityChange));