From c220c1a11bb1c9bdb3c91743ed1f8ce72831e230 Mon Sep 17 00:00:00 2001 From: Svenn-Arne Dragly Date: Mon, 29 Oct 2018 17:17:29 +0100 Subject: Clean up texture loading in Dragon render aspect - Simplify LoadedTexture and LoadedTextureImage by not copying generator from Texture and TextureImage, respectively. - Remove code referring to asynchronous loading. This might be re- introduced at a later point, but is currently non-functional. - Clean up the code in loadTextures and loadTextureImages by removing old comments, commented-out code, and references to async loading. Change-Id: If07cbf26109cc83191f95c4abc53d28dae055c5f Reviewed-by: Andy Nichols --- src/runtime/dragon/dragongltexture.cpp | 6 +- src/runtime/dragon/dragontexture.cpp | 2 - src/runtime/dragon/dragontexture_p.h | 5 +- src/runtime/dragon/dragontextureimage.cpp | 2 +- src/runtime/dragon/dragontextureimage_p.h | 5 +- src/runtime/dragon/jobs/dragontexturejobs.cpp | 112 +++----------------------- src/runtime/dragon/jobs/dragontexturejobs_p.h | 12 +-- 7 files changed, 18 insertions(+), 126 deletions(-) diff --git a/src/runtime/dragon/dragongltexture.cpp b/src/runtime/dragon/dragongltexture.cpp index 3bedd7f..0023569 100644 --- a/src/runtime/dragon/dragongltexture.cpp +++ b/src/runtime/dragon/dragongltexture.cpp @@ -67,7 +67,7 @@ TextureProperties deriveProperties(const Immutable &loadedTexture TextureProperties properties; properties = loadedTexture->texture->properties; - if (loadedTexture->generator != nullptr) { + if (loadedTexture->texture->generator != nullptr) { // TODO untested branch, needs testing if (loadedTexture->data == nullptr) { qWarning() << "[Qt3DRender::GLTexture] No QTextureData generated from Texture " @@ -125,7 +125,7 @@ GLTexture createGlTexture(const Immutable &loadedTexture, QOpenGL Q_ASSERT(openGLContext != nullptr); bool needUpload = false; - if (loadedTexture->generator != nullptr || loadedTexture->images.size() > 0) { + if (loadedTexture->texture->generator != nullptr || loadedTexture->images.size() > 0) { needUpload = true; } @@ -135,7 +135,7 @@ GLTexture createGlTexture(const Immutable &loadedTexture, QOpenGL result.parameters = loadedTexture->texture->parameters; result.actualTarget = loadedTexture->texture->properties.target; - if (loadedTexture->generator) { + if (loadedTexture->texture->generator) { result.actualTarget = loadedTexture->data->target(); } diff --git a/src/runtime/dragon/dragontexture.cpp b/src/runtime/dragon/dragontexture.cpp index 7e181a5..cae7980 100644 --- a/src/runtime/dragon/dragontexture.cpp +++ b/src/runtime/dragon/dragontexture.cpp @@ -51,7 +51,6 @@ void Texture::sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e) switch (e->type()) { case PropertyUpdated: { QPropertyUpdatedChangePtr propertyChange = qSharedPointerCast(e); - qDebug() << "Property changed" << propertyChange->propertyName(); if (propertyChange->propertyName() == QByteArrayLiteral("width")) { properties.width = propertyChange->value().toInt(); dirty = DirtyProperties; @@ -201,7 +200,6 @@ bool operator ==(const Texture &a, const Texture &b) return compareGenerators(a.generator, b.generator) && a.properties == b.properties && a.parameters == b.parameters; } - } } QT_END_NAMESPACE diff --git a/src/runtime/dragon/dragontexture_p.h b/src/runtime/dragon/dragontexture_p.h index 435416b..f55d6c3 100644 --- a/src/runtime/dragon/dragontexture_p.h +++ b/src/runtime/dragon/dragontexture_p.h @@ -133,7 +133,7 @@ public: TextureParameters parameters; QTextureGeneratorPtr generator; - bool isLoadWithinFrameEnabled = true; + // TODO this is not used because a texture image must be a child of a texture to work, // due to a limitation in the change arbiter not being able to pick up changes at construction // time. @@ -143,13 +143,10 @@ public: bool operator ==(const Texture &a, const Texture &b); -// TODO add support for texture generators and not just texture images struct LoadedTexture { Immutable texture; - QTextureGeneratorPtr generator; QVector> images; -// QFuture future; QTextureDataPtr data; }; diff --git a/src/runtime/dragon/dragontextureimage.cpp b/src/runtime/dragon/dragontextureimage.cpp index fca57de..35dc282 100644 --- a/src/runtime/dragon/dragontextureimage.cpp +++ b/src/runtime/dragon/dragontextureimage.cpp @@ -88,7 +88,7 @@ bool operator ==(const TextureImage &a, const TextureImage &b) bool operator ==(const LoadedTextureImage &a, const LoadedTextureImage &b) { - return *a.generator == *b.generator; + return a.image == b.image; } } diff --git a/src/runtime/dragon/dragontextureimage_p.h b/src/runtime/dragon/dragontextureimage_p.h index 441f3be..3a7fa55 100644 --- a/src/runtime/dragon/dragontextureimage_p.h +++ b/src/runtime/dragon/dragontextureimage_p.h @@ -65,11 +65,12 @@ public: Qt3DCore::QNodeId parentId; int layer = 0; int mipLevel = 0; - bool loadWithinFrame = true; QAbstractTexture::CubeMapFace face; QTextureImageDataGeneratorPtr generator; }; +bool operator ==(const TextureImage &a, const TextureImage &b); + inline bool operator<(const TextureImage &a, const TextureImage &b) { return a.peerId() < b.peerId(); @@ -87,8 +88,6 @@ struct LoadedTextureImage }; Status status = Loading; Immutable image; - QTextureImageDataGeneratorPtr generator; - QFuture future; QTextureImageDataPtr data; }; diff --git a/src/runtime/dragon/jobs/dragontexturejobs.cpp b/src/runtime/dragon/jobs/dragontexturejobs.cpp index 8df5630..09d4387 100644 --- a/src/runtime/dragon/jobs/dragontexturejobs.cpp +++ b/src/runtime/dragon/jobs/dragontexturejobs.cpp @@ -50,96 +50,22 @@ namespace Dragon { LoadedTextureImages loadTextureImages(LoadedTextureImages loadedTextureImages, const ValueContainer &textureImages) { - loadedTextureImages.nodes.reset(); + loadedTextureImages.reset(); if (!textureImages.anythingDirty()) return loadedTextureImages; - // TODO unfortunate that we ended up capturing loadedTextureImages here just to be able to - // modify `created` - auto generateTexture = [&loadedTextureImages](const QNodeId &id, - const Immutable &textureImage) { + auto generateTexture = [](const QNodeId &id, const Immutable &textureImage) { Q_UNUSED(id); - auto gen = [](QTextureImageDataGeneratorPtr generator) { return (*generator)(); }; LoadedTextureImage info; - info.generator = textureImage->generator; info.image = textureImage; - - // Note: This does not and should not use the same thread pool as the jobs, - // because the pool size could be 1, in which case this job might already - // have filled the entire pool and we cannot wait for it to finish - info.future = QtConcurrent::run(gen, textureImage->generator); - - // TODO remove once we got it working properly - info.future.waitForFinished(); - info.data = info.future.result(); - - loadedTextureImages.loadingImages.push_back(id); + info.data = (*textureImage->generator)(); return info; }; - // TODO add this back, but make sure that things like parentId are set correctly when texture - // images are shared. -// auto compare = [](const Value &cached, -// const Value &textureImage) { -// return textureImage->generator != nullptr && cached->generator != nullptr -// && *textureImage->generator == *cached->generator; -// }; -// loadedTextureImages.nodes = synchronizeKeys(std::move(loadedTextureImages.nodes), textureImages, -// compare, generateTexture); - - - loadedTextureImages.nodes = synchronizeKeys(std::move(loadedTextureImages.nodes), textureImages, - generateTexture); - -// for (const auto &id : textureImages.keys()) { -// const auto textureImage = textureImages[id]; -// loadedTextureImages.nodes[id] = generateTexture(id, textureImage); -// loadedTextureImages.nodes.markDirty(id); -// } - -// TODO Remove results that were completed last frame to save memory -// const auto &pendingCleanup = loadedTextureImages.loadedImages; -// for (const auto &cleanupId : pendingCleanup) { -// if (!loadedTextureImages.nodes.contains(cleanupId)) -// continue; -// auto &node = loadedTextureImages.nodes[cleanupId]; -// node->data.reset(); -// Q_ASSERT(node->status == LoadedTextureImage::Status::Loaded); -// node->status = LoadedTextureImage::Status::Cleared; -// } - -// loadedTextureImages.loadedImages = {}; - -// QVector completed; -// const auto &pendingLoad = loadedTextureImages.loadingImages; -// for (const auto &pendingId : pendingLoad) { -// if (!loadedTextureImages.nodes.contains(pendingId)) { -// // might have been removed in synchronizeKeys if texture was deleted -// completed.push_back(pendingId); -// continue; -// } -// auto &node = loadedTextureImages.nodes[pendingId]; -// if (!node->image->loadWithinFrame) { -// node->future.waitForFinished(); -// } -// if (node->future.isResultReadyAt(0)) { -// Q_ASSERT(node->status == LoadedTextureImage::Status::Loading); -// node->status = LoadedTextureImage::Status::Loaded; -// node->data = node->future.result(); -// node->future = {}; // Clear the future now that we have the data - -// loadedTextureImages.nodes.markDirty(pendingId); - -// completed.push_back(pendingId); -// } -// } - -// // Make sure those that are completed are removed next frame (to save memory) -// for (const auto &completedId : completed) { -// loadedTextureImages.loadingImages.removeOne(completedId); -// loadedTextureImages.loadedImages.push_back(completedId); -// } + loadedTextureImages = synchronizeKeys(std::move(loadedTextureImages), + textureImages, + generateTexture); return loadedTextureImages; } @@ -149,17 +75,16 @@ LoadedTextures loadTextures(LoadedTextures loadedTextures, const ValueContainer< { loadedTextures.reset(); - if (!textures.anythingDirty() && !loadedImages.nodes.anythingDirty()) + if (!textures.anythingDirty() && !loadedImages.anythingDirty()) return loadedTextures; auto generateTexture = [&loadedImages](const QNodeId &id, const Immutable &texture) { LoadedTexture loadedTexture; - loadedTexture.generator = texture->generator; loadedTexture.texture = texture; - for (const auto &loadedImageId : loadedImages.nodes.keys()) { + for (const auto &loadedImageId : loadedImages.keys()) { // TODO avoid lookup - const auto &loadedImage = loadedImages.nodes[loadedImageId]; + const auto &loadedImage = loadedImages[loadedImageId]; if (loadedImage->image->parentId == id) { loadedTexture.images.push_back(loadedImage); } @@ -172,10 +97,6 @@ LoadedTextures loadTextures(LoadedTextures loadedTextures, const ValueContainer< qWarning() << "[Qt3DRender::GLTexture] When a texture provides a generator, it's " "target is expected to be TargetAutomatic"; - // TODO consider adding async function back - // auto generate = [](QTextureGeneratorPtr generator) { return (*generator)(); }; - // loadedTexture.future = QtConcurrent::run(generate, texture->generator); - loadedTexture.data = (*texture->generator)(); return loadedTexture; @@ -187,7 +108,7 @@ LoadedTextures loadTextures(LoadedTextures loadedTextures, const ValueContainer< // TODO this is not the cleanest approach to finding dirty texture images that affect our // texture. Consider storing a hash of textures using different images, or implementing // more sophisticated dependency tracking between different types of nodes. - const auto &dirtyOrNewImages = loadedImages.nodes.dirtyOrNew(); + const auto &dirtyOrNewImages = loadedImages.dirtyOrNew(); for (const auto &textureId : loadedTextures.keys()) { const auto &loadedTexture = loadedTextures[textureId]; for (const auto &loadedTextureImage : loadedTexture->images) { @@ -197,19 +118,6 @@ LoadedTextures loadTextures(LoadedTextures loadedTextures, const ValueContainer< } } } - -// for (const auto &id : textures.keys()) { -// const auto &texture = textures[id]; -// loadedTextures[id] = generateTexture(id, texture); -// loadedTextures.markDirty(id); -// } - - // for (auto &loadingTexture : loadedTextures) { - // if (!loadingTexture->texture->isLoadWithinFrameEnabled) { - // loadingTexture->future.waitForFinished(); - // } - // } - return loadedTextures; } diff --git a/src/runtime/dragon/jobs/dragontexturejobs_p.h b/src/runtime/dragon/jobs/dragontexturejobs_p.h index 5539c1b..e81609b 100644 --- a/src/runtime/dragon/jobs/dragontexturejobs_p.h +++ b/src/runtime/dragon/jobs/dragontexturejobs_p.h @@ -53,18 +53,8 @@ namespace Dragon { class Texture; struct LoadedTexture; -// using TextureImageContainer = ValueContainer; - -struct LoadedTextureImages -{ - ValueContainer nodes; -// QVector> result; - QVector loadingImages; - QVector loadedImages; -}; - -// TODO explicitly state ValueContainer using LoadedTextures = ValueContainer; +using LoadedTextureImages = ValueContainer; LoadedTextureImages loadTextureImages(LoadedTextureImages loadedTextureImages, const ValueContainer &textureImages); -- cgit v1.2.3