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

add SceneNode deletion convenience methods #2963

Merged
merged 7 commits into from
Nov 7, 2023

Conversation

ribbon-otter
Copy link
Contributor

This commit adds convenience method to delete a node and its attached objects, as I needed and people asked about here and here.

The use case is if you want to clear a SceneNode and put everything back from scratch (a simple if slower way of updating an object to reflect game state), or if you wish to stop rendering a node (and thus things attached to it can be garbage collected).

This patch adds three new non-virtual methods to SceneNode that analogous to the other methods but also delete the detached objects:

SceneNode::destroyGraphSegment, analogous to SceneNode::removeAndDestroyAllChildren, except it also destroys all MovableObjects attached to this and to any of the descendants. In other words, all children are destroyed and any objects attached to them and any objects attached to this.

SceneNode::destroyChildGraphSegment, analogous to SceneNode::removeAndDestroyChild, except it also destroys the objects attached to the child.

SceneNode::destroyAllObjects, analogous to SceneNode::detatchAllObjects, except it destroys the objects too. Unlike detatchAllObjects, this function is not virtual. I believe it is not required for it to be, since everything it does could be achieved using external methods. Moreover, I did not want to increase memory footprint of SceneNode. I called it destroyAllObjects rather than detatchAndDestroyAllObjects to keep the name short and because detatching is already considered part of destroying MovableObjects.

The naming scheme of GraphSegment was chosen as removeAndDestroyAllChildrenAndAttached seemed too long. removeAndDestroy in the original methods' names feel redundant to me, but best not changed as to not break the API.

There are tests added to test the basic behaviour of the 3 added methods included in this pull request.

This code deletes MovableObjects only; I have never worked with skeletons or animations, so it may not delete those. I am not sure if deleting animations would even be desired behaviour, as animations might be more analogous to meshes than entities.

Please view the code with some skepticism, as I am unfamiliar with the Ogre code base and thus, it is greater risk that I broke expected behaviour or introduced undefined behaviour. And I am of course, happy to make any changes that are needed.

@ribbon-otter
Copy link
Contributor Author

is there a command to run the trailing whitespace check locally? I have been using make && make test and it doesn't seem to be catching it.

@paroj
Copy link
Member

paroj commented Nov 6, 2023

is there a command to run the trailing whitespace check locally?

git diff --check origin/master

@@ -194,6 +194,15 @@ namespace Ogre {
needUpdate();
}
//-----------------------------------------------------------------------
void SceneNode::destroyAllObjects(void)
{
while (getAttachedObjects().size() > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

@paroj
Copy link
Member

paroj commented Nov 6, 2023

The naming scheme of GraphSegment

While GraphSegment technically makes perfect sense, it is not used anywhere else within ogre. Therefore I would rather keep to "Children" and "Objects" for now.

* Does **not** destroy animations, textures, meshes associated with those movable objects
* Does not destroy the node itself
* */
void destroyGraphSegment();
Copy link
Member

@paroj paroj Nov 6, 2023

Choose a reason for hiding this comment

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

Suggested change
void destroyGraphSegment();
void destroyAllChildrenAndObjects();

*
* Does **not** destroy animation, textures, meshes associated with those movable objects
* */
void destroyChildGraphSegment(const String& name);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void destroyChildGraphSegment(const String& name);
void destroyChildAndObjects(const String& name);

}

void SceneNode::destroyChildGraphSegment(unsigned short index) {
SceneNode* pChild = static_cast<SceneNode*>(getChildren()[index]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SceneNode* pChild = static_cast<SceneNode*>(getChildren()[index]);
SceneNode* pChild = static_cast<SceneNode*>(removeChild(index));

otherwise pChild might be OOB. This is an issue in the original code as well

@ribbon-otter
Copy link
Contributor Author

I believe I have made the changes you have requested

@paroj paroj merged commit 356ee5e into OGRECave:master Nov 7, 2023
4 checks passed
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.

2 participants