From fddf2cabab581e71bf04f0f91d468cd1384de345 Mon Sep 17 00:00:00 2001
From: Deukhoofd <deukhoofd@gmail.com>
Date: Fri, 5 Jun 2020 18:06:45 +0200
Subject: [PATCH] Rework ScriptIterator to jump to first value on reset.

---
 .../ScriptHandling/ScriptAggregator.hpp       | 47 +++++++++++--------
 src/Battling/ScriptHandling/ScriptSet.hpp     |  2 +-
 src/Battling/ScriptHandling/ScriptWrapper.hpp | 13 +++--
 .../ScriptTests/ScriptAggregatorTests.cpp     | 17 ++++---
 .../ScriptTests/ScriptSourceTest.cpp          |  9 ++--
 5 files changed, 51 insertions(+), 37 deletions(-)

diff --git a/src/Battling/ScriptHandling/ScriptAggregator.hpp b/src/Battling/ScriptHandling/ScriptAggregator.hpp
index 7910b41..da329ac 100644
--- a/src/Battling/ScriptHandling/ScriptAggregator.hpp
+++ b/src/Battling/ScriptHandling/ScriptAggregator.hpp
@@ -14,6 +14,25 @@ namespace CreatureLib::Battling {
         size_t _index = 0;
         size_t _setIndex = 0;
 
+        inline void IncrementToNextNotNull(bool initialIncrement = true) {
+            if (_scripts[_index].IsSet()) {
+                _setIndex++;
+                if (_setIndex >= _scripts[_index].GetScriptSet()->Count()) {
+                    _setIndex = 0;
+                } else {
+                    return;
+                }
+            }
+            if (initialIncrement)
+                _index++;
+            while (HasNext()) {
+                if (_scripts[_index].HasValue()) {
+                    return;
+                }
+                _index++;
+            }
+        }
+
     public:
         ScriptAggregator(){};
         explicit ScriptAggregator(const ArbUt::List<ScriptWrapper>& scripts)
@@ -21,11 +40,12 @@ namespace CreatureLib::Battling {
 
         inline void Reset() {
             _index = 0;
-            _setIndex = 0;
+            _setIndex = -1;
+            IncrementToNextNotNull(false);
         }
         inline bool HasNext() { return _index < _size; }
 
-        Script* GetNextNotNull() {
+        ArbUt::BorrowedPtr<Script> GetNextNotNull() {
             while (HasNext()) {
                 auto s = GetNext();
                 if (s != nullptr) {
@@ -35,28 +55,17 @@ namespace CreatureLib::Battling {
             return nullptr;
         }
 
-        Script* GetNext() {
+        ArbUt::BorrowedPtr<Script> GetNext() {
             auto& current = _scripts[_index];
             if (!current.IsSet()) {
-                _index++;
                 auto s = current.GetScript();
-                if (s == nullptr)
-                    return nullptr;
-                return (*s).get();
+                IncrementToNextNotNull();
+                return (*s);
             } else {
                 auto& set = current.GetScriptSet()->GetIterator();
-                auto count = set.Count();
-                if (_setIndex >= count) {
-                    _index++;
-                    _setIndex = 0;
-                    return nullptr;
-                }
-                auto v = set[_setIndex++];
-                if (_setIndex >= count) {
-                    _index++;
-                    _setIndex = 0;
-                }
-                return v.GetRaw();
+                auto v = set[_setIndex];
+                IncrementToNextNotNull();
+                return v;
             }
         }
     };
diff --git a/src/Battling/ScriptHandling/ScriptSet.hpp b/src/Battling/ScriptHandling/ScriptSet.hpp
index adde538..39942bd 100644
--- a/src/Battling/ScriptHandling/ScriptSet.hpp
+++ b/src/Battling/ScriptHandling/ScriptSet.hpp
@@ -68,7 +68,7 @@ namespace CreatureLib::Battling {
 
         inline size_t Count() const { return _scripts.Count(); }
 
-        const ArbUt::UniquePtrList<Script>& GetIterator() const { return _scripts; }
+        inline const ArbUt::UniquePtrList<Script>& GetIterator() const { return _scripts; }
     };
 }
 
diff --git a/src/Battling/ScriptHandling/ScriptWrapper.hpp b/src/Battling/ScriptHandling/ScriptWrapper.hpp
index cacd2b8..fa4f34d 100644
--- a/src/Battling/ScriptHandling/ScriptWrapper.hpp
+++ b/src/Battling/ScriptHandling/ScriptWrapper.hpp
@@ -21,10 +21,17 @@ namespace CreatureLib::Battling {
         static inline ScriptWrapper FromScript(std::unique_ptr<Script>* s) { return ScriptWrapper(s, false); }
         static inline ScriptWrapper FromSet(ScriptSet* s) { return ScriptWrapper(s, true); }
 
-        bool IsSet() const { return _isSet; }
+        inline bool IsSet() const noexcept { return _isSet; }
 
-        inline const std::unique_ptr<Script>* GetScript() const { return _script; }
-        inline const ScriptSet* GetScriptSet() const { return _scriptSet; }
+        inline const std::unique_ptr<Script>* GetScript() const noexcept { return _script; }
+        inline const ScriptSet* GetScriptSet() const noexcept { return _scriptSet; }
+
+        inline bool HasValue() const noexcept {
+            if (_isSet)
+                return _scriptSet->Count() > 0;
+            else
+                return *_script != nullptr;
+        }
     };
 }
 
diff --git a/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp b/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp
index 79f5ee5..12f5bfa 100644
--- a/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp
+++ b/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp
@@ -27,7 +27,7 @@ TEST_CASE("Script Aggregator properly iterates containing script.", "[Battling,
     while (aggr.HasNext()) {
         auto next = aggr.GetNext();
         REQUIRE(next != nullptr);
-        dynamic_cast<TestScript*>(next)->TestMethod(ran);
+        next.As<TestScript>()->TestMethod(ran);
     }
     CHECK(ran == 1);
 }
@@ -45,7 +45,7 @@ TEST_CASE("Script Aggregator properly iterates multiple scripts.", "[Battling, S
     while (aggr.HasNext()) {
         auto next = aggr.GetNext();
         REQUIRE(next != nullptr);
-        dynamic_cast<TestScript*>(next)->TestMethod(ran);
+        next.As<TestScript>()->TestMethod(ran);
     }
     CHECK(ran == 3);
 }
@@ -64,7 +64,7 @@ TEST_CASE("Script Aggregator properly iterates Script Set.", "[Battling, Scripti
     while (aggr.HasNext()) {
         auto next = aggr.GetNextNotNull();
         REQUIRE(next != nullptr);
-        dynamic_cast<TestScript*>(next)->TestMethod(ran);
+        next.As<TestScript>()->TestMethod(ran);
     }
     CHECK(ran == 3);
 }
@@ -83,7 +83,7 @@ TEST_CASE("Script Aggregator properly iterates data of Script Set and Script.",
     while (aggr.HasNext()) {
         auto next = aggr.GetNextNotNull();
         REQUIRE(next != nullptr);
-        dynamic_cast<TestScript*>(next)->TestMethod(ran);
+        next.As<TestScript>()->TestMethod(ran);
     }
     CHECK(ran == 3);
 }
@@ -102,7 +102,7 @@ TEST_CASE("Script Aggregator properly iterates data of Script and Script Set.",
     while (aggr.HasNext()) {
         auto next = aggr.GetNextNotNull();
         REQUIRE(next != nullptr);
-        dynamic_cast<TestScript*>(next)->TestMethod(ran);
+        next.As<TestScript>()->TestMethod(ran);
     }
     CHECK(ran == 3);
 }
@@ -123,7 +123,7 @@ TEST_CASE("Script Aggregator properly iterates data of Script, Script Set and Sc
     while (aggr.HasNext()) {
         auto next = aggr.GetNextNotNull();
         REQUIRE(next != nullptr);
-        dynamic_cast<TestScript*>(next)->TestMethod(ran);
+        next.As<TestScript>()->TestMethod(ran);
     }
     CHECK(ran == 4);
 }
@@ -143,12 +143,11 @@ TEST_CASE("Script Aggregator properly iterates empty Script Set.", "[Battling, S
     auto set = ScriptSet();
     auto vec = ArbUt::List<ScriptWrapper>{ScriptWrapper::FromSet(&set)};
     auto aggr = ScriptAggregator(vec);
+    aggr.Reset();
     while (aggr.HasNext()) {
-        auto next = aggr.GetNext();
-        REQUIRE(next == nullptr);
         ran++;
     }
-    CHECK(ran == 1);
+    CHECK(ran == 0);
 }
 
 #endif
\ No newline at end of file
diff --git a/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp b/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp
index 6b5707a..6636dd0 100644
--- a/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp
+++ b/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp
@@ -59,19 +59,18 @@ TEST_CASE("Script source with script ptr being set.", "[Battling, Scripting]") {
 TEST_CASE("Script source with script ptr being set after first iteration.", "[Battling, Scripting]") {
     auto source = ScriptSourceWithScriptPtr();
     auto scripts = source.GetScriptIterator();
-    auto first = scripts.GetNext();
-    CHECK(first == nullptr);
+    REQUIRE_FALSE(scripts.HasNext());
     source.ScriptPtr = std::make_unique<TestScript>("foobar");
     scripts = source.GetScriptIterator();
-    first = scripts.GetNext();
+    auto first = scripts.GetNext();
     CHECK(first != nullptr);
 }
 
 TEST_CASE("Script source with empty script set.", "[Battling, Scripting]") {
     auto source = ScriptSourceWithScriptSet();
     auto scripts = source.GetScriptIterator();
-    auto first = scripts.GetNext();
-    CHECK(first == nullptr);
+    scripts.Reset();
+    REQUIRE_FALSE(scripts.HasNext());
 }
 
 TEST_CASE("Script source with single item script set.", "[Battling, Scripting]") {