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 <andy.nichols@qt.io>

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> &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> &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> &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<QPropertyUpdatedChange>(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> texture;
-    QTextureGeneratorPtr generator;
     QVector<Immutable<LoadedTextureImage>> images;
-//    QFuture<QTextureDataPtr> 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<TextureImage> image;
-    QTextureImageDataGeneratorPtr generator;
-    QFuture<QTextureImageDataPtr> 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<TextureImage> &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> &textureImage) {
+    auto generateTexture = [](const QNodeId &id, const Immutable<TextureImage> &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<LoadedTextureImage> &cached,
-//                      const Value<TextureImage> &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<QNodeId> 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> &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<LoadedTextureImage>;
-
-struct LoadedTextureImages
-{
-    ValueContainer<LoadedTextureImage> nodes;
-//    QVector<Value<LoadedTextureImageData>> result;
-    QVector<Qt3DCore::QNodeId> loadingImages;
-    QVector<Qt3DCore::QNodeId> loadedImages;
-};
-
-// TODO explicitly state ValueContainer
 using LoadedTextures = ValueContainer<LoadedTexture>;
+using LoadedTextureImages = ValueContainer<LoadedTextureImage>;
 
 LoadedTextureImages loadTextureImages(LoadedTextureImages loadedTextureImages,
                                       const ValueContainer<TextureImage> &textureImages);