From 2baa0cbb6dfc17fdf971ba33de4cc018e5aa822d Mon Sep 17 00:00:00 2001
From: Emil Axelsson <nils.emil.axelsson@gmail.com>
Date: Fri, 22 Dec 2017 00:15:23 +0100
Subject: [PATCH] Fix bug with assets changing state prematurely, while loading

---
 include/openspace/scene/asset.h | 20 ++++++++++++++-----
 src/scene/asset.cpp             | 35 ++++++++++++++++++++-------------
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/include/openspace/scene/asset.h b/include/openspace/scene/asset.h
index 9c3639a8c1..0f9ffddb26 100644
--- a/include/openspace/scene/asset.h
+++ b/include/openspace/scene/asset.h
@@ -84,27 +84,37 @@ public:
 
     void syncStateChanged(ResourceSynchronization::State s);
 
+    /**
+     * Load this asset and return true if successful,
+     * i.e. if this and all required assets loaded without errors.
+     */
+    bool load();
     bool hasLoadedParent() const;
-    void load();
     bool isLoaded() const;
     void unload();
     void unloadIfUnwanted();
 
-    // Sync
+    /**
+     * Start synchronizations of this asset and return true if all
+     * its own synchronizations and required assets' synchronizations could start.
+     */
+    bool startSynchronizations();
     bool hasSyncingOrResolvedParent() const;
     bool isSynchronized() const;
     bool isSyncingOrResolved() const;
-    bool startSynchronizations();
     bool cancelAllSynchronizations();
     bool cancelUnwantedSynchronizations();
     bool restartAllSynchronizations();
     float requestedSynchronizationProgress();
     float requiredSynchronizationProgress();
 
-    // Init
+    /**
+     * Initialize this asset and return true if successful,
+     * i.e. if this and all required assets initialized without errors.
+     */
+    bool initialize();
     bool hasInitializedParent() const;
     bool isInitialized() const;
-    void initialize();
     void deinitialize();
     void deinitializeIfUnwanted();
 
diff --git a/src/scene/asset.cpp b/src/scene/asset.cpp
index cdf459c76d..06b4cf7332 100644
--- a/src/scene/asset.cpp
+++ b/src/scene/asset.cpp
@@ -129,6 +129,11 @@ void Asset::setState(Asset::State state) {
 void Asset::requiredAssetChangedState(std::shared_ptr<Asset> child,
     Asset::State childState)
 {
+    if (!isLoaded()) {
+        // Prohibit state change to SyncResolved if additional requirements
+        // may still be added.
+        return;
+    }
     if (isInitialized()) {
         // Do not do anything if this asset was already initialized.
         // This may happen if there are multiple requirement paths from
@@ -344,7 +349,8 @@ bool Asset::isInitialized() const {
 
 bool Asset::startSynchronizations() {
     if (!isLoaded()) {
-        LERROR("Cannot start synchronizations of unloaded asset " << id());
+        LWARNING("Cannot start synchronizations of unloaded asset " << id());
+        return false;
     }
     for (auto& child : requestedAssets()) {
         child->startSynchronizations();
@@ -357,27 +363,26 @@ bool Asset::startSynchronizations() {
 
     setState(State::Synchronizing);
 
-    bool foundUnresolved = false;
+    bool childFailed = false;
 
     // Start synchronization of all children first.
     for (auto& child : requiredAssets()) {
-        if (child->startSynchronizations()) {
-            foundUnresolved = true;
+        if (!child->startSynchronizations()) {
+            childFailed = true;
         }
     }
 
     // Now synchronize its own synchronizations.
     for (const auto& s : ownSynchronizations()) {
         if (!s->isResolved()) {
-            foundUnresolved = true;
             s->start();
         }
     }
     // If all syncs are resolved (or no syncs exist), mark as resolved.
-    if (!isInitialized() && !foundUnresolved) {
+    if (!isInitialized() && isSyncResolveReady()) {
         setState(State::SyncResolved);
     }
-    return foundUnresolved;
+    return !childFailed;
 }
 
 bool Asset::cancelAllSynchronizations() {
@@ -440,13 +445,14 @@ float Asset::requestedSynchronizationProgress() {
     return syncProgress(assets);
 }
 
-void Asset::load() {
+bool Asset::load() {
     if (isLoaded()) {
-        return;
+        return true;
     }
 
     bool loaded = loader()->loadAsset(shared_from_this());
     setState(loaded ? State::Loaded : State::LoadingFailed);
+    return loaded;
 }
 
 void Asset::unload() {
@@ -472,13 +478,13 @@ void Asset::unloadIfUnwanted() {
     unload();
 }
 
-void Asset::initialize() {
+bool Asset::initialize() {
     if (isInitialized()) {
-        return;
+        return true;
     }
     if (!isSynchronized()) {
         LERROR("Cannot initialize unsynchronized asset " << id());
-        return;
+        return false;
     }
     LDEBUG("Initializing asset " << id());
 
@@ -501,7 +507,7 @@ void Asset::initialize() {
         LERROR("Failed to initialize asset " << id() << ". " <<
             e.component << ": " << e.message);
         // TODO: rollback;
-        return;
+        return false;
     }
 
     // 4. Update state
@@ -516,6 +522,7 @@ void Asset::initialize() {
                 child->id() << " of " << id() << ". " <<
                 e.component << ": " << e.message);
             // TODO: rollback;
+            return false;
         }
     }
 
@@ -547,7 +554,7 @@ void Asset::initialize() {
             }
         }
     }
-
+    return true;
 }
 
 void Asset::deinitializeIfUnwanted() {