From f61b626fad3ed2225b86c19a61b292813967c80e Mon Sep 17 00:00:00 2001
From: adazem009 <68537469+adazem009@users.noreply.github.com>
Date: Wed, 27 Dec 2023 13:48:43 +0100
Subject: [PATCH] Refactor script starting logic

---
 include/scratchcpp/iengine.h                  |   2 +-
 src/engine/internal/engine.cpp                | 143 ++++++++++--------
 src/engine/internal/engine.h                  |   4 +-
 test/engine/engine_test.cpp                   |  35 ++++-
 test/mocks/enginemock.h                       |   2 +-
 .../395_reset_running_hats.sb3                | Bin 0 -> 1017 bytes
 6 files changed, 114 insertions(+), 72 deletions(-)
 create mode 100644 test/regtest_projects/395_reset_running_hats.sb3

diff --git a/include/scratchcpp/iengine.h b/include/scratchcpp/iengine.h
index 7b52d008..fa05e8dd 100644
--- a/include/scratchcpp/iengine.h
+++ b/include/scratchcpp/iengine.h
@@ -54,7 +54,7 @@ class LIBSCRATCHCPP_EXPORT IEngine
         virtual void stop() = 0;
 
         /*! Starts a script with the given top level block as the given Target (a sprite or the stage). */
-        virtual void startScript(std::shared_ptr<Block> topLevelBlock, std::shared_ptr<Target> target) = 0;
+        virtual VirtualMachine *startScript(std::shared_ptr<Block> topLevelBlock, Target *) = 0;
 
         /*! Starts the script of the broadcast with the given index. */
         virtual void broadcast(unsigned int index, VirtualMachine *sourceScript, bool wait = false) = 0;
diff --git a/src/engine/internal/engine.cpp b/src/engine/internal/engine.cpp
index 3683761d..6f99df71 100644
--- a/src/engine/internal/engine.cpp
+++ b/src/engine/internal/engine.cpp
@@ -159,7 +159,7 @@ void Engine::start()
     for (auto target : m_targets) {
         auto gfBlocks = target->greenFlagBlocks();
         for (auto block : gfBlocks)
-            startScript(block, target);
+            startScript(block, target.get());
     }
 
     m_eventLoopMutex.unlock();
@@ -171,23 +171,27 @@ void Engine::stop()
     deleteClones();
 }
 
-void Engine::startScript(std::shared_ptr<Block> topLevelBlock, std::shared_ptr<Target> target)
+VirtualMachine *Engine::startScript(std::shared_ptr<Block> topLevelBlock, Target *target)
 {
     if (!topLevelBlock) {
         std::cout << "warning: starting a script with a null top level block (nothing will happen)" << std::endl;
-        return;
+        return nullptr;
     }
 
     if (!target) {
         std::cout << "error: scripts must be started by a target";
         assert(false);
-        return;
+        return nullptr;
     }
 
     if (topLevelBlock->next()) {
         auto script = m_scripts[topLevelBlock];
-        addRunningScript(script->start());
+        std::shared_ptr<VirtualMachine> vm = script->start(target);
+        addRunningScript(vm);
+        return vm.get();
     }
+
+    return nullptr;
 }
 
 void Engine::broadcast(unsigned int index, VirtualMachine *sourceScript, bool wait)
@@ -201,61 +205,22 @@ void Engine::broadcast(unsigned int index, VirtualMachine *sourceScript, bool wa
 void Engine::broadcastByPtr(Broadcast *broadcast, VirtualMachine *sourceScript, bool wait)
 {
     const std::vector<Script *> &scripts = m_broadcastMap[broadcast];
+    auto &runningBroadcasts = m_runningBroadcastMap[broadcast];
 
     for (auto script : scripts) {
-        std::vector<VirtualMachine *> runningBroadcastScripts;
-
-        for (const auto &[target, targetScripts] : m_runningScripts) {
-            for (auto vm : targetScripts) {
-                if (vm->script() == script) {
-                    runningBroadcastScripts.push_back(vm.get());
-                }
-            }
+        for (auto &pair : runningBroadcasts) {
+            if (pair.second->script() == script)
+                pair.first = sourceScript;
         }
 
-        // Reset running scripts
-        for (VirtualMachine *vm : runningBroadcastScripts) {
-            vm->reset();
-
-            // Remove the script from scripts to remove because it's going to run again
-            m_scriptsToRemove.erase(std::remove(m_scriptsToRemove.begin(), m_scriptsToRemove.end(), vm), m_scriptsToRemove.end());
-            assert(std::find(m_scriptsToRemove.begin(), m_scriptsToRemove.end(), vm) == m_scriptsToRemove.end());
-
-            auto &scripts = m_runningBroadcastMap[broadcast];
-
-            for (auto &pair : scripts) {
-                if (pair.second->script() == script)
-                    pair.first = sourceScript;
-            }
-
-            if (script == sourceScript->script())
-                sourceScript->stop(false, !wait); // source script is the broadcast script
-        }
-
-        // Start scripts which are not running
-        Target *root = script->target();
-        std::vector<Target *> targets = { root };
-
-        if (!root->isStage()) {
-            Sprite *sprite = dynamic_cast<Sprite *>(root);
-            assert(sprite);
-            assert(!sprite->isClone());
-            const auto &children = sprite->clones();
-
-            for (auto child : children)
-                targets.push_back(child.get());
-        }
+        if (script == sourceScript->script())
+            sourceScript->stop(false, !wait); // source script is the broadcast script
+    }
 
-        for (Target *target : targets) {
-            auto it = std::find_if(runningBroadcastScripts.begin(), runningBroadcastScripts.end(), [target](VirtualMachine *vm) { return vm->target() == target; });
+    std::vector<VirtualMachine *> startedScripts = startHats(scripts);
 
-            if (it == runningBroadcastScripts.end()) {
-                auto vm = script->start(target);
-                addRunningScript(vm);
-                m_runningBroadcastMap[broadcast].push_back({ sourceScript, vm.get() });
-            }
-        }
-    }
+    for (VirtualMachine *vm : startedScripts)
+        runningBroadcasts.push_back({ sourceScript, vm });
 }
 
 void Engine::stopScript(VirtualMachine *vm)
@@ -565,12 +530,12 @@ void Engine::setKeyState(const KeyEvent &event, bool pressed)
         auto it = m_whenKeyPressedScripts.find(event.name());
 
         if (it != m_whenKeyPressedScripts.cend())
-            startWhenKeyPressedScripts(it->second);
+            startHats(it->second);
 
         it = m_whenKeyPressedScripts.find("any");
 
         if (it != m_whenKeyPressedScripts.cend())
-            startWhenKeyPressedScripts(it->second);
+            startHats(it->second);
     }
 }
 
@@ -583,7 +548,7 @@ void Engine::setAnyKeyPressed(bool pressed)
         auto it = m_whenKeyPressedScripts.find("any");
 
         if (it != m_whenKeyPressedScripts.cend())
-            startWhenKeyPressedScripts(it->second);
+            startHats(it->second);
     }
 }
 
@@ -1249,18 +1214,66 @@ void Engine::addRunningScript(std::shared_ptr<VirtualMachine> vm)
     }
 }
 
-void Engine::startWhenKeyPressedScripts(const std::vector<Script *> &scripts)
+std::vector<VirtualMachine *> Engine::startHats(const std::vector<Script *> &scripts)
 {
+    std::vector<VirtualMachine *> startedScripts;
+
     for (auto script : scripts) {
-        std::shared_ptr<Block> block = nullptr;
+        std::vector<VirtualMachine *> runningScripts;
+
+        for (const auto &[target, targetScripts] : m_runningScripts) {
+            for (auto vm : targetScripts) {
+                if (vm->script() == script) {
+                    runningScripts.push_back(vm.get());
+                }
+            }
+        }
+
+        // Reset running scripts
+        for (VirtualMachine *vm : runningScripts) {
+            vm->reset();
+
+            // Remove the script from scripts to remove because it's going to run again
+            m_scriptsToRemove.erase(std::remove(m_scriptsToRemove.begin(), m_scriptsToRemove.end(), vm), m_scriptsToRemove.end());
+            assert(std::find(m_scriptsToRemove.begin(), m_scriptsToRemove.end(), vm) == m_scriptsToRemove.end());
+        }
+
+        // Start scripts which are not running
+        Target *root = script->target();
+        std::vector<Target *> targets = { root };
+
+        if (!root->isStage()) {
+            Sprite *sprite = dynamic_cast<Sprite *>(root);
+            assert(sprite);
+            assert(!sprite->isClone());
+            const auto &children = sprite->clones();
 
-        for (const auto &[b, s] : m_scripts) {
-            if (s.get() == script)
-                block = b;
+            for (auto child : children)
+                targets.push_back(child.get());
         }
 
-        assert(block);
-        assert(std::find_if(m_targets.begin(), m_targets.end(), [script](std::shared_ptr<Target> target) { return script->target() == target.get(); }) != m_targets.end());
-        startScript(block, *std::find_if(m_targets.begin(), m_targets.end(), [script](std::shared_ptr<Target> target) { return script->target() == target.get(); }));
+        for (Target *target : targets) {
+            std::shared_ptr<Block> block = nullptr;
+
+            if (!runningScripts.empty()) {
+                auto it = std::find_if(runningScripts.begin(), runningScripts.end(), [target, script](VirtualMachine *vm) { return (vm->target() == target) && (vm->script() == script); });
+
+                if (it != runningScripts.end())
+                    continue; // skip the script because it was already running and was reset
+            }
+
+            for (const auto &[b, s] : m_scripts) {
+                if (s.get() == script) {
+                    block = b;
+                    break;
+                }
+            }
+
+            assert(block);
+            assert(std::find_if(m_targets.begin(), m_targets.end(), [script](std::shared_ptr<Target> target) { return script->target() == target.get(); }) != m_targets.end());
+            startedScripts.push_back(startScript(block, target));
+        }
     }
+
+    return startedScripts;
 }
diff --git a/src/engine/internal/engine.h b/src/engine/internal/engine.h
index cf0e2300..bf5be563 100644
--- a/src/engine/internal/engine.h
+++ b/src/engine/internal/engine.h
@@ -32,7 +32,7 @@ class Engine : public IEngine
 
         void start() override;
         void stop() override;
-        void startScript(std::shared_ptr<Block> topLevelBlock, std::shared_ptr<Target> target) override;
+        VirtualMachine *startScript(std::shared_ptr<Block> topLevelBlock, Target *target) override;
         void broadcast(unsigned int index, VirtualMachine *sourceScript, bool wait = false) override;
         void broadcastByPtr(Broadcast *broadcast, VirtualMachine *sourceScript, bool wait = false) override;
         void stopScript(VirtualMachine *vm) override;
@@ -153,7 +153,7 @@ class Engine : public IEngine
 
         void updateFrameDuration();
         void addRunningScript(std::shared_ptr<VirtualMachine> vm);
-        void startWhenKeyPressedScripts(const std::vector<Script *> &scripts);
+        std::vector<VirtualMachine *> startHats(const std::vector<Script *> &scripts);
 
         std::unordered_map<std::shared_ptr<IBlockSection>, std::unique_ptr<BlockSectionContainer>> m_sections;
         std::vector<std::shared_ptr<Target>> m_targets;
diff --git a/test/engine/engine_test.cpp b/test/engine/engine_test.cpp
index caa4bf41..6aaea6fd 100644
--- a/test/engine/engine_test.cpp
+++ b/test/engine/engine_test.cpp
@@ -183,6 +183,7 @@ TEST(EngineTest, ExecutionOrder)
     ASSERT_EQ((*list)[9].toString(), "Sprite1 1 msg");
     ASSERT_EQ((*list)[10].toString(), "Sprite1 2 msg");
     ASSERT_EQ((*list)[11].toString(), "Sprite1 3 msg");
+    ASSERT_EQ((*list)[12].toString(), "Stage msg");
 }
 
 TEST(EngineTest, KeyState)
@@ -479,7 +480,7 @@ TEST(EngineTest, WhenKeyPressed)
     ASSERT_VAR(stage, "right_arrow_pressed");
     ASSERT_EQ(GET_VAR(stage, "right_arrow_pressed")->value().toInt(), 2);
     ASSERT_VAR(stage, "any_key_pressed");
-    ASSERT_EQ(GET_VAR(stage, "any_key_pressed")->value().toInt(), 9);
+    ASSERT_EQ(GET_VAR(stage, "any_key_pressed")->value().toInt(), 8);
     ASSERT_VAR(stage, "a_pressed");
     ASSERT_EQ(GET_VAR(stage, "a_pressed")->value().toInt(), 1);
     ASSERT_VAR(stage, "x_pressed");
@@ -1144,7 +1145,6 @@ TEST(EngineTest, CloneLimit)
     ASSERT_EQ(engine->cloneCount(), 0);
 }
 
-// TODO: Uncomment this after fixing #256 and #257
 TEST(EngineTest, BackdropBroadcasts)
 {
     // TODO: Set "infinite" FPS (#254)
@@ -1169,7 +1169,6 @@ TEST(EngineTest, BackdropBroadcasts)
     ASSERT_EQ(GET_VAR(stage, "test5")->value().toString(), "2 2 0 0");
 }
 
-// TODO: Uncomment this after fixing #256 and #257
 TEST(EngineTest, BroadcastsProject)
 {
     // TODO: Set "infinite" FPS (#254)
@@ -1287,3 +1286,33 @@ TEST(EngineTest, NoStopWhenCallingRunningBroadcastFromCustomBlock)
     ASSERT_VAR(stage, "passed2");
     ASSERT_TRUE(GET_VAR(stage, "passed2")->value().toBool());
 }
+
+TEST(EngineTest, ResetRunningHats)
+{
+    // Regtest for #395
+    Project p("regtest_projects/395_reset_running_hats.sb3");
+    ASSERT_TRUE(p.load());
+
+    auto engine = p.engine();
+
+    Stage *stage = engine->stage();
+    ASSERT_TRUE(stage);
+
+    engine->setKeyState(KeyEvent(KeyEvent::Type::Space), true);
+    engine->setKeyState(KeyEvent(KeyEvent::Type::Space), false);
+    engine->setKeyState(KeyEvent(KeyEvent::Type::Space), true);
+    engine->setKeyState(KeyEvent(KeyEvent::Type::Space), false);
+    engine->run();
+
+    ASSERT_VAR(stage, "test");
+    ASSERT_EQ(GET_VAR(stage, "test")->value().toInt(), 1);
+
+    engine->run();
+    ASSERT_EQ(GET_VAR(stage, "test")->value().toInt(), 1);
+
+    engine->setKeyState(KeyEvent(KeyEvent::Type::Space), true);
+    engine->setKeyState(KeyEvent(KeyEvent::Type::Space), false);
+    engine->run();
+
+    ASSERT_EQ(GET_VAR(stage, "test")->value().toInt(), 2);
+}
diff --git a/test/mocks/enginemock.h b/test/mocks/enginemock.h
index 655f8abf..8b39b797 100644
--- a/test/mocks/enginemock.h
+++ b/test/mocks/enginemock.h
@@ -16,7 +16,7 @@ class EngineMock : public IEngine
 
         MOCK_METHOD(void, start, (), (override));
         MOCK_METHOD(void, stop, (), (override));
-        MOCK_METHOD(void, startScript, (std::shared_ptr<Block>, std::shared_ptr<Target>), (override));
+        MOCK_METHOD(VirtualMachine *, startScript, (std::shared_ptr<Block>, Target *), (override));
         MOCK_METHOD(void, broadcast, (unsigned int, VirtualMachine *, bool), (override));
         MOCK_METHOD(void, broadcastByPtr, (Broadcast *, VirtualMachine *, bool), (override));
         MOCK_METHOD(void, stopScript, (VirtualMachine *), (override));
diff --git a/test/regtest_projects/395_reset_running_hats.sb3 b/test/regtest_projects/395_reset_running_hats.sb3
new file mode 100644
index 0000000000000000000000000000000000000000..2d00049be4f2202dcaca9d90c988956034b5557c
GIT binary patch
literal 1017
zcmWIWW@h1HU|`^2NZLIs+~uK&Z3GhoLkkN70}oKNpeR2pHMvADt2jTeHRNvoWdni#
z;S(wuHA6hF`Gm$T=<%DSar?-zBO#%Od)N1_Om^Qkd7IYX`#Xz+zI{9px9aoPod5gg
z*nYmV?tt~Q^WS`SNuLitP?Rs-G2@x&wKcC@*E|jT)XFsH?&kRY>aYC|+ZTQNR`d7c
z-HhLLHReA8-rkdG|6ud%V5Qj6&(24tl$0EoJKbs;Bl4$kt<Ofj^rByerHL%D+`1Y6
zH)uMpPYjPP;A2^IVZ+xS@0m`|ik3;-nKz|t#+A}yn_^AJ&VpyxPyRi8_3PWCS5*~@
zO!X&l<tl`%dvYizV&9Wfit+icH1DnI`?Fog-_Kp}_C_0D<ECFc3m(j~7g4>gZLult
z%|FFA4zh|$+toR4-mUnR+|)2B!>lLb(6i6!3*H~QuYYX*^2zRk@z;OvuW@?ZAl$lA
zi{;Ac5R+*S=1M-7-ek3`F`&Hi1@ndnN24Nw)jOI!=1WHxDm}d=y3NAsu|CT>?ulxV
zMk0k}lXQ+2?ew^8BIm^8%9doJ!mBm0lx?4x^$Wi3$BOlP?VRT%)Grb~{3G?dT&>j6
zc1x}VDPN&;8}fR&mQ0c`*VxBgoyvOSz$NEU>1p$mZhA33yH@nJDV%lIT-n<*YW$fb
z&Sspyc7N7vsRbI#GpdB+xNiEGhr4;_=lt*U=n0oy>>E2F;iR;yp8ff2lRb|dTP=R`
z#UjUT*TbrI@T8S)%-)%0Wp5tz!ze`cMCeQB&V%Qi))*_jN~u^M`)sb$qi?gXByNcR
zn5)0IBz}pu&X2lzGZyPCkezk$bzCDq|4qlMlkU7p5#UeyRTr|OgFQ21LB+$^4Tm4<
z2cV?Dj&lNxlYuGk6cDQbadL{0p{b!sih-%IVOmOBT9T2Gk%397L29yTYKnzPqF!-X
zdakcGU$cV9o$9XZ+u4t-GdZ-#)Bl3-7O$3&qX$-BxP3ipx^J4{%zWwVjA6oyK5uq9
zsp?c@8`&z_WSX~O%jK&xOFl*_PTsJv$@lbS3-{J(8JR5}njcG^+3_sWIvtdA*nP>#
z#k1%CZ`!mjd*kjE+zsE3NW^@8#5_|@E`LYqL%+GKZ0_d&SnMnxEvx5D@DYFe-MgM4
vz?+dtgaLQP0{RRIAZIglo#+9A(AEUZsKf+MfHx}}NEZ_j_5kT{W)KemHHxXx

literal 0
HcmV?d00001