From 10a0f77d4cef341e56b3ea68d3a3090a51337dc1 Mon Sep 17 00:00:00 2001 From: SamGondelman Date: Tue, 12 Jun 2018 16:48:47 -0700 Subject: [PATCH 1/3] FINALLY fix transparent textures rendering wrong --- .../model-networking/src/model-networking/ModelCache.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/libraries/model-networking/src/model-networking/ModelCache.cpp b/libraries/model-networking/src/model-networking/ModelCache.cpp index 716d2aeab1..f669e8aaef 100644 --- a/libraries/model-networking/src/model-networking/ModelCache.cpp +++ b/libraries/model-networking/src/model-networking/ModelCache.cpp @@ -422,7 +422,7 @@ bool Geometry::areTexturesLoaded() const { } // Failed texture downloads need to be considered as 'loaded' // or the object will never fade in - bool finished = texture->isLoaded() || texture->isFailed(); + bool finished = texture->isFailed() || (texture->isLoaded() && texture->getGPUTexture() && texture->getGPUTexture()->isDefined()); if (!finished) { return true; } @@ -434,8 +434,11 @@ bool Geometry::areTexturesLoaded() const { } // If material textures are loaded, check the material translucency + // FIXME: This should not be done here. The opacity map should already be reset in Material::setTextureMap. + // However, currently that code can be called before the albedo map is defined, so resetOpacityMap will fail. + // Geometry::areTexturesLoaded() is called repeatedly until it returns true, so we do the check here for now const auto albedoTexture = material->_textures[NetworkMaterial::MapChannel::ALBEDO_MAP]; - if (albedoTexture.texture && albedoTexture.texture->getGPUTexture()) { + if (albedoTexture.texture) { material->resetOpacityMap(); } } From 5ac7ced3062ff63b9aa15c2421cbb39ac495bdac Mon Sep 17 00:00:00 2001 From: SamGondelman Date: Wed, 13 Jun 2018 17:14:52 -0700 Subject: [PATCH 2/3] cleanup renderable model entity item, fix crash on reload, fix different transparent bug --- .../src/EntityTreeRenderer.cpp | 1 - .../src/RenderableEntityItem.cpp | 2 +- .../src/RenderableModelEntityItem.cpp | 131 ++++++++++-------- .../src/RenderableModelEntityItem.h | 10 +- libraries/networking/src/ResourceCache.cpp | 22 +-- 5 files changed, 87 insertions(+), 79 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 600d1c32a8..53e386c223 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -382,7 +382,6 @@ void EntityTreeRenderer::updateChangedEntities(const render::ScenePointer& scene uint64_t expiry = updateStart + timeBudget; // process the sorted renderables - std::unordered_map::iterator itr; size_t numSorted = sortedRenderables.size(); while (!sortedRenderables.empty() && usecTimestampNow() < expiry) { const auto renderable = sortedRenderables.top().getRenderer(); diff --git a/libraries/entities-renderer/src/RenderableEntityItem.cpp b/libraries/entities-renderer/src/RenderableEntityItem.cpp index ae4c13d96f..6ff2ff576c 100644 --- a/libraries/entities-renderer/src/RenderableEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableEntityItem.cpp @@ -284,8 +284,8 @@ bool EntityRenderer::addToScene(const ScenePointer& scene, Transaction& transact makeStatusGetters(_entity, statusGetters); renderPayload->addStatusGetters(statusGetters); transaction.resetItem(_renderItemID, renderPayload); - updateInScene(scene, transaction); onAddToScene(_entity); + updateInScene(scene, transaction); return true; } diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp index a91534668c..64cab39701 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp @@ -191,7 +191,7 @@ bool RenderableModelEntityItem::needsUpdateModelBounds() const { } } - return model->needsReload(); + return false; } void RenderableModelEntityItem::updateModelBounds() { @@ -1182,19 +1182,8 @@ void ModelEntityRenderer::animate(const TypedEntityPointer& entity) { } bool ModelEntityRenderer::needsRenderUpdate() const { - ModelPointer model; - withReadLock([&] { - model = _model; - }); - - if (model) { - if (_needsJointSimulation || _moving || _animating) { - return true; - } - - // When the individual mesh parts of a model finish fading, they will mark their Model as needing updating - // we will watch for that and ask the model to update it's render items - if (_parsedModelURL != model->getURL()) { + if (resultWithReadLock([&] { + if (_moving || _animating) { return true; } @@ -1202,23 +1191,44 @@ bool ModelEntityRenderer::needsRenderUpdate() const { return true; } + if (_needsCollisionGeometryUpdate) { + return true; + } + + if (!_prevModelLoaded) { + return true; + } + + return false; + })) { + return true; + } + + ModelPointer model; + QUrl parsedModelURL; + withReadLock([&] { + model = _model; + parsedModelURL = _parsedModelURL; + }); + + if (model) { + // When the individual mesh parts of a model finish fading, they will mark their Model as needing updating + // we will watch for that and ask the model to update it's render items + if (parsedModelURL != model->getURL()) { + return true; + } + if (model->needsReload()) { return true; } - // FIXME what is the difference between these two? if (model->needsFixupInScene()) { return true; } - // FIXME what is the difference between these two? ^^^^ if (model->getRenderItemsNeedUpdate()) { return true; } - - if (_needsCollisionGeometryUpdate) { - return true; - } } return Parent::needsRenderUpdate(); } @@ -1229,7 +1239,7 @@ bool ModelEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPoin return true; } - if (_lastModelURL != entity->getModelURL()) { + if (_parsedModelURL != entity->getModelURL()) { return true; } @@ -1242,10 +1252,6 @@ bool ModelEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPoin return true; } - if (_renderAnimationProperties != entity->getAnimationProperties()) { - return true; - } - if (_animating != entity->isAnimatingSomething()) { return true; } @@ -1259,7 +1265,7 @@ bool ModelEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPoin }); if (model && model->isLoaded()) { - if (!entity->_dimensionsInitialized || entity->_needsInitialSimulation) { + if (!entity->_dimensionsInitialized || entity->_needsInitialSimulation || !entity->_originalTexturesRead) { return true; } @@ -1296,16 +1302,15 @@ void ModelEntityRenderer::setCollisionMeshKey(const void*key) { void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene, Transaction& transaction, const TypedEntityPointer& entity) { DETAILED_PROFILE_RANGE(simulation_physics, __FUNCTION__); if (_hasModel != entity->hasModel()) { - _hasModel = entity->hasModel(); + withWriteLock([&] { + _hasModel = entity->hasModel(); + }); } - _marketplaceEntity = entity->getMarketplaceID().length() != 0; - _animating = entity->isAnimatingSomething(); - withWriteLock([&] { - if (_lastModelURL != entity->getModelURL()) { - _lastModelURL = entity->getModelURL(); - _parsedModelURL = QUrl(_lastModelURL); + _animating = entity->isAnimatingSomething(); + if (_parsedModelURL != entity->getModelURL()) { + _parsedModelURL = QUrl(entity->getModelURL()); } }); @@ -1313,7 +1318,7 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce ModelPointer model; withReadLock([&] { model = _model; }); if (!_hasModel) { - if ((bool)model) { + if (model) { model->removeFromScene(scene, transaction); withWriteLock([&] { _model.reset(); }); transaction.updateItem(getRenderItemID(), [](PayloadProxyInterface& data) { @@ -1327,8 +1332,9 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce } // Check for addition - if (_hasModel && !(bool)_model) { + if (_hasModel && !model) { model = std::make_shared(nullptr, entity.get()); + connect(model.get(), &Model::requestRenderUpdate, this, &ModelEntityRenderer::requestRenderUpdate); connect(model.get(), &Model::setURLFinished, this, [&](bool didVisualGeometryRequestSucceed) { setKey(didVisualGeometryRequestSucceed); emit requestRenderUpdate(); @@ -1338,7 +1344,6 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce } _didLastVisualGeometryRequestSucceed = didVisualGeometryRequestSucceed; }); - connect(model.get(), &Model::requestRenderUpdate, this, &ModelEntityRenderer::requestRenderUpdate); connect(entity.get(), &RenderableModelEntityItem::requestCollisionGeometryUpdate, this, &ModelEntityRenderer::flagForCollisionGeometryUpdate); model->setLoadingPriority(EntityTreeRenderer::getEntityLoadingPriority(*entity)); entity->setModel(model); @@ -1346,19 +1351,28 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce } // From here on, we are guaranteed a populated model - withWriteLock([&] { - if (_parsedModelURL != model->getURL()) { + if (_parsedModelURL != model->getURL()) { + withWriteLock([&] { _texturesLoaded = false; model->setURL(_parsedModelURL); - } - }); + }); + } // Nothing else to do unless the model is loaded if (!model->isLoaded()) { + withWriteLock([&] { + _prevModelLoaded = false; + }); + emit requestRenderUpdate(); return; + } else if (!_prevModelLoaded) { + withWriteLock([&] { + _prevModelLoaded = true; + }); } // Check for initializing the model + // FIXME: There are several places below here where we are modifying the entity, which we should not be doing from the renderable if (!entity->_dimensionsInitialized) { EntityItemProperties properties; properties.setLastEdited(usecTimestampNow()); // we must set the edit time since we're editing it @@ -1376,16 +1390,16 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce // Default to _originalTextures to avoid remapping immediately and lagging on load entity->_originalTextures = model->getTextures(); entity->_originalTexturesRead = true; - _currentTextures = entity->_originalTextures; } if (_lastTextures != entity->getTextures()) { - _texturesLoaded = false; - _lastTextures = entity->getTextures(); + withWriteLock([&] { + _texturesLoaded = false; + _lastTextures = entity->getTextures(); + }); auto newTextures = parseTexturesToMap(_lastTextures, entity->_originalTextures); - if (newTextures != _currentTextures) { + if (newTextures != model->getTextures()) { model->setTextures(newTextures); - _currentTextures = newTextures; } } if (entity->_needsJointSimulation) { @@ -1394,14 +1408,11 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce entity->updateModelBounds(); entity->stopModelOverrideIfNoParent(); - render::hifi::Tag tagMask = getTagMask(); if (model->isVisible() != _visible) { - // FIXME: this seems like it could be optimized if we tracked our last known visible state in - // the renderable item. As it stands now the model checks it's visible/invisible state - // so most of the time we don't do anything in this function. model->setVisibleInScene(_visible, scene); } + render::hifi::Tag tagMask = getTagMask(); if (model->getTagMask() != tagMask) { model->setTagMask(tagMask, scene); } @@ -1420,7 +1431,7 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce // NOTE: it is OK if _collisionMeshKey is nullptr graphics::MeshPointer mesh = collisionMeshCache.getMesh(_collisionMeshKey); // NOTE: the model will render the collisionGeometry if it has one - _model->setCollisionMesh(mesh); + model->setCollisionMesh(mesh); } else { if (_collisionMeshKey) { // release mesh @@ -1428,7 +1439,7 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce } // clear model's collision geometry graphics::MeshPointer mesh = nullptr; - _model->setCollisionMesh(mesh); + model->setCollisionMesh(mesh); } } @@ -1450,7 +1461,9 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce } if (!_texturesLoaded && model->getGeometry() && model->getGeometry()->areTexturesLoaded()) { - _texturesLoaded = true; + withWriteLock([&] { + _texturesLoaded = true; + }); model->updateRenderItems(); } else if (!_texturesLoaded) { emit requestRenderUpdate(); @@ -1503,15 +1516,15 @@ void ModelEntityRenderer::doRender(RenderArgs* args) { batch.setModelTransform(getModelTransform()); // we want to include the scale as well DependencyManager::get()->renderWireCubeInstance(args, batch, greenColor); - // Enqueue updates for the next frame #if WANT_EXTRA_DEBUGGING - // debugging... - gpu::Batch& batch = *args->_batch; - _model->renderDebugMeshBoxes(batch); + ModelPointer model; + withReadLock([&] { + model = _model; + }); + if (model) { + model->renderDebugMeshBoxes(batch); + } #endif - - // Remap textures for the next frame to avoid flicker - // remapTextures(); } void ModelEntityRenderer::mapJoints(const TypedEntityPointer& entity, const QStringList& modelJointNames) { diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.h b/libraries/entities-renderer/src/RenderableModelEntityItem.h index f1748ca069..58608d0725 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.h +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.h @@ -171,7 +171,7 @@ protected: private: void animate(const TypedEntityPointer& entity); void mapJoints(const TypedEntityPointer& entity, const QStringList& modelJointNames); - bool jointsMapped() const { return _jointMappingURL == _renderAnimationProperties.getURL() && _jointMappingCompleted; } + bool jointsMapped() const { return _jointMappingCompleted; } // Transparency is handled in ModelMeshPartPayload virtual bool isTransparent() const override { return false; } @@ -180,33 +180,27 @@ private: ModelPointer _model; GeometryResource::Pointer _compoundShapeResource; QString _lastTextures; - QVariantMap _currentTextures; bool _texturesLoaded { false }; - AnimationPropertyGroup _renderAnimationProperties; int _lastKnownCurrentFrame { -1 }; #ifdef MODEL_ENTITY_USE_FADE_EFFECT bool _hasTransitioned{ false }; #endif - bool _needsJointSimulation { false }; bool _needsCollisionGeometryUpdate { false }; const void* _collisionMeshKey { nullptr }; // used on client side bool _jointMappingCompleted{ false }; QVector _jointMapping; // domain is index into model-joints, range is index into animation-joints - QString _jointMappingURL; AnimationPointer _animation; - QString _lastModelURL; QUrl _parsedModelURL; - bool _marketplaceEntity { false }; - bool _shouldHighlight { false }; bool _animating { false }; uint64_t _lastAnimated { 0 }; render::ItemKey _itemKey { render::ItemKey::Builder().withTypeMeta() }; bool _didLastVisualGeometryRequestSucceed { true }; + bool _prevModelLoaded { false }; void processMaterials(); }; diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index 0b1334daba..d07420f87e 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -706,7 +706,16 @@ void Resource::handleDownloadProgress(uint64_t bytesReceived, uint64_t bytesTota } void Resource::handleReplyFinished() { - Q_ASSERT_X(_request, "Resource::handleReplyFinished", "Request should not be null while in handleReplyFinished"); + if (!_request || _request != sender()) { + // This can happen in the edge case that a request is timed out, but a `finished` signal is emitted before it is deleted. + qWarning(networking) << "Received signal Resource::handleReplyFinished from ResourceRequest that is not the current" + << " request: " << sender() << ", " << _request; + PROFILE_ASYNC_END(resource, "Resource:" + getType(), QString::number(_requestID), { + { "from_cache", false }, + { "size_mb", _bytesTotal / 1000000.0 } + }); + return; + } PROFILE_ASYNC_END(resource, "Resource:" + getType(), QString::number(_requestID), { { "from_cache", _request->loadedFromCache() }, @@ -715,15 +724,8 @@ void Resource::handleReplyFinished() { setSize(_bytesTotal); - if (!_request || _request != sender()) { - // This can happen in the edge case that a request is timed out, but a `finished` signal is emitted before it is deleted. - qWarning(networking) << "Received signal Resource::handleReplyFinished from ResourceRequest that is not the current" - << " request: " << sender() << ", " << _request; - return; - } - ResourceCache::requestCompleted(_self); - + auto result = _request->getResult(); if (result == ResourceRequest::Success) { auto extraInfo = _url == _activeUrl ? "" : QString(", %1").arg(_activeUrl.toDisplayString()); @@ -733,7 +735,7 @@ void Resource::handleReplyFinished() { if (!relativePathURL.isEmpty()) { _effectiveBaseURL = relativePathURL; } - + auto data = _request->getData(); emit loaded(data); downloadFinished(data); From 0b79e9f6c71e037e96949478c914c146b8ef012e Mon Sep 17 00:00:00 2001 From: SamGondelman Date: Fri, 15 Jun 2018 15:13:29 -0700 Subject: [PATCH 3/3] whoops --- libraries/entities-renderer/src/RenderableModelEntityItem.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp index 9975e2340e..a7126b847e 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp @@ -1185,10 +1185,6 @@ bool ModelEntityRenderer::needsRenderUpdate() const { return true; } - if (_needsCollisionGeometryUpdate) { - return true; - } - if (!_prevModelLoaded) { return true; }