Skip to content

added a pre-order traversal of the scene graph.#170

Closed
shamanDevel wants to merge 3 commits into
jMonkeyEngine:masterfrom
shamanDevel:pre-order-traversal
Closed

added a pre-order traversal of the scene graph.#170
shamanDevel wants to merge 3 commits into
jMonkeyEngine:masterfrom
shamanDevel:pre-order-traversal

Conversation

@shamanDevel
Copy link
Copy Markdown
Contributor

Adds a new method called depthLastTraversal() to Spatial and subclasses as an extension to depthFirstTraversal() and breathFirstTraversal().

Called depthLastTraversal to keep name coherent with depthFirstTraversal
@pspeed42
Copy link
Copy Markdown
Contributor

First, it's not at all clear to me that "depthLastTraversal" is the best name for pre-order traversal. While I get it, it sounds backwards. Adding a second depthFirstTraversal that took a preorder flag might have been preferable.

I wonder what the use-case is, though. It seems all of the typical use-cases I can think of, post-order is fine.

@shamanDevel
Copy link
Copy Markdown
Contributor Author

Yea, I was thinking longer about the name. preOrderTraversal() would break the naming sheme with the other traversal methods. The extra flag is a good solution. I will change it.

The use-case is building a tree out of the spatials (e.g. in a UI). There, I need the parent first to create the node and then the children.

@pspeed42
Copy link
Copy Markdown
Contributor

But it seems like in that case, that your tree model (I assume there is one) would want to wrap the actual node hierarchy... thus not even really needing the traversal API. What kind of GUI is this for?

@shamanDevel
Copy link
Copy Markdown
Contributor Author

I use it in two ways:

  1. As a scene dump. The output is much more clear if the parent is listed first.
  2. building a node hierarchie without wrapping, so it is independend from the actual spatials

@jmaasing
Copy link
Copy Markdown

+1 for adding a flag or maybe even an Enum or similar for indicating the strategy for walking the tree (pre-order, in-order, post-order). Preferrably matching the description on wikipedia:
http://en.wikipedia.org/wiki/Tree_traversal

Pre-order traversals are convenient for duplicating a tree.

@pspeed42
Copy link
Copy Markdown
Contributor

"in order" doesn't really make sense in this case as there is no "left half" and "right half"... which is why I suggested the flag.

@shamanDevel
Copy link
Copy Markdown
Contributor Author

Is this pull request now acceptable?

@empirephoenix
Copy link
Copy Markdown
Contributor

So any answer here? This is kinda long stalled as well, but look sfine for me, merge?

@empirephoenix
Copy link
Copy Markdown
Contributor

@shadowislord @pspeed42 Any rejections? Else I will merge this(manually) next time I review the open pull requests.

@Shredder121
Copy link
Copy Markdown

Is it maybe a good idea to add a method rather than replacing it?
Backward compatible patches are more likely to be applied quickly.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I didnt see this one, my comment can be ignored.

@empirephoenix
Copy link
Copy Markdown
Contributor

Closing due to merge conflict, however I would like to see this again in a form that just adds methods, as that can be easily applies without braking anything. I personally find different traversal methods usefull.

@shamanDevel
Copy link
Copy Markdown
Contributor Author

Oops, I totally forgot that this was still hanging around.
I'll start with a clean pull from the head of the jme3 branch again. Then there should be no merge conflicts and I'll include all the suggestions. You'll see a new pull request soon.

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