From cbcd70aee6c00f2a39b4d3757470f626256dd1c8 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 12 Dec 2018 18:52:26 -0800 Subject: [PATCH] namechanges and comments to help clarify necessary fixes --- interface/src/avatar/MyAvatar.cpp | 69 ++++++++++-------- interface/src/avatar/MyAvatar.h | 2 +- .../src/avatars-renderer/Avatar.cpp | 6 +- libraries/avatars/src/AvatarData.cpp | 73 +++++++++++-------- libraries/avatars/src/AvatarData.h | 10 ++- 5 files changed, 90 insertions(+), 70 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 4bf446f6b9..7b7adf14ca 100755 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -1321,22 +1321,22 @@ void MyAvatar::saveAvatarEntityDataToSettings() { for (const auto& id : _entitiesToRemoveFromSettings) { // remove entitiesToSave.erase(id); - std::map::iterator itr = _poorlyFormattedAvatarEntityData.find(id); - if (itr != _poorlyFormattedAvatarEntityData.end()) { - _poorlyFormattedAvatarEntityData.erase(itr); + std::map::iterator itr = _cachedAvatarEntityDataSettings.find(id); + if (itr != _cachedAvatarEntityDataSettings.end()) { + _cachedAvatarEntityDataSettings.erase(itr); } } for (const auto& id : entitiesToSave) { // remove old strings to be replaced by new saves - std::map::iterator itr = _poorlyFormattedAvatarEntityData.find(id); - if (itr != _poorlyFormattedAvatarEntityData.end()) { - _poorlyFormattedAvatarEntityData.erase(itr); + std::map::iterator itr = _cachedAvatarEntityDataSettings.find(id); + if (itr != _cachedAvatarEntityDataSettings.end()) { + _cachedAvatarEntityDataSettings.erase(itr); } } _entitiesToRemoveFromSettings.clear(); }); - uint32_t numEntities = (uint32_t)(entitiesToSave.size() + _poorlyFormattedAvatarEntityData.size()); + uint32_t numEntities = (uint32_t)(entitiesToSave.size() + _cachedAvatarEntityDataSettings.size()); uint32_t prevNumEntities = _avatarEntityCountSetting.get(0); resizeAvatarEntitySettingHandles(std::max(numEntities, prevNumEntities)); @@ -1353,11 +1353,11 @@ void MyAvatar::saveAvatarEntityDataToSettings() { } } }); - // convert properties to strings + // convert properties to our unfortunately-formatted-binary-blob format QScriptEngine scriptEngine; QScriptValue toStringMethod = scriptEngine.evaluate("(function() { return JSON.stringify(this) })"); for (const auto& entry : allProperties) { - // begin recipe our unfortunate legacy avatarEntityData format + // begin recipe for converting to our unfortunately-formatted-binar-blob QScriptValue scriptValue = EntityItemNonDefaultPropertiesToScriptValue(&scriptEngine, entry.second); QVariant variantProperties = scriptValue.toVariant(); QJsonDocument jsonProperties = QJsonDocument::fromVariant(variantProperties); @@ -1372,11 +1372,12 @@ void MyAvatar::saveAvatarEntityDataToSettings() { QByteArray binaryProperties = jsonProperties.toBinaryData(); // end recipe - _poorlyFormattedAvatarEntityData[entry.first] = binaryProperties; + // remember this unfortunately-formatted-binary-blob for later so we don't have go through this again + _cachedAvatarEntityDataSettings[entry.first] = binaryProperties; } - // save all strings + // save all unfortunately-formatted-binary-blobs to settings uint32_t i = 0; - for (const auto& entry : _poorlyFormattedAvatarEntityData) { + for (const auto& entry : _cachedAvatarEntityDataSettings) { _avatarEntityIDSettings[i].set(entry.first); _avatarEntityDataSettings[i].set(entry.second); ++i; @@ -1570,7 +1571,7 @@ void MyAvatar::loadData() { void MyAvatar::loadAvatarEntityDataFromSettings() { _avatarEntitiesLock.withReadLock([&] { - _avatarEntityData.clear(); + _packedAvatarEntityData.clear(); }); _reloadAvatarEntityDataFromSettings = false; @@ -1580,32 +1581,34 @@ void MyAvatar::loadAvatarEntityDataFromSettings() { } QScriptEngine scriptEngine; - std::vector entitiesToLoad; + std::vector< std::pair > entitiesToLoad; resizeAvatarEntitySettingHandles(numEntities); for (int i = 0; i < numEntities; i++) { - QUuid entityID = QUuid::createUuid(); // generate a new ID - QByteArray binaryData = _avatarEntityDataSettings[i].get(); - //updateAvatarEntity(entityID, binaryData); + QUuid id = QUuid::createUuid(); // generate a new ID + // the avatarEntityData is stored as an unfortunately-formatted-binary-blob + QByteArray binaryData = _avatarEntityDataSettings[i].get(); _avatarEntityDataChanged = true; if (_clientTraitsHandler) { // we have a client traits handler, so we need to mark this instanced trait as changed // so that changes will be sent next frame - _clientTraitsHandler->markInstancedTraitUpdated(AvatarTraits::AvatarEntity, entityID); + _clientTraitsHandler->markInstancedTraitUpdated(AvatarTraits::AvatarEntity, id); } + // begin recipe to extract EntityItemProperties from unfortunately-formatted-binary-blob QJsonDocument jsonProperties = QJsonDocument::fromBinaryData(binaryData); if (!jsonProperties.isObject()) { qCDebug(interfaceapp) << "bad avatarEntityData json" << QString(binaryData.toHex()); continue; } - - QVariant variantProperties = jsonProperties.toVariant(); - QVariantMap asMap = variantProperties.toMap(); - QScriptValue scriptProperties = variantMapToScriptValue(asMap, scriptEngine); + QVariant variant = jsonProperties.toVariant(); + QVariantMap variantMap = variant.toMap(); + QScriptValue scriptValue = variantMapToScriptValue(variantMap, scriptEngine); EntityItemProperties properties; - EntityItemPropertiesFromScriptValueHonorReadOnly(scriptProperties, properties); + EntityItemPropertiesFromScriptValueHonorReadOnly(scriptValue, properties); + // end recipe + properties.setEntityHostType(entity::HostType::AVATAR); properties.setOwningAvatarID(getID()); @@ -1624,36 +1627,38 @@ void MyAvatar::loadAvatarEntityDataFromSettings() { // and seems safe (per Seth). properties.markAllChanged(); - entitiesToLoad.push_back(properties); + entitiesToLoad.push_back({id, properties}); } - _poorlyFormattedAvatarEntityData.clear(); + _cachedAvatarEntityDataSettings.clear(); auto entityTreeRenderer = qApp->getEntities(); EntityTreePointer entityTree = entityTreeRenderer ? entityTreeRenderer->getTree() : nullptr; if (entityTree) { OctreePacketData packetData(false, AvatarTraits::MAXIMUM_TRAIT_SIZE); EncodeBitstreamParams params; EntityTreeElementExtraEncodeDataPointer extra { nullptr }; - for (uint32_t i = 0; i < entitiesToLoad.size(); ++i) { + uint32_t i = 0; + for (const auto& entry : entitiesToLoad) { // try to create the entity - QUuid entityID = QUuid::createUuid(); // generate a new ID entityTree->withWriteLock([&] { - EntityItemPointer entity = entityTree->addEntity(entityID, entitiesToLoad[i]); + QUuid id = entry.first; + EntityItemPointer entity = entityTree->addEntity(id, entry.second); if (entity) { // use the entity to build the data payload OctreeElement::AppendState appendState = entity->appendEntityData(&packetData, params, extra); if (appendState == OctreeElement::COMPLETED) { // only remember an AvatarEntity that successfully loads and can be packed QByteArray tempArray((const char*)packetData.getUncompressedData(), packetData.getUncompressedSize()); - storeAvatarEntityDataPayload(entityID, tempArray); + storeAvatarEntityDataPayload(id, tempArray); - // we store these binaryProperties for later: when saving back to settings - // unfortunately we must use this non-human-readable format because reasons - _poorlyFormattedAvatarEntityData[entityID] = _avatarEntityDataSettings[i].get(); + // only cache things that successfully loaded and packed + // we cache these binaryProperties for later: for when saving back to settings + _cachedAvatarEntityDataSettings[id] = _avatarEntityDataSettings[i].get(); } packetData.reset(); } }); + ++i; } } else { // TODO? handle this case: try to load AvatatEntityData from settings again later? diff --git a/interface/src/avatar/MyAvatar.h b/interface/src/avatar/MyAvatar.h index 98c93f9ccc..9bbb25a3be 100644 --- a/interface/src/avatar/MyAvatar.h +++ b/interface/src/avatar/MyAvatar.h @@ -1948,7 +1948,7 @@ private: Setting::Handle _allowTeleportingSetting { "allowTeleporting", true }; std::vector> _avatarEntityIDSettings; std::vector> _avatarEntityDataSettings; - std::map _poorlyFormattedAvatarEntityData; + std::map _cachedAvatarEntityDataSettings; std::set _entitiesToSaveToSettings; std::set _entitiesToRemoveFromSettings; }; diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index 59d9f13f04..b41e1381ef 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -436,12 +436,12 @@ void Avatar::updateAvatarEntities() { AvatarEntityIDs recentlyDetachedAvatarEntities = getAndClearRecentlyDetachedIDs(); if (!recentlyDetachedAvatarEntities.empty()) { // only lock this thread when absolutely necessary - AvatarEntityMap avatarEntityData; + AvatarEntityMap packedAvatarEntityData; _avatarEntitiesLock.withReadLock([&] { - avatarEntityData = _avatarEntityData; + packedAvatarEntityData = _packedAvatarEntityData; }); foreach (auto entityID, recentlyDetachedAvatarEntities) { - if (!avatarEntityData.contains(entityID)) { + if (!packedAvatarEntityData.contains(entityID)) { entityTree->deleteEntity(entityID, true, true); } } diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index f08e66af82..093b86d0e6 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1908,10 +1908,9 @@ qint64 AvatarData::packAvatarEntityTraitInstance(AvatarTraits::TraitType traitTy // grab a read lock on the avatar entities and check for entity data for the given ID QByteArray entityBinaryData; - _avatarEntitiesLock.withReadLock([this, &entityBinaryData, &traitInstanceID] { - if (_avatarEntityData.contains(traitInstanceID)) { - entityBinaryData = _avatarEntityData[traitInstanceID]; + if (_packedAvatarEntityData.contains(traitInstanceID)) { + entityBinaryData = _packedAvatarEntityData[traitInstanceID]; } }); @@ -1998,7 +1997,7 @@ qint64 AvatarData::packTraitInstance(AvatarTraits::TraitType traitType, AvatarTr void AvatarData::prepareResetTraitInstances() { if (_clientTraitsHandler) { _avatarEntitiesLock.withReadLock([this]{ - foreach (auto entityID, _avatarEntityData.keys()) { + foreach (auto entityID, _packedAvatarEntityData.keys()) { _clientTraitsHandler->markInstancedTraitUpdated(AvatarTraits::AvatarEntity, entityID); } foreach (auto grabID, _avatarGrabData.keys()) { @@ -2367,7 +2366,7 @@ void AvatarData::setRecordingBasis(std::shared_ptr recordingBasis) { void AvatarData::createRecordingIDs() { _avatarEntitiesLock.withReadLock([&] { _avatarEntityForRecording.clear(); - for (int i = 0; i < _avatarEntityData.size(); i++) { + for (int i = 0; i < _packedAvatarEntityData.size(); i++) { _avatarEntityForRecording.insert(QUuid::createUuid()); } }); @@ -2422,6 +2421,25 @@ JointData jointDataFromJsonValue(int version, const QJsonValue& json) { return result; } +void AvatarData::avatarEntityDataToJson(QJsonObject& root) const { + // ADEBUG this is broken + _avatarEntitiesLock.withReadLock([&] { + AvatarEntityMap _avatarEntityData = _packedAvatarEntityData; // hack to compile + if (!_avatarEntityData.empty()) { + QJsonArray avatarEntityJson; + int entityCount = 0; + for (auto entityID : _avatarEntityData.keys()) { + QVariantMap entityData; + QUuid newId = _avatarEntityForRecording.size() == _avatarEntityData.size() ? _avatarEntityForRecording.values()[entityCount++] : entityID; + entityData.insert("id", newId); + entityData.insert("properties", _avatarEntityData.value(entityID).toBase64()); + avatarEntityJson.push_back(QVariant(entityData).toJsonObject()); + } + root[JSON_AVATAR_ENTITIES] = avatarEntityJson; + } + }); +} + QJsonObject AvatarData::toJson() const { QJsonObject root; @@ -2433,20 +2451,8 @@ QJsonObject AvatarData::toJson() const { if (!getDisplayName().isEmpty()) { root[JSON_AVATAR_DISPLAY_NAME] = getDisplayName(); } - _avatarEntitiesLock.withReadLock([&] { - if (!_avatarEntityData.empty()) { - QJsonArray avatarEntityJson; - int entityCount = 0; - for (auto entityID : _avatarEntityData.keys()) { - QVariantMap entityData; - QUuid newId = _avatarEntityForRecording.size() == _avatarEntityData.size() ? _avatarEntityForRecording.values()[entityCount++] : entityID; - entityData.insert("id", newId); - entityData.insert("properties", _avatarEntityData.value(entityID).toBase64()); - avatarEntityJson.push_back(QVariant(entityData).toJsonObject()); - } - root[JSON_AVATAR_ENTITIES] = avatarEntityJson; - } - }); + + avatarEntityDataToJson(root); auto recordingBasis = getRecordingBasis(); bool success; @@ -2568,10 +2574,10 @@ void AvatarData::fromJson(const QJsonObject& json, bool useFrameSkeleton) { for (auto attachmentJson : attachmentsJson) { if (attachmentJson.isObject()) { QVariantMap entityData = attachmentJson.toObject().toVariantMap(); - QUuid entityID = entityData.value("id").toUuid(); + QUuid id = entityData.value("id").toUuid(); // ADEBUG TODO: fix this broken path - QByteArray properties = QByteArray::fromBase64(entityData.value("properties").toByteArray()); - storeAvatarEntityDataPayload(entityID, properties); + QByteArray data = QByteArray::fromBase64(entityData.value("properties").toByteArray()); + updateAvatarEntityData(id, data); } } } @@ -2757,10 +2763,10 @@ const int MAX_NUM_AVATAR_ENTITIES = 42; void AvatarData::storeAvatarEntityDataPayload(const QUuid& entityID, const QByteArray& data) { _avatarEntitiesLock.withWriteLock([&] { - AvatarEntityMap::iterator itr = _avatarEntityData.find(entityID); - if (itr == _avatarEntityData.end()) { - if (_avatarEntityData.size() < MAX_NUM_AVATAR_ENTITIES) { - _avatarEntityData.insert(entityID, data); + PackedAvatarEntityMap::iterator itr = _packedAvatarEntityData.find(entityID); + if (itr == _packedAvatarEntityData.end()) { + if (_packedAvatarEntityData.size() < MAX_NUM_AVATAR_ENTITIES) { + _packedAvatarEntityData.insert(entityID, data); } } else { itr.value() = data; @@ -2776,8 +2782,12 @@ void AvatarData::storeAvatarEntityDataPayload(const QUuid& entityID, const QByte } } -void AvatarData::updateAvatarEntity(const QUuid& id, const QScriptValue& data) { - // no op +void AvatarData::updateAvatarEntity(const QUuid& id, const QScriptValue& scriptValue) { + // overridden where needed +} + +void AvatarData::updateAvatarEntityData(const QUuid& id, const QByteArray& data) { + // ADEBUG TODO: implement this } void AvatarData::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFromTree) { @@ -2785,7 +2795,7 @@ void AvatarData::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFr bool removedEntity = false; _avatarEntitiesLock.withWriteLock([this, &removedEntity, &entityID] { - removedEntity = _avatarEntityData.remove(entityID); + removedEntity = _packedAvatarEntityData.remove(entityID); }); insertDetachedEntityID(entityID); @@ -2798,9 +2808,10 @@ void AvatarData::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFr } AvatarEntityMap AvatarData::getAvatarEntityData() const { + // ADEBUG broken AvatarEntityMap result; _avatarEntitiesLock.withReadLock([&] { - result = _avatarEntityData; + result = _packedAvatarEntityData; }); return result; } @@ -2823,7 +2834,9 @@ void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) { std::vector deletedEntityIDs; QList updatedEntityIDs; + // ADEBUG this is broken _avatarEntitiesLock.withWriteLock([&] { + AvatarEntityMap _avatarEntityData = _packedAvatarEntityData; // hack to compile if (_avatarEntityData != avatarEntityData) { // keep track of entities that were attached to this avatar but no longer are diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 3d6dd78e05..1bae3cd32f 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -63,6 +63,7 @@ using AvatarWeakPointer = std::weak_ptr; using AvatarHash = QHash; using AvatarEntityMap = QMap; +using PackedAvatarEntityMap = QMap; // similar to AvatarEntityMap, but different internal format using AvatarEntityIDs = QSet; using AvatarGrabDataMap = QMap; @@ -959,7 +960,8 @@ public: * @param {Uuid} entityID * @param {object} entityData */ - Q_INVOKABLE virtual void updateAvatarEntity(const QUuid& id, const QScriptValue& data); + Q_INVOKABLE virtual void updateAvatarEntity(const QUuid& id, const QScriptValue& scriptValue); + virtual void updateAvatarEntityData(const QUuid& id, const QByteArray& data); /**jsdoc * @function MyAvatar.clearAvatarEntity @@ -1126,6 +1128,7 @@ public: TransformPointer getRecordingBasis() const; void setRecordingBasis(TransformPointer recordingBasis = TransformPointer()); void createRecordingIDs(); + void avatarEntityDataToJson(QJsonObject& root) const; QJsonObject toJson() const; void fromJson(const QJsonObject& json, bool useFrameSkeleton = true); @@ -1140,11 +1143,10 @@ public: Q_INVOKABLE AvatarEntityMap getAvatarEntityData() const; /**jsdoc - * Temporarily disabled. Use updateAvatarEntity(id, properties) in the meantime. * @function MyAvatar.setAvatarEntityData * @param {object} avatarEntityData */ - void setAvatarEntityData(const AvatarEntityMap& avatarEntityData); + Q_INVOKABLE void setAvatarEntityData(const AvatarEntityMap& avatarEntityData); virtual void setAvatarEntityDataChanged(bool value) { _avatarEntityDataChanged = value; } void insertDetachedEntityID(const QUuid entityID); @@ -1465,7 +1467,7 @@ protected: mutable ReadWriteLockable _avatarEntitiesLock; AvatarEntityIDs _avatarEntityDetached; // recently detached from this avatar AvatarEntityIDs _avatarEntityForRecording; // create new entities id for avatar recording - AvatarEntityMap _avatarEntityData; + PackedAvatarEntityMap _packedAvatarEntityData; bool _avatarEntityDataChanged { false }; mutable ReadWriteLockable _avatarGrabsLock;