Skip to content
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.

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

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

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

Conversation

aaryaag03
Copy link

No description provided.

Copy link
Contributor

@r41k0u r41k0u left a comment

Choose a reason for hiding this comment

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

editor.app.json and imgui.ini must not be pushed

@r41k0u
Copy link
Contributor

r41k0u commented Jan 31, 2023

Please give a better PR name like WIP: Refactoring scene tree to use unordered_set of unique_ptr or something like that

@aaryaag03 aaryaag03 changed the title Scene tree WIP: Refactoring scene tree to make unordered set of unique pointers Jan 31, 2023
@@ -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
Contributor

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
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.

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

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

Choose a reason for hiding this comment

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

?

void setFullName(String& name) { m_FullName = name; }
SceneSettings& getSettings() { return m_Settings; }
};
#pragma once
Copy link
Contributor

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.

{ return l.Dot(r); };
vector2["cross"] = [](const Vector2& l, const Vector2& r)
{ return l.Cross(r); };
sol::meta_function::addition, [](Vector2& l, Vector2& r) { return l + r; },
Copy link
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.

Run a linter here to have this being the original, ideally your diff should only contain things which you've changed functionally or semantiaclly.

rootex/common/types.h Show resolved Hide resolved
@@ -51,7 +51,7 @@ class Scene
Entity m_Entity;

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

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
Contributor

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
Member

@twaritwaikar twaritwaikar Feb 15, 2023

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
Contributor

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
Member

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.

@sin3point14
Copy link
Contributor

sin3point14 commented Feb 16, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants