Skip to content

WIP: Refactoring scene tree to make unordered set of unique pointers #605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aaryaag03 wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

WIP: Refactoring scene tree to make unordered set of unique pointers #605

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c2fe22
three nodes
Jan 24, 2023
f43806f
store children as unordered set
Jan 25, 2023
7a65531
unordered set bug fixes
Jan 31, 2023
1124f54
lint fix
Jan 31, 2023
a8af29c
revert commits to unstage json and imgui
Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions editor/gui/scene_dock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include "vendor/ImGUI/imgui_impl_dx11.h"
#include "vendor/ImGUI/imgui_impl_win32.h"

void SceneDock::showSceneTree(const Ptr<Scene>& scene)
void SceneDock::showSceneTree(Ptr<Scene>& scene)
{
ZoneScoped;

Expand Down Expand Up @@ -78,16 +78,17 @@ void SceneDock::showSceneTree(const Ptr<Scene>& scene)
Scene* rearrangeScenePtr = rearrangeScene.get();
Scene* scenePtr = scene.get();

EventManager::GetSingleton()->defer([=]()
{ scenePtr->snatchChild(rearrangeScenePtr); });
EventManager::GetSingleton()->defer([=]() {
scenePtr->snatchChild(rearrangeScenePtr);
});

openScene(scenePtr);
}
}
ImGui::EndDragDropTarget();
}

for (auto&& child : scene->getChildren())
for (auto& child : scene->getChildren())
{
showSceneTree(child);
}
Expand Down
2 changes: 1 addition & 1 deletion editor/gui/scene_dock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class SceneDock
SceneDockSettings m_SceneDockSettings;
SceneID m_OpenedSceneID;

void showSceneTree(const Ptr<Scene>& scene);
void showSceneTree(Ptr<Scene>& scene);
void openScene(Scene* scene);

Variant selectOpenScene(const Event* event);
Expand Down
26 changes: 12 additions & 14 deletions rootex/framework/scene.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,28 +249,27 @@ bool Scene::snatchChild(Scene* child)
}

Unordered_set<Ptr<Scene>>& children = child->getParent()->getChildren();
for (auto&& child_scene : children)
for (auto& child_scene : children)
Copy link
Copy Markdown
Contributor

@Aviii06 Aviii06 Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to use an lvalue reference instead of rvalue?

Copy link
Copy Markdown
Member

@twaritwaikar twaritwaikar Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the universal binder, not an rvalue reference. However, this line diff doesn't seem to change anything.

{
if (child_scene.get() == child)
{
auto nh = children.extract(child_scene);
m_ChildrenScenes.insert(std::move(nh));
child->m_ParentScene = this;
return true;
m_ChildrenScenes.insert(std::move(child_scene));
children.erase(child_scene);
}
}

/*
for (int i = 0; i < children.size(); i++)
{
if (children.at(i).get() == child)
{
m_ChildrenScenes.insert(std::move(children[i]));
children.erase(children.begin() + i);
}
if (children.at(i).get() == child)
{
m_ChildrenScenes.insert(std::move(children[i]));
children.erase(children.begin() + i);
}
}
*/
return false;
child->m_ParentScene = this;
return true;
}

bool Scene::checkCycle(Scene* child)
Expand Down Expand Up @@ -307,9 +306,8 @@ bool Scene::addChild(Ptr<Scene>& child)
}
m_ChildrenScenes.insert(std::move(child));

auto it = m_ChildrenScenes.end();
--it;
ScriptSystem::GetSingleton()->addEnterScriptEntity(&(*it)->getEntity());
//aarya
Copy link
Copy Markdown
Contributor

@Aviii06 Aviii06 Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

//ScriptSystem::GetSingleton()->addEnterScriptEntity(&m_ChildrenScenes.back()->getEntity());
}
else
{
Expand Down
196 changes: 98 additions & 98 deletions rootex/framework/scene.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,98 +1,98 @@
#pragma once
#include "common/common.h"
#include "entity.h"
#include "core/input/input_manager.h"
class Entity;
#define ROOT_SCENE_ID 1
struct SceneSettings
{
ResourceCollection preloads;
SceneID camera = ROOT_SCENE_ID;
SceneID listener = ROOT_SCENE_ID;
HashMap<String, InputScheme> inputSchemes;
String startScheme = {};
void drawCameraSceneSelectables(Scene* scene, SceneID& toSet);
void drawListenerSceneSelectables(Scene* scene, SceneID& toSet);
void drawInputScheme(InputDescription& floatInput);
void draw();
};
void to_json(JSON::json& j, const SceneSettings& s);
void from_json(const JSON::json& j, SceneSettings& s);
class Scene
{
public:
enum class ImportStyle
{
/// If scene is not imported but created raw inside this scene
Local,
/// If scene is linked to another scene file
External
};
private:
bool m_IsScenePaused;
static Vector<Scene*> s_Scenes;
SceneID m_ID;
String m_Name;
String m_FullName;
ImportStyle m_ImportStyle;
/// Contains the current file name if local, else contains the linked scene file
String m_SceneFile;
SceneSettings m_Settings;
Entity m_Entity;
Scene* m_ParentScene = nullptr;
Unordered_set<Ptr<Scene>> m_ChildrenScenes;
bool checkCycle(Scene* child);
public:
static void ResetNextID();
static Ptr<Scene> Create(const JSON::json& sceneData, const bool assignNewIDs);
static Ptr<Scene> CreateFromFile(const String& sceneFile);
static Ptr<Scene> CreateEmpty();
static Ptr<Scene> CreateEmptyAtPath(const String& sceneFile);
static Ptr<Scene> CreateRootScene();
static bool isReservedName(const String& sceneName);
static Vector<Scene*> FindScenesByName(const String& name);
static Scene* FindSceneByID(const SceneID& id);
static const Vector<Scene*>& FindAllScenes();
Scene(SceneID id, const String& name, const SceneSettings& settings, ImportStyle importStyle, const String& sceneFile);
~Scene();
Scene* findScene(SceneID scene);
void reimport();
void onLoad();
bool snatchChild(Scene* child);
bool addChild(Ptr<Scene>& child);
bool removeChild(Scene* toRemove);
void setName(const String& name);
JSON::json getJSON() const;
bool& getIsScenePaused() { return m_IsScenePaused; }
void setIsScenePaused(bool pause) { m_IsScenePaused = pause; }
Unordered_set<Ptr<Scene>>& getChildren() { return m_ChildrenScenes; }
SceneID getID() const { return m_ID; }
ImportStyle getImportStyle() const { return m_ImportStyle; }
String getScenePath() const { return m_SceneFile; }
Scene* getParent() const { return m_ParentScene; }
Entity& getEntity() { return m_Entity; }
const String& getName() const { return m_Name; }
const String& getFullName() const { return m_FullName; }
void setFullName(String& name) { m_FullName = name; }
SceneSettings& getSettings() { return m_Settings; }
};
#pragma once
Copy link
Copy Markdown
Contributor

@Aviii06 Aviii06 Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix this, your diff shouldn't be the entire file.


#include "common/common.h"
#include "entity.h"
#include "core/input/input_manager.h"

class Entity;

#define ROOT_SCENE_ID 1

struct SceneSettings
{
ResourceCollection preloads;
SceneID camera = ROOT_SCENE_ID;
SceneID listener = ROOT_SCENE_ID;
HashMap<String, InputScheme> inputSchemes;
String startScheme = {};

void drawCameraSceneSelectables(Scene* scene, SceneID& toSet);
void drawListenerSceneSelectables(Scene* scene, SceneID& toSet);
void drawInputScheme(InputDescription& floatInput);
void draw();
};

void to_json(JSON::json& j, const SceneSettings& s);
void from_json(const JSON::json& j, SceneSettings& s);

class Scene
{
public:
enum class ImportStyle
{
/// If scene is not imported but created raw inside this scene
Local,
/// If scene is linked to another scene file
External
};

private:
bool m_IsScenePaused;

static Vector<Scene*> s_Scenes;

SceneID m_ID;
String m_Name;
String m_FullName;
ImportStyle m_ImportStyle;
/// Contains the current file name if local, else contains the linked scene file
String m_SceneFile;
SceneSettings m_Settings;
Entity m_Entity;

Scene* m_ParentScene = nullptr;
Unordered_set<Ptr<Scene>> m_ChildrenScenes;
Copy link
Copy Markdown
Member

@twaritwaikar twaritwaikar Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use an unordered set? What are the advantages of using one here?

Copy link
Copy Markdown
Contributor

@Bashar-Ahmed Bashar-Ahmed Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, we needed O(1) addressing along with O(1) deletion. Based on this, the most ideal data structure seemed to be an unodered_set. O(1) deletion was needed because currently in many functions we loop over the complete vector which isn't necessary.

Copy link
Copy Markdown
Member

@twaritwaikar twaritwaikar Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bashar-Ahmed Is there any performance testing done to see if using a cache inefficient data structure but with a theoretically better complexity is performing better than a vector?

Also, do you want to save the order in which the children are added in a parent?

Copy link
Copy Markdown
Contributor

@Bashar-Ahmed Bashar-Ahmed Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as of now no performance testing has been done. Will have to look into it and get back to you. Also caching due to vector would've been minimal, if any, due to the fact that it was a vector of ptr, instead of the actual scenes. So when fetching the address,caching would be useful but when getting the object the caching wouldn't be effective. (Had a discussion regarding this with @sin3point14 ). Apologies if this approach is flawed, and do correct me.

As far as the order is concerned, we looked at this aspect and weren't able to find any viable usecase where saving the order of scenes would be necessary

Copy link
Copy Markdown
Member

@twaritwaikar twaritwaikar Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way the order is important is when you need to iterate over the children to display them on the editor. Seeing the order of the children change as you make any changes in the scene hierarchy can get confusing.


bool checkCycle(Scene* child);

public:
static void ResetNextID();

static Ptr<Scene> Create(const JSON::json& sceneData, const bool assignNewIDs);
static Ptr<Scene> CreateFromFile(const String& sceneFile);
static Ptr<Scene> CreateEmpty();
static Ptr<Scene> CreateEmptyAtPath(const String& sceneFile);
static Ptr<Scene> CreateRootScene();
static bool isReservedName(const String& sceneName);

static Vector<Scene*> FindScenesByName(const String& name);
static Scene* FindSceneByID(const SceneID& id);
static const Vector<Scene*>& FindAllScenes();

Scene(SceneID id, const String& name, const SceneSettings& settings, ImportStyle importStyle, const String& sceneFile);
~Scene();

Scene* findScene(SceneID scene);
void reimport();

void onLoad();
bool snatchChild(Scene* child);
bool addChild(Ptr<Scene>& child);
bool removeChild(Scene* toRemove);

void setName(const String& name);

JSON::json getJSON() const;
bool& getIsScenePaused() { return m_IsScenePaused; }
void setIsScenePaused(bool pause) { m_IsScenePaused = pause; }
Unordered_set<Ptr<Scene>>& getChildren() { return m_ChildrenScenes; }
SceneID getID() const { return m_ID; }
ImportStyle getImportStyle() const { return m_ImportStyle; }
String getScenePath() const { return m_SceneFile; }
Scene* getParent() const { return m_ParentScene; }
Entity& getEntity() { return m_Entity; }
const String& getName() const { return m_Name; }
const String& getFullName() const { return m_FullName; }
void setFullName(String& name) { m_FullName = name; }
SceneSettings& getSettings() { return m_Settings; }
};
Loading