Merge pull request #13362 from SamGondelman/modelTextures

Fix transparent textures rendering wrong sometimes

libraries/entities-renderer/src/EntityTreeRenderer.cpp

@@ -404,7 +404,6 @@ void EntityTreeRenderer::updateChangedEntities(const render::ScenePointer& scene
             uint64_t expiry = updateStart + timeBudget;
 
             // process the sorted renderables
-            std::unordered_map<EntityItemID, EntityRendererPointer>::iterator itr;
             size_t numSorted = sortedRenderables.size();
             while (!sortedRenderables.empty() && usecTimestampNow() < expiry) {
                 const auto renderable = sortedRenderables.top().getRenderer();

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;
 }
 

libraries/entities-renderer/src/RenderableModelEntityItem.cpp

@@ -188,7 +188,7 @@ bool RenderableModelEntityItem::needsUpdateModelBounds() const {
         }
     }
 
-    return model->needsReload();
+    return false;
 }
 
 void RenderableModelEntityItem::updateModelBounds() {
@@ -1176,19 +1176,8 @@ void ModelEntityRenderer::animate(const TypedEntityPointer& entity) {
 }
 
 bool ModelEntityRenderer::needsRenderUpdate() const {
-    ModelPointer model;
+    if (resultWithReadLock<bool>([&] {
-    withReadLock([&] {
+        if (_moving || _animating) {
-        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()) {
             return true;
         }
 
@@ -1196,16 +1185,37 @@ bool ModelEntityRenderer::needsRenderUpdate() const {
             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;
         }
@@ -1219,7 +1229,7 @@ bool ModelEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPoin
             return true;
         }
 
-        if (_lastModelURL != entity->getModelURL()) {
+        if (_parsedModelURL != entity->getModelURL()) {
             return true;
         }
 
@@ -1232,10 +1242,6 @@ bool ModelEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPoin
             return true;
         }
 
-        if (_renderAnimationProperties != entity->getAnimationProperties()) {
-            return true;
-        }
-
         if (_animating != entity->isAnimatingSomething()) {
             return true;
         }
@@ -1249,7 +1255,7 @@ bool ModelEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPoin
     });
 
     if (model && model->isLoaded()) {
-        if (!entity->_dimensionsInitialized || entity->_needsInitialSimulation) {
+        if (!entity->_dimensionsInitialized || entity->_needsInitialSimulation || !entity->_originalTexturesRead) {
             return true;
        } 
 
@@ -1281,16 +1287,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()) {
+        _animating = entity->isAnimatingSomething();
-            _lastModelURL = entity->getModelURL();
+        if (_parsedModelURL != entity->getModelURL()) {
-            _parsedModelURL = QUrl(_lastModelURL);
+            _parsedModelURL = QUrl(entity->getModelURL());
         }
     });
 
@@ -1298,7 +1303,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<PayloadProxyInterface>(getRenderItemID(), [](PayloadProxyInterface& data) {
@@ -1312,8 +1317,9 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce
     }
 
     // Check for addition
-    if (_hasModel && !(bool)_model) {
+    if (_hasModel && !model) {
         model = std::make_shared<Model>(nullptr, entity.get());
+        connect(model.get(), &Model::requestRenderUpdate, this, &ModelEntityRenderer::requestRenderUpdate);
         connect(model.get(), &Model::setURLFinished, this, [&](bool didVisualGeometryRequestSucceed) {
             setKey(didVisualGeometryRequestSucceed);
             emit requestRenderUpdate();
@@ -1323,26 +1329,34 @@ void ModelEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& sce
             }
             _didLastVisualGeometryRequestSucceed = didVisualGeometryRequestSucceed;
         });
-        connect(model.get(), &Model::requestRenderUpdate, this, &ModelEntityRenderer::requestRenderUpdate);
         model->setLoadingPriority(EntityTreeRenderer::getEntityLoadingPriority(*entity));
         entity->setModel(model);
         withWriteLock([&] { _model = model; });
     }
 
     // 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
@@ -1360,16 +1374,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;
+        withWriteLock([&] {
-        _lastTextures = entity->getTextures();
+            _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) {
@@ -1378,14 +1392,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);
     }
@@ -1414,7 +1425,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();
@@ -1462,15 +1475,15 @@ void ModelEntityRenderer::doRender(RenderArgs* args) {
     batch.setModelTransform(getModelTransform()); // we want to include the scale as well
     DependencyManager::get<GeometryCache>()->renderWireCubeInstance(args, batch, greenColor);
 
-    // Enqueue updates for the next frame
 #if WANT_EXTRA_DEBUGGING
-    // debugging...
+    ModelPointer model;
-    gpu::Batch& batch = *args->_batch;
+    withReadLock([&] {
-    _model->renderDebugMeshBoxes(batch);
+        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) {

libraries/entities-renderer/src/RenderableModelEntityItem.h

@@ -170,7 +170,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; }
@@ -179,32 +179,26 @@ 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 };
     const void* _collisionMeshKey { nullptr };
 
     // used on client side
     bool _jointMappingCompleted{ false };
     QVector<int> _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();
 };

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();
             }
         }

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);