From be10b3515c50034c1bba34f08e98af1d6b86222b Mon Sep 17 00:00:00 2001 From: Deukhoofd Date: Sun, 18 Apr 2021 12:50:48 +0200 Subject: [PATCH] Reworks script aggregator. Cleans up API and code, and now handles scripts being removed from a set while we're iterating. Signed-off-by: Deukhoofd --- .../ScriptHandling/ScriptAggregator.hpp | 57 +++++---------- src/Battling/ScriptHandling/ScriptMacros.hpp | 4 +- .../ScriptTests/ScriptAggregatorTests.cpp | 71 ++++++++++++------- .../ScriptTests/ScriptSourceTest.cpp | 31 ++++---- 4 files changed, 83 insertions(+), 80 deletions(-) diff --git a/src/Battling/ScriptHandling/ScriptAggregator.hpp b/src/Battling/ScriptHandling/ScriptAggregator.hpp index e8711d3..4a41a4a 100644 --- a/src/Battling/ScriptHandling/ScriptAggregator.hpp +++ b/src/Battling/ScriptHandling/ScriptAggregator.hpp @@ -9,73 +9,54 @@ namespace CreatureLib::Battling { class ScriptAggregator { const ScriptWrapper* _scripts; i32 _size; - i32 _index = 0; - i32 _setIndex = 0; + i32 _index = -1; + i32 _setIndex = -1; - inline void IncrementToNextNotNull(bool initialIncrement = true) { - if (_scripts[_index].IsSet()) { + inline bool IncrementToNextNotNull() { + if (_index != -1 && _scripts[_index].IsSet()) { _setIndex++; if (_setIndex >= (i32)_scripts[_index].GetScriptSet()->Count()) { _setIndex = -1; } else { - return; + return true; } } - if (initialIncrement) - _index++; - while (HasNext()) { + for (_index++; _index < _size; ++_index) { if (_scripts[_index].HasValue()) { if (_scripts[_index].IsSet()) { - _setIndex = -1; + _setIndex = 0; } - return; + return true; } - _index++; } + return false; } public: ScriptAggregator(){}; explicit ScriptAggregator(const ArbUt::List& scripts) - : _scripts(scripts.RawData()), _size(scripts.Count()) { - Reset(); - }; + : _scripts(scripts.RawData()), _size(scripts.Count()){}; inline void Reset() { - _index = 0; - if (_size > 0) { - if (_scripts[_index].IsSet()) { - _setIndex = -1; - } - IncrementToNextNotNull(false); - } + _index = -1; + _setIndex = -1; } - inline bool HasNext() { return _index < _size; } - - std::optional> GetNextNotNull() { - while (HasNext()) { - auto s = GetNext(); - return s; + bool GetNext(ArbUt::BorrowedPtr& out) { + if (!IncrementToNextNotNull()) { + return false; } - return {}; - } - - ArbUt::BorrowedPtr GetNext() { auto& current = _scripts[_index]; if (!current.IsSet()) { auto s = current.GetScript(); - IncrementToNextNotNull(); - return (*s); + out = *s; + return true; } else { auto& set = current.GetScriptSet()->GetIterator(); - if (_setIndex == -1) { - _setIndex = 0; - } auto v = set[_setIndex]; - IncrementToNextNotNull(); - return v; + out = v; + return true; } } }; diff --git a/src/Battling/ScriptHandling/ScriptMacros.hpp b/src/Battling/ScriptHandling/ScriptMacros.hpp index 8664271..b2dd1cc 100644 --- a/src/Battling/ScriptHandling/ScriptMacros.hpp +++ b/src/Battling/ScriptHandling/ScriptMacros.hpp @@ -2,8 +2,8 @@ { \ try { \ auto aggregator = source->GetScriptIterator(); \ - while (aggregator.HasNext()) { \ - auto next = aggregator.GetNext(); \ + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; \ + while (aggregator.GetNext(next)) { \ try { \ next->hookName(__VA_ARGS__); \ } catch (const std::exception& e) { \ diff --git a/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp b/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp index 7b53bc4..61d6954 100644 --- a/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp +++ b/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp @@ -27,8 +27,8 @@ TEST_CASE("Script Aggregator properly iterates containing script.") { auto vec = ArbUt::List{ ScriptWrapper::FromScript(reinterpret_cast*>(&script))}; auto aggr = ScriptAggregator(vec); - while (aggr.HasNext()) { - auto next = aggr.GetNext(); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (aggr.GetNext(next)) { next.As()->TestMethod(ran); } CHECK(ran == 1); @@ -44,8 +44,8 @@ TEST_CASE("Script Aggregator properly iterates multiple scripts.") { ScriptWrapper::FromScript(reinterpret_cast*>(&script2)), ScriptWrapper::FromScript(reinterpret_cast*>(&script3))}; auto aggr = ScriptAggregator(vec); - while (aggr.HasNext()) { - auto next = aggr.GetNext(); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (aggr.GetNext(next)) { next.As()->TestMethod(ran); } CHECK(ran == 3); @@ -62,13 +62,33 @@ TEST_CASE("Script Aggregator properly iterates Script Set.") { set.Add(script3); auto vec = ArbUt::List{ScriptWrapper::FromSet(&set)}; auto aggr = ScriptAggregator(vec); - while (aggr.HasNext()) { - auto next = aggr.GetNextNotNull(); - next.value().As()->TestMethod(ran); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (aggr.GetNext(next)) { + next.As()->TestMethod(ran); } CHECK(ran == 3); } +TEST_CASE("Script Aggregator properly iterates Script Set when removing from it.") { + BattleScript* script = new TestScript("test"); + BattleScript* script2 = new TestScript("test2"); + BattleScript* script3 = new TestScript("test3"); + auto ran = 0; + auto set = ScriptSet(); + set.Add(script); + set.Add(script2); + set.Add(script3); + auto vec = ArbUt::List{ScriptWrapper::FromSet(&set)}; + auto aggr = ScriptAggregator(vec); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (ran <= 2) { + aggr.GetNext(next); + next.As()->TestMethod(ran); + } + set.Remove("test3"_cnc); + REQUIRE_FALSE(aggr.GetNext(next)); +} + TEST_CASE("Script Aggregator properly iterates data of Script Set and Script.") { auto script = std::make_unique("test"); BattleScript* script2 = new TestScript("test2"); @@ -81,9 +101,9 @@ TEST_CASE("Script Aggregator properly iterates data of Script Set and Script.") ScriptWrapper::FromSet(&set), ScriptWrapper::FromScript(reinterpret_cast*>(&script))}; auto aggr = ScriptAggregator(vec); - while (aggr.HasNext()) { - auto next = aggr.GetNextNotNull(); - next.value().As()->TestMethod(ran); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (aggr.GetNext(next)) { + next.As()->TestMethod(ran); } CHECK(ran == 3); } @@ -100,9 +120,9 @@ TEST_CASE("Script Aggregator properly iterates data of Script and Script Set.") ArbUt::List{ScriptWrapper::FromScript(reinterpret_cast*>(&script)), ScriptWrapper::FromSet(&set)}; auto aggr = ScriptAggregator(vec); - while (aggr.HasNext()) { - auto next = aggr.GetNextNotNull(); - next.value().As()->TestMethod(ran); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (aggr.GetNext(next)) { + next.As()->TestMethod(ran); } CHECK(ran == 3); } @@ -121,9 +141,9 @@ TEST_CASE("Script Aggregator properly iterates data of Script, Script Set and Sc ScriptWrapper::FromSet(&set), ScriptWrapper::FromScript(reinterpret_cast*>(&script4))}; auto aggr = ScriptAggregator(vec); - while (aggr.HasNext()) { - auto next = aggr.GetNextNotNull(); - next.value().As()->TestMethod(ran); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (aggr.GetNext(next)) { + next.As()->TestMethod(ran); } CHECK(ran == 4); } @@ -143,17 +163,16 @@ TEST_CASE("Script Aggregator properly iterates multiple script sets.") { }; auto aggr = ScriptAggregator(vec); auto ran = 0; - while (aggr.HasNext()) { - auto next = aggr.GetNextNotNull(); - next.value().As()->TestMethod(ran); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (aggr.GetNext(next)) { + next.As()->TestMethod(ran); } CHECK(ran == 3); aggr.Reset(); ran = 0; - while (aggr.HasNext()) { - auto next = aggr.GetNextNotNull(); - next.value().As()->TestMethod(ran); + while (aggr.GetNext(next)) { + next.As()->TestMethod(ran); } CHECK(ran == 3); } @@ -162,9 +181,8 @@ TEST_CASE("Script Aggregator properly iterates when empty.") { auto ran = 0; auto vec = ArbUt::List{}; auto aggr = ScriptAggregator(vec); - while (aggr.HasNext()) { - THROW("Aggregator returned a script, but should have been empty."); - } + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + REQUIRE_FALSE(aggr.GetNext(next)); CHECK(ran == 0); } @@ -174,7 +192,8 @@ TEST_CASE("Script Aggregator properly iterates empty Script Set.") { auto vec = ArbUt::List{ScriptWrapper::FromSet(&set)}; auto aggr = ScriptAggregator(vec); aggr.Reset(); - while (aggr.HasNext()) { + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + while (aggr.GetNext(next)) { ran++; } CHECK(ran == 0); diff --git a/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp b/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp index 81b3d42..569f8fa 100644 --- a/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp +++ b/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp @@ -43,30 +43,34 @@ protected: TEST_CASE("Script source with unset script ptr.") { auto source = ScriptSourceWithScriptPtr(); auto scripts = source.GetScriptIterator(); - REQUIRE_FALSE(scripts.HasNext()); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + REQUIRE_FALSE(scripts.GetNext(next)); } TEST_CASE("Script source with script ptr being set.") { auto source = ScriptSourceWithScriptPtr(); source.ScriptPtr = std::make_unique("foobar"); auto scripts = source.GetScriptIterator(); - [[maybe_unused]] auto first = scripts.GetNext(); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + REQUIRE(scripts.GetNext(next)); } TEST_CASE("Script source with script ptr being set after first iteration.") { auto source = ScriptSourceWithScriptPtr(); auto scripts = source.GetScriptIterator(); - REQUIRE_FALSE(scripts.HasNext()); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + REQUIRE_FALSE(scripts.GetNext(next)); source.ScriptPtr = std::make_unique("foobar"); scripts = source.GetScriptIterator(); - [[maybe_unused]] auto first = scripts.GetNext(); + REQUIRE(scripts.GetNext(next)); } TEST_CASE("Script source with empty script set.") { auto source = ScriptSourceWithScriptSet(); auto scripts = source.GetScriptIterator(); scripts.Reset(); - REQUIRE_FALSE(scripts.HasNext()); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + REQUIRE_FALSE(scripts.GetNext(next)); } TEST_CASE("Script source with single item script set.") { @@ -74,9 +78,9 @@ TEST_CASE("Script source with single item script set.") { auto s = new TestScript("foobar"); source.Set.Add(s); auto scripts = source.GetScriptIterator(); - auto first = scripts.GetNextNotNull(); - REQUIRE(first.has_value()); - CHECK(first.value()->GetName() == "foobar"); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + REQUIRE(scripts.GetNext(next)); + CHECK(next->GetName() == "foobar"); } TEST_CASE("Script source with multiple item script set.") { @@ -86,12 +90,11 @@ TEST_CASE("Script source with multiple item script set.") { source.Set.Add(s); source.Set.Add(s2); auto scripts = source.GetScriptIterator(); - auto first = scripts.GetNextNotNull(); - REQUIRE(first.has_value()); - CHECK(first.value()->GetName() == "foobar"); - auto second = scripts.GetNextNotNull(); - REQUIRE(second.has_value()); - CHECK(second.value()->GetName() == "foobar2"); + ArbUt::BorrowedPtr next = (CreatureLib::Battling::BattleScript*)1; + REQUIRE(scripts.GetNext(next)); + CHECK(next->GetName() == "foobar"); + REQUIRE(scripts.GetNext(next)); + CHECK(next->GetName() == "foobar2"); } #endif