Refactor script starting logic

Author: adazem009

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;

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;
 }

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;

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);
+}

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));