Merge pull request #383 from scratchcpp/refactor_clones

Refactor clone API

Author: adazem009

include/scratchcpp/iengine.h

@@ -73,10 +73,10 @@ class LIBSCRATCHCPP_EXPORT IEngine
         virtual void stopTarget(Target *target, VirtualMachine *exceptScript) = 0;
 
         /*! Calls the "when I start as a clone" blocks of the given sprite. */
-        virtual void initClone(Sprite *clone) = 0;
+        virtual void initClone(std::shared_ptr<Sprite> clone) = 0;
 
         /*! Automatically called from clones that are being deleted. */
-        virtual void deinitClone(Sprite *clone) = 0;
+        virtual void deinitClone(std::shared_ptr<Sprite> clone) = 0;
 
         /*!
          * Calls and runs "when green flag clicked" blocks.

include/scratchcpp/sprite.h

@@ -13,7 +13,9 @@ class IGraphicsEffect;
 class SpritePrivate;
 
 /*! \brief The Sprite class represents a Scratch sprite. */
-class LIBSCRATCHCPP_EXPORT Sprite : public Target
+class LIBSCRATCHCPP_EXPORT Sprite
+    : public Target
+    , public std::enable_shared_from_this<Sprite>
 {
     public:
         enum class RotationStyle
@@ -25,18 +27,16 @@ class LIBSCRATCHCPP_EXPORT Sprite : public Target
 
         Sprite();
         Sprite(const Sprite &) = delete;
-        ~Sprite();
 
         void setInterface(ISpriteHandler *newInterface);
 
         std::shared_ptr<Sprite> clone();
+        void deleteClone();
 
         bool isClone() const;
 
-        Sprite *cloneRoot() const;
-        Sprite *cloneParent() const;
-        const std::vector<std::shared_ptr<Sprite>> &children() const;
-        std::vector<std::shared_ptr<Sprite>> allChildren() const;
+        Sprite *cloneSprite() const;
+        const std::vector<std::shared_ptr<Sprite>> &clones() const;
 
         bool visible() const;
         void setVisible(bool newVisible);

src/blocks/controlblocks.cpp

@@ -286,11 +286,11 @@ unsigned int ControlBlocks::createCloneOfMyself(VirtualMachine *vm)
 
 unsigned int ControlBlocks::deleteThisClone(VirtualMachine *vm)
 {
-    Target *target = vm->target();
+    Sprite *sprite = dynamic_cast<Sprite *>(vm->target());
 
-    if (target) {
-        vm->engine()->stopTarget(target, nullptr);
-        target->~Target();
+    if (sprite && sprite->isClone()) {
+        vm->engine()->stopTarget(sprite, nullptr);
+        sprite->deleteClone();
     }
 
     return 0;

src/engine/internal/engine.cpp

@@ -239,7 +239,8 @@ void Engine::broadcastByPtr(Broadcast *broadcast, VirtualMachine *sourceScript,
         if (!root->isStage()) {
             Sprite *sprite = dynamic_cast<Sprite *>(root);
             assert(sprite);
-            auto children = sprite->allChildren();
+            assert(!sprite->isClone());
+            const auto &children = sprite->clones();
 
             for (auto child : children)
                 targets.push_back(child.get());
@@ -278,17 +279,15 @@ void Engine::stopTarget(Target *target, VirtualMachine *exceptScript)
         stopScript(script);
 }
 
-void Engine::initClone(Sprite *clone)
+void Engine::initClone(std::shared_ptr<Sprite> clone)
 {
     if (!clone || ((m_cloneLimit >= 0) && (m_clones.size() >= m_cloneLimit)))
         return;
 
-    Sprite *source = clone->cloneParent();
-    Target *root = clone->cloneRoot();
-    assert(source);
+    Target *root = clone->cloneSprite();
     assert(root);
 
-    if (!source || !root)
+    if (!root)
         return;
 
     auto it = m_cloneInitScriptsMap.find(root);
@@ -300,26 +299,26 @@ void Engine::initClone(Sprite *clone)
         // Since we're initializing the clone, it shouldn't have any running scripts
         for (const auto &[target, targetScripts] : m_runningScripts) {
             for (const auto script : targetScripts)
-                assert((target != clone) || (std::find(m_scriptsToRemove.begin(), m_scriptsToRemove.end(), script.get()) != m_scriptsToRemove.end()));
+                assert((target != clone.get()) || (std::find(m_scriptsToRemove.begin(), m_scriptsToRemove.end(), script.get()) != m_scriptsToRemove.end()));
         }
 #endif
 
         for (auto script : scripts) {
-            auto vm = script->start(clone);
+            auto vm = script->start(clone.get());
             addRunningScript(vm);
         }
     }
 
     assert(std::find(m_clones.begin(), m_clones.end(), clone) == m_clones.end());
-    assert(std::find(m_executableTargets.begin(), m_executableTargets.end(), clone) == m_executableTargets.end());
-    m_clones.push_back(clone);
-    m_executableTargets.push_back(clone); // execution order needs to be updated after this
+    assert(std::find(m_executableTargets.begin(), m_executableTargets.end(), clone.get()) == m_executableTargets.end());
+    m_clones.insert(clone);
+    m_executableTargets.push_back(clone.get()); // execution order needs to be updated after this
 }
 
-void Engine::deinitClone(Sprite *clone)
+void Engine::deinitClone(std::shared_ptr<Sprite> clone)
 {
-    m_clones.erase(std::remove(m_clones.begin(), m_clones.end(), clone), m_clones.end());
-    m_executableTargets.erase(std::remove(m_executableTargets.begin(), m_executableTargets.end(), clone), m_executableTargets.end());
+    m_clones.erase(clone);
+    m_executableTargets.erase(std::remove(m_executableTargets.begin(), m_executableTargets.end(), clone.get()), m_executableTargets.end());
 }
 
 void Engine::run()
@@ -1204,11 +1203,11 @@ void Engine::deleteClones()
         Sprite *sprite = dynamic_cast<Sprite *>(target.get());
 
         if (sprite) {
-            std::vector<std::shared_ptr<Sprite>> clones = sprite->children();
+            std::vector<std::shared_ptr<Sprite>> clones = sprite->clones();
 
             for (auto clone : clones) {
                 assert(clone);
-                clone->~Sprite();
+                clone->deleteClone();
             }
         }
     }
@@ -1219,8 +1218,8 @@ void Engine::deleteClones()
 void Engine::removeExecutableClones()
 {
     // Remove clones from the executable targets
-    for (Target *target : m_clones)
-        m_executableTargets.erase(std::remove(m_executableTargets.begin(), m_executableTargets.end(), target), m_executableTargets.end());
+    for (std::shared_ptr<Sprite> clone : m_clones)
+        m_executableTargets.erase(std::remove(m_executableTargets.begin(), m_executableTargets.end(), clone.get()), m_executableTargets.end());
 }
 
 void Engine::updateFrameDuration()

src/engine/internal/engine.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <chrono>
 #include <mutex>
+#include <set>
 
 #include "blocksectioncontainer.h"
 
@@ -36,8 +37,8 @@ class Engine : public IEngine
         void broadcastByPtr(Broadcast *broadcast, VirtualMachine *sourceScript, bool wait = false) override;
         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 initClone(std::shared_ptr<Sprite> clone) override;
+        void deinitClone(std::shared_ptr<Sprite> clone) override;
 
         void run() override;
         void runEventLoop() override;
@@ -183,7 +184,7 @@ class Engine : public IEngine
         unsigned int m_stageWidth = 480;
         unsigned int m_stageHeight = 360;
         int m_cloneLimit = 300;
-        std::vector<Sprite *> m_clones;
+        std::set<std::shared_ptr<Sprite>> m_clones;
         bool m_spriteFencingEnabled = true;
 
         bool m_running = false;

src/engine/script.cpp

@@ -63,7 +63,7 @@ std::shared_ptr<VirtualMachine> Script::start(Target *target)
         sprite = dynamic_cast<Sprite *>(target);
 
     if (impl->target && sprite && sprite->isClone() && impl->engine) {
-        Target *root = sprite->cloneRoot();
+        Target *root = sprite->cloneSprite();
 
         if (root != impl->target) {
             std::cout << "warning: a clone tried to start a script of another target (this is a bug in libscratchcpp or in your code!)" << std::endl;

src/scratch/sprite.cpp

@@ -8,6 +8,7 @@
 #include <scratchcpp/costume.h>
 #include <scratchcpp/rect.h>
 #include <cassert>
+#include <iostream>
 
 #include "sprite_p.h"
 
@@ -20,25 +21,6 @@ Sprite::Sprite() :
 {
 }
 
-/*! Destroys the Sprite object. */
-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);
-    }
-}
-
 /*! Sets the sprite interface. */
 void Sprite::setInterface(ISpriteHandler *newInterface)
 {
@@ -55,13 +37,13 @@ std::shared_ptr<Sprite> Sprite::clone()
     if (eng && (eng->cloneLimit() == -1 || eng->cloneCount() < eng->cloneLimit())) {
         std::shared_ptr<Sprite> clone = std::make_shared<Sprite>();
 
-        if (impl->cloneRoot == nullptr)
-            clone->impl->cloneRoot = this;
-        else
-            clone->impl->cloneRoot = impl->cloneRoot;
-
-        clone->impl->cloneParent = this;
-        impl->childClones.push_back(clone);
+        if (impl->cloneSprite == nullptr) {
+            clone->impl->cloneSprite = this;
+            impl->clones.push_back(clone);
+        } else {
+            clone->impl->cloneSprite = impl->cloneSprite;
+            impl->cloneSprite->impl->clones.push_back(clone);
+        }
 
         // Copy data
         clone->setName(name());
@@ -91,7 +73,7 @@ std::shared_ptr<Sprite> Sprite::clone()
         clone->setEngine(engine());
 
         // Call "when I start as clone" scripts
-        eng->initClone(clone.get());
+        eng->initClone(clone);
 
         if (impl->visible)
             eng->requestRedraw();
@@ -108,42 +90,43 @@ std::shared_ptr<Sprite> Sprite::clone()
     return nullptr;
 }
 
-/*! Returns true if this is a clone. */
-bool Sprite::isClone() const
+/*! Deletes this clone (if the sprite is a clone). */
+void Sprite::deleteClone()
 {
-    return (impl->cloneParent != nullptr);
-}
+    assert(isClone());
 
-/*! Returns the sprite this clone was created from, or nullptr if this isn't a clone. */
-Sprite *Sprite::cloneRoot() const
-{
-    return impl->cloneRoot;
+    if (isClone()) {
+        IEngine *eng = engine();
+
+        if (eng)
+            eng->deinitClone(shared_from_this());
+
+        assert(impl->cloneSprite);
+        impl->cloneDeleted = true;
+        impl->cloneSprite->impl->removeClone(this);
+    }
 }
 
-/*! Returns the sprite or clone this clone was created from, or nullptr if this isn't a clone. */
-Sprite *Sprite::cloneParent() const
+/*! Returns true if this is a clone. */
+bool Sprite::isClone() const
 {
-    return impl->cloneParent;
+    return (impl->cloneSprite != nullptr);
 }
 
-/*! Returns list of child clones. */
-const std::vector<std::shared_ptr<Sprite>> &Sprite::children() const
+/*! Returns the sprite this clone was created from, or nullptr if this isn't a clone. */
+Sprite *Sprite::cloneSprite() const
 {
-    return impl->childClones;
+    return impl->cloneSprite;
 }
 
-/*! Returns list of child clones and their children (recursive). */
-std::vector<std::shared_ptr<Sprite>> Sprite::allChildren() const
+/*! Returns list of clones of the sprite. */
+const std::vector<std::shared_ptr<Sprite>> &Sprite::clones() const
 {
-    std::vector<std::shared_ptr<Sprite>> ret;
-
-    for (auto clone : impl->childClones) {
-        ret.push_back(clone);
-        auto children = clone->allChildren();
-        ret.insert(ret.end(), children.begin(), children.end());
-    }
-
-    return ret;
+    if (isClone()) {
+        assert(impl->cloneSprite);
+        return impl->cloneSprite->impl->clones;
+    } else
+        return impl->clones;
 }
 
 /*! Returns true if the sprite is visible. */
@@ -419,7 +402,7 @@ void Sprite::clearGraphicsEffects()
 
 Target *Sprite::dataSource() const
 {
-    return impl->cloneRoot;
+    return impl->cloneSprite;
 }
 
 void Sprite::setXY(double x, double y)

src/scratch/sprite_p.cpp

@@ -21,9 +21,9 @@ SpritePrivate::SpritePrivate(Sprite *sprite) :
 void SpritePrivate::removeClone(Sprite *clone)
 {
     int index = 0;
-    for (const auto &child : childClones) {
+    for (const auto &child : clones) {
         if (child.get() == clone) {
-            childClones.erase(childClones.begin() + index);
+            clones.erase(clones.begin() + index);
             return;
         }
 

src/scratch/sprite_p.h

@@ -21,9 +21,9 @@ struct SpritePrivate
 
         Sprite *sprite = nullptr;
         ISpriteHandler *iface = nullptr;
-        Sprite *cloneRoot = nullptr;
-        Sprite *cloneParent = nullptr;
-        std::vector<std::shared_ptr<Sprite>> childClones;
+        Sprite *cloneSprite = nullptr;
+        std::vector<std::shared_ptr<Sprite>> clones;
+        bool cloneDeleted = false;
         bool visible = true;
         double x = 0;
         double y = 0;