From 1d455ea45707464579e51ccb246df6fcfa639558 Mon Sep 17 00:00:00 2001 From: adazem009 <68537469+adazem009@users.noreply.github.com> Date: Mon, 30 Oct 2023 12:11:10 +0100 Subject: [PATCH 1/3] Automatically delete clones when the project stops --- include/scratchcpp/iengine.h | 3 ++ src/engine/internal/engine.cpp | 51 +++++++++++++++++++++++++++++++--- src/engine/internal/engine.h | 5 ++++ src/scratch/sprite.cpp | 10 +++++++ test/mocks/enginemock.h | 1 + 5 files changed, 66 insertions(+), 4 deletions(-) diff --git a/include/scratchcpp/iengine.h b/include/scratchcpp/iengine.h index 262ebaa8..24d50beb 100644 --- a/include/scratchcpp/iengine.h +++ b/include/scratchcpp/iengine.h @@ -81,6 +81,9 @@ class LIBSCRATCHCPP_EXPORT IEngine /*! Calls the "when I start as a clone" blocks of the given sprite. */ virtual void initClone(Sprite *clone) = 0; + /*! Automatically called from clones that are being deleted. */ + virtual void deinitClone(Sprite *clone) = 0; + /*! * Runs the event loop and calls "when green flag clicked" blocks. * \note This function returns when all scripts finish.\n diff --git a/src/engine/internal/engine.cpp b/src/engine/internal/engine.cpp index 372f9f6b..fac33a58 100644 --- a/src/engine/internal/engine.cpp +++ b/src/engine/internal/engine.cpp @@ -32,11 +32,17 @@ Engine::Engine() : { } +Engine::~Engine() +{ + m_clones.clear(); +} + void Engine::clear() { m_sections.clear(); m_targets.clear(); m_broadcasts.clear(); + m_clones.clear(); } // Resolves ID references and sets pointers of entities. @@ -172,6 +178,11 @@ void Engine::frame() void Engine::start() { + if (m_running) + finalize(); + + deleteClones(); + m_timer->reset(); m_running = true; @@ -184,9 +195,8 @@ void Engine::start() void Engine::stop() { - m_runningScripts.clear(); - m_scriptsToRemove.clear(); - m_running = false; + finalize(); + deleteClones(); } void Engine::startScript(std::shared_ptr topLevelBlock, std::shared_ptr target) @@ -328,6 +338,14 @@ void Engine::initClone(Sprite *clone) m_runningScripts.push_back(vm); } } + + assert(std::find(m_clones.begin(), m_clones.end(), clone) == m_clones.end()); + m_clones.push_back(clone); +} + +void Engine::deinitClone(Sprite *clone) +{ + m_clones.erase(std::remove(m_clones.begin(), m_clones.end(), clone), m_clones.end()); } void Engine::run() @@ -360,7 +378,7 @@ void Engine::run() lastFrameTime = currentTime; } - stop(); + finalize(); } bool Engine::isRunning() const @@ -879,6 +897,31 @@ BlockSectionContainer *Engine::blockSectionContainer(IBlockSection *section) con return nullptr; } +void Engine::finalize() +{ + m_runningScripts.clear(); + m_scriptsToRemove.clear(); + m_running = false; +} + +void Engine::deleteClones() +{ + m_clones.clear(); + + for (auto target : m_targets) { + Sprite *sprite = dynamic_cast(target.get()); + + if (sprite) { + std::vector> clones = sprite->children(); + + for (auto clone : clones) { + assert(clone); + clone->~Sprite(); + } + } + } +} + void Engine::updateFrameDuration() { m_frameDuration = std::chrono::milliseconds(static_cast(1000 / m_fps)); diff --git a/src/engine/internal/engine.h b/src/engine/internal/engine.h index e60cba19..f4631763 100644 --- a/src/engine/internal/engine.h +++ b/src/engine/internal/engine.h @@ -21,6 +21,7 @@ class Engine : public IEngine public: Engine(); Engine(const Engine &) = delete; + ~Engine(); void clear() override; void resolveIds(); @@ -35,6 +36,7 @@ class Engine : public IEngine void stopScript(VirtualMachine *vm) override; void stopTarget(Target *target, VirtualMachine *exceptScript) override; void initClone(libscratchcpp::Sprite *clone) override; + void deinitClone(libscratchcpp::Sprite *clone) override; void run() override; bool isRunning() const override; @@ -109,6 +111,8 @@ class Engine : public IEngine BlockSectionContainer *blockSectionContainer(IBlockSection *section) const; private: + void finalize(); + void deleteClones(); std::shared_ptr getBlock(const std::string &id); std::shared_ptr getVariable(const std::string &id); std::shared_ptr getList(const std::string &id); @@ -142,6 +146,7 @@ class Engine : public IEngine bool m_mousePressed = false; unsigned int m_stageWidth = 480; unsigned int m_stageHeight = 360; + std::vector m_clones; bool m_running = false; bool m_breakFrame = false; diff --git a/src/scratch/sprite.cpp b/src/scratch/sprite.cpp index 3635f006..cb5351d7 100644 --- a/src/scratch/sprite.cpp +++ b/src/scratch/sprite.cpp @@ -23,6 +23,16 @@ Sprite::Sprite() : Sprite::~Sprite() { if (isClone()) { + IEngine *eng = engine(); + + if (eng) { + eng->deinitClone(this); + + auto children = allChildren(); + for (auto child : children) + eng->deinitClone(child.get()); + } + assert(impl->cloneParent); impl->cloneParent->impl->removeClone(this); } diff --git a/test/mocks/enginemock.h b/test/mocks/enginemock.h index d658dbf2..593efd56 100644 --- a/test/mocks/enginemock.h +++ b/test/mocks/enginemock.h @@ -23,6 +23,7 @@ class EngineMock : public IEngine MOCK_METHOD(void, stopScript, (VirtualMachine *), (override)); MOCK_METHOD(void, stopTarget, (Target *, VirtualMachine *), (override)); MOCK_METHOD(void, initClone, (Sprite *), (override)); + MOCK_METHOD(void, deinitClone, (Sprite *), (override)); MOCK_METHOD(void, run, (), (override)); MOCK_METHOD(bool, isRunning, (), (const, override)); From 55f6e95d3b4430517af96967a5a5aa359b342a5a Mon Sep 17 00:00:00 2001 From: adazem009 <68537469+adazem009@users.noreply.github.com> Date: Mon, 30 Oct 2023 12:13:30 +0100 Subject: [PATCH 2/3] Add clone limit --- include/scratchcpp/iengine.h | 6 ++++ src/engine/internal/engine.cpp | 12 +++++++- src/engine/internal/engine.h | 4 +++ test/clone_limit.sb3 | Bin 0 -> 2364 bytes test/engine/engine_test.cpp | 52 +++++++++++++++++++++++++++++++++ test/mocks/enginemock.h | 3 ++ 6 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 test/clone_limit.sb3 diff --git a/include/scratchcpp/iengine.h b/include/scratchcpp/iengine.h index 24d50beb..9aa60772 100644 --- a/include/scratchcpp/iengine.h +++ b/include/scratchcpp/iengine.h @@ -140,6 +140,12 @@ class LIBSCRATCHCPP_EXPORT IEngine /*! Sets the stage height. */ virtual void setStageHeight(unsigned int height) = 0; + /*! Returns the maximum number of clones (or -1 if the limit is disabled). */ + virtual int cloneLimit() const = 0; + + /*! Sets the maximum number of clones (use -1 or any negative number to disable the limit). */ + virtual void setCloneLimit(int limit) = 0; + /*! Returns true if there are any running script of the broadcast with the given index. */ virtual bool broadcastRunning(unsigned int index, VirtualMachine *sourceScript) = 0; diff --git a/src/engine/internal/engine.cpp b/src/engine/internal/engine.cpp index fac33a58..1235d235 100644 --- a/src/engine/internal/engine.cpp +++ b/src/engine/internal/engine.cpp @@ -311,7 +311,7 @@ void Engine::stopTarget(Target *target, VirtualMachine *exceptScript) void Engine::initClone(Sprite *clone) { - if (!clone) + if (!clone || ((m_cloneLimit >= 0) && (m_clones.size() >= m_cloneLimit))) return; Sprite *source = clone->cloneParent(); @@ -481,6 +481,16 @@ void Engine::setStageHeight(unsigned int height) m_stageHeight = height; } +int Engine::cloneLimit() const +{ + return m_cloneLimit; +} + +void Engine::setCloneLimit(int limit) +{ + m_cloneLimit = limit < 0 ? -1 : limit; +} + bool Engine::broadcastRunning(unsigned int index, VirtualMachine *sourceScript) { if (index < 0 || index >= m_broadcasts.size()) diff --git a/src/engine/internal/engine.h b/src/engine/internal/engine.h index f4631763..9a14d8a7 100644 --- a/src/engine/internal/engine.h +++ b/src/engine/internal/engine.h @@ -63,6 +63,9 @@ class Engine : public IEngine unsigned int stageHeight() const override; void setStageHeight(unsigned int height) override; + int cloneLimit() const override; + void setCloneLimit(int limit) override; + bool broadcastRunning(unsigned int index, VirtualMachine *sourceScript) override; bool broadcastByPtrRunning(Broadcast *broadcast, VirtualMachine *sourceScript) override; @@ -146,6 +149,7 @@ class Engine : public IEngine bool m_mousePressed = false; unsigned int m_stageWidth = 480; unsigned int m_stageHeight = 360; + int m_cloneLimit = 300; std::vector m_clones; bool m_running = false; diff --git a/test/clone_limit.sb3 b/test/clone_limit.sb3 new file mode 100644 index 0000000000000000000000000000000000000000..75338c6e3776b4cf79f0b86ca095a73a4b596252 GIT binary patch literal 2364 zcma)8do&Xa8y^w&(#0hTVKR|i#zb=2D7R+BhGj!zLo#H%5ouX^-B~0fg~Ct^xqdbG zOU$8RuM* zS`tJT0syF*0D$QJH87au=Zz21^t(w4;9+)dW?9MXI*%eqFBJ4I=1bG*cKCk17ndAO zKe|DlU_DVFrT{v>b9Q$vLfXC&z^8X3U@j`i1IxL^%3Y@#wJ>!Q*%lFx!mRY^qKAb? z@okv}DfdwZm)0;q1XJnD6S&6@`9FhDCwauZ46W)zmbvcpg(M(x?8m@J{3f_YLHwHw z{vTebZ~bW;&+6R_<7 zpp@lc4q-c%3rera>xl6wjnF-Xac?}}klLC`y`=1T@J=$vzWv|=EMnBf;fmGZuVuMw z?%=o?Il8$r$N&tZQ$@*hL zUFc^U1GtY4{()(%oZ_P~*1tR$xf!xljM@#;y=e2`49MCQ9g~6scdTZ@MP(@-)-Yyh zLs7JHI0(~{VJSJUpFDS9vLgXR?sMnzZMBNWs-(3vaZ`0cu)FW7*$=E|BS$Ndx4+=@ z6c?A=HR;cHtuRF6?<|KC`#mdr|3L zeBLwMjPUMsRr;|O-vILDT#qEt;=wB3o)qIC+pf{y4|7TnIbYMmaO zG;drUHukO-cANkih>NDl%=>83q|nZL77^e66?hot1!o3%yEQGxsAt&w@@d*HCCHG? zLHAWulo@ulla2bWpi05FuYE@@352n`9?_R1&qy!XGy%fm^~vxSPpRwoUPIwu2ycu*G@LHiU`?{B=d}3Qk*V?Q% zCbcv7o}E~k58+EXA&5U4t>vI5%rS^jH$EoDcUkQb;P;84&vvm*_gDM|-BKxnZ(<#u zbWP86))^rW!B@OC6mM7j5&V@D$}xfgWj8jT>*D3=+mgCZt>{J7WVgvTP-AQ?NaKOp z@~Rr9bKtR2RZVB>&BBT{S0^MveNeVVAh=ybvA3SsV7(*b8)A|*=l|(pS#@zdzlzJM zNEBi-G%EF!xzd~GyB$ce3o^r6p7PWlnBGXjD^TfaEoP(6Q%gfXLVEt(m~`%yt%||I zZ-)$POiZ)5)%7BV+-es&@`{rjSzL8pf4p{m>?fzF%fSEDs)hfnbZtlIIkSh6k&ra* z=&~VWJKKOEN6W7|V5pwbc!sDxr^H}JRb*$hs^iWD@qcH_yR7IcL5bfTn&{-yQz(D< z>_FC|uXo&s2aRfTz?Ho9ntrZ2u&LfAr=i}Xslj{z$I@YW|w&SVw zv>^o6sPW9}x@m(XiQpg1ji9dbVqU96X~K<)$qsQ@jHNl~@O}1q0qBgz*_o3*9Zp`o z6MvMBjDI$1cCv?N?FH$#ax5VnWzDN2q<#0^yowY=18 z%CkoHN>tj!IJGvE^o|W-9w7)ik4dsad}xWhpB3;<6?m|*D^-dD4PaJ&8h9MC?5ad# zKBFK$d@``4X88EVSg9?C%$bPhseicQ-)vdXnNQE%qm;kA{<^;Cx9GJN8MmL)2Ogh1 zNva`F+2sfxv^=*fU}8A^;*N;NWoh2J?Hz!cloneLimit(), 300); + + // TODO: Set "infinite" FPS and remove this (#254) + engine->setFps(100000); + + Stage *stage = engine->stage(); + ASSERT_TRUE(stage); + + p.run(); + ASSERT_VAR(stage, "count"); + ASSERT_EQ(GET_VAR(stage, "count")->value().toInt(), 300); + ASSERT_VAR(stage, "delete_passed"); + ASSERT_TRUE(GET_VAR(stage, "delete_passed")->value().toBool()); + + engine->setCloneLimit(475); + ASSERT_EQ(engine->cloneLimit(), 475); + p.run(); + ASSERT_VAR(stage, "count"); + ASSERT_EQ(GET_VAR(stage, "count")->value().toInt(), 475); + ASSERT_VAR(stage, "delete_passed"); + ASSERT_TRUE(GET_VAR(stage, "delete_passed")->value().toBool()); + + engine->setCloneLimit(0); + ASSERT_EQ(engine->cloneLimit(), 0); + p.run(); + ASSERT_VAR(stage, "count"); + ASSERT_EQ(GET_VAR(stage, "count")->value().toInt(), 0); + ASSERT_VAR(stage, "delete_passed"); + ASSERT_TRUE(GET_VAR(stage, "delete_passed")->value().toBool()); + + engine->setCloneLimit(-1); + ASSERT_EQ(engine->cloneLimit(), -1); + p.run(); + ASSERT_VAR(stage, "count"); + ASSERT_GT(GET_VAR(stage, "count")->value().toInt(), 500); + ASSERT_VAR(stage, "delete_passed"); + ASSERT_TRUE(GET_VAR(stage, "delete_passed")->value().toBool()); + + engine->setCloneLimit(-5); + ASSERT_EQ(engine->cloneLimit(), -1); + p.run(); + ASSERT_VAR(stage, "count"); + ASSERT_GT(GET_VAR(stage, "count")->value().toInt(), 500); + ASSERT_VAR(stage, "delete_passed"); + ASSERT_TRUE(GET_VAR(stage, "delete_passed")->value().toBool()); +} + // TODO: Uncomment this after fixing #256 and #257 /*TEST(EngineTest, BackdropBroadcasts) { diff --git a/test/mocks/enginemock.h b/test/mocks/enginemock.h index 593efd56..713c0017 100644 --- a/test/mocks/enginemock.h +++ b/test/mocks/enginemock.h @@ -50,6 +50,9 @@ class EngineMock : public IEngine MOCK_METHOD(unsigned int, stageHeight, (), (const, override)); MOCK_METHOD(void, setStageHeight, (unsigned int), (override)); + MOCK_METHOD(int, cloneLimit, (), (const, override)); + MOCK_METHOD(void, setCloneLimit, (int), (override)); + MOCK_METHOD(bool, broadcastRunning, (unsigned int, VirtualMachine *), (override)); MOCK_METHOD(bool, broadcastByPtrRunning, (Broadcast *, VirtualMachine *), (override)); From f40cb9d34d78cb184e64799650a38a09281d7192 Mon Sep 17 00:00:00 2001 From: adazem009 <68537469+adazem009@users.noreply.github.com> Date: Mon, 30 Oct 2023 12:56:01 +0100 Subject: [PATCH 3/3] Fix sprite deletion in clone tests --- test/scratch_classes/sprite_test.cpp | 102 ++++++++++-------- .../target_interfaces/ispritehandler_test.cpp | 4 + 2 files changed, 62 insertions(+), 44 deletions(-) diff --git a/test/scratch_classes/sprite_test.cpp b/test/scratch_classes/sprite_test.cpp index 05ee32ff..d8186a0f 100644 --- a/test/scratch_classes/sprite_test.cpp +++ b/test/scratch_classes/sprite_test.cpp @@ -27,36 +27,36 @@ TEST(SpriteTest, Visible) TEST(SpriteTest, Clone) { - Sprite sprite; - sprite.setName("Sprite1"); + std::unique_ptr sprite = std::make_unique(); + sprite->setName("Sprite1"); auto var1 = std::make_shared("a", "var1", "hello"); auto var2 = std::make_shared("b", "var2", "world"); - sprite.addVariable(var1); - sprite.addVariable(var2); + sprite->addVariable(var1); + sprite->addVariable(var2); auto c1 = std::make_shared("costume1", "", "svg"); - sprite.addCostume(c1); + sprite->addCostume(c1); auto list1 = std::make_shared("c", "list1"); list1->push_back("item1"); list1->push_back("item2"); auto list2 = std::make_shared("d", "list2"); list2->push_back("test"); - sprite.addList(list1); - sprite.addList(list2); - - sprite.addCostume(std::make_shared("", "", "")); - sprite.addCostume(std::make_shared("", "", "")); - sprite.setCurrentCostume(2); - sprite.setLayerOrder(5); - sprite.setVolume(50); - - sprite.setVisible(false); - sprite.setX(100.25); - sprite.setY(-45.43); - sprite.setSize(54.121); - sprite.setDirection(179.4); - sprite.setDraggable(true); - sprite.setRotationStyle(Sprite::RotationStyle::DoNotRotate); + sprite->addList(list1); + sprite->addList(list2); + + sprite->addCostume(std::make_shared("", "", "")); + sprite->addCostume(std::make_shared("", "", "")); + sprite->setCurrentCostume(2); + sprite->setLayerOrder(5); + sprite->setVolume(50); + + sprite->setVisible(false); + sprite->setX(100.25); + sprite->setY(-45.43); + sprite->setSize(54.121); + sprite->setDirection(179.4); + sprite->setDraggable(true); + sprite->setRotationStyle(Sprite::RotationStyle::DoNotRotate); auto checkCloneData = [](Sprite *clone) { ASSERT_TRUE(clone); @@ -97,42 +97,42 @@ TEST(SpriteTest, Clone) ASSERT_EQ(clone->rotationStyle(), Sprite::RotationStyle::DoNotRotate); }; - ASSERT_FALSE(sprite.isClone()); - ASSERT_EQ(sprite.cloneRoot(), nullptr); - ASSERT_EQ(sprite.cloneParent(), nullptr); + ASSERT_FALSE(sprite->isClone()); + ASSERT_EQ(sprite->cloneRoot(), nullptr); + ASSERT_EQ(sprite->cloneParent(), nullptr); - ASSERT_EQ(sprite.clone(), nullptr); - ASSERT_FALSE(sprite.isClone()); - ASSERT_EQ(sprite.cloneRoot(), nullptr); - ASSERT_EQ(sprite.cloneParent(), nullptr); + ASSERT_EQ(sprite->clone(), nullptr); + ASSERT_FALSE(sprite->isClone()); + ASSERT_EQ(sprite->cloneRoot(), nullptr); + ASSERT_EQ(sprite->cloneParent(), nullptr); EngineMock engine; - sprite.setEngine(&engine); + sprite->setEngine(&engine); Sprite *clone1; EXPECT_CALL(engine, initClone(_)).WillOnce(SaveArg<0>(&clone1)); - ASSERT_EQ(sprite.clone().get(), clone1); - ASSERT_FALSE(sprite.isClone()); - ASSERT_EQ(sprite.cloneRoot(), nullptr); - ASSERT_EQ(sprite.cloneParent(), nullptr); + ASSERT_EQ(sprite->clone().get(), clone1); + ASSERT_FALSE(sprite->isClone()); + ASSERT_EQ(sprite->cloneRoot(), nullptr); + ASSERT_EQ(sprite->cloneParent(), nullptr); ASSERT_TRUE(clone1->isClone()); - ASSERT_EQ(clone1->cloneRoot(), &sprite); - ASSERT_EQ(clone1->cloneParent(), &sprite); + ASSERT_EQ(clone1->cloneRoot(), sprite.get()); + ASSERT_EQ(clone1->cloneParent(), sprite.get()); checkCloneData(clone1); // Modify root sprite data to make sure parent is used - sprite.setLayerOrder(3); + sprite->setLayerOrder(3); Sprite *clone2; EXPECT_CALL(engine, initClone(_)).WillOnce(SaveArg<0>(&clone2)); ASSERT_EQ(clone1->clone().get(), clone2); ASSERT_TRUE(clone1->isClone()); - ASSERT_EQ(clone1->cloneRoot(), &sprite); - ASSERT_EQ(clone1->cloneParent(), &sprite); + ASSERT_EQ(clone1->cloneRoot(), sprite.get()); + ASSERT_EQ(clone1->cloneParent(), sprite.get()); ASSERT_TRUE(clone2->isClone()); - ASSERT_EQ(clone2->cloneRoot(), &sprite); + ASSERT_EQ(clone2->cloneRoot(), sprite.get()); ASSERT_EQ(clone2->cloneParent(), clone1); checkCloneData(clone2); @@ -143,10 +143,10 @@ TEST(SpriteTest, Clone) Sprite *clone4; EXPECT_CALL(engine, initClone(_)).WillOnce(SaveArg<0>(&clone4)); - ASSERT_EQ(sprite.clone().get(), clone4); + ASSERT_EQ(sprite->clone().get(), clone4); // children - const auto &children1 = sprite.children(); + const auto &children1 = sprite->children(); ASSERT_EQ(children1.size(), 2); ASSERT_EQ(children1[0].get(), clone1); ASSERT_EQ(children1[1].get(), clone4); @@ -163,7 +163,7 @@ TEST(SpriteTest, Clone) ASSERT_TRUE(clone4->children().empty()); // allChildren - auto allChildren = sprite.allChildren(); + auto allChildren = sprite->allChildren(); ASSERT_EQ(allChildren.size(), 4); ASSERT_EQ(allChildren[0].get(), clone1); ASSERT_EQ(allChildren[1].get(), clone2); @@ -181,10 +181,24 @@ TEST(SpriteTest, Clone) ASSERT_TRUE(clone4->allChildren().empty()); - ASSERT_EQ(clone2->costumes(), sprite.costumes()); + ASSERT_EQ(clone2->costumes(), sprite->costumes()); auto c2 = std::make_shared("costume2", "", "png"); clone2->addCostume(c2); - ASSERT_EQ(clone2->costumes(), sprite.costumes()); + ASSERT_EQ(clone2->costumes(), sprite->costumes()); + + EXPECT_CALL(engine, deinitClone(clone2)); + clone2->~Sprite(); + + EXPECT_CALL(engine, deinitClone(clone3)); + clone3->~Sprite(); + + EXPECT_CALL(engine, deinitClone(clone1)).Times(2); + clone1->~Sprite(); + + EXPECT_CALL(engine, deinitClone(clone4)).Times(2); + clone4->~Sprite(); + + sprite.reset(); } TEST(SpriteTest, X) diff --git a/test/target_interfaces/ispritehandler_test.cpp b/test/target_interfaces/ispritehandler_test.cpp index 75b69fc7..90feabf7 100644 --- a/test/target_interfaces/ispritehandler_test.cpp +++ b/test/target_interfaces/ispritehandler_test.cpp @@ -34,6 +34,10 @@ TEST_F(ISpriteHandlerTest, Clone) m_sprite.clone(); ASSERT_TRUE(clone); ASSERT_EQ(cloneArg, clone); + + // Engine is used during deletion, so let's just set it to nullptr (this is tested elsewhere) + m_sprite.setEngine(nullptr); + clone->setEngine(nullptr); } TEST_F(ISpriteHandlerTest, Visible)