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

Naming nodes #31

Open
Wokarol opened this issue Apr 14, 2020 · 7 comments
Open

Naming nodes #31

Wokarol opened this issue Apr 14, 2020 · 7 comments

Comments

@Wokarol
Copy link
Contributor

Wokarol commented Apr 14, 2020

I touched this asset again and I noticed big flaw when it comes to debugging. Currently we cannot name nodes like selector or sequence in easy way. This is key feature in my opinion.
Without this debugging is basically pointless
image
With naming we would be able to explicitly say "this part of tree is handling steering"

@Wokarol
Copy link
Contributor Author

Wokarol commented Apr 14, 2020

Update: I found out you can "Label" nodes. What about adding extension method or member method to node like

new Selector(...).Labeled("Steering");

or "fake decorator" like

NPBehave.Labeled("Steering", new Selector(...));

Probably best solution would be to add this directly in the constructor like so

new Selector("Steering", ...);

But that would require a lot of changes

@meniku
Copy link
Owner

meniku commented Jun 1, 2020

Hey, so the intended syntax for the labelling is quite similar to your first idea
new Selector(...).Labeled("Steering");

it goes like this:

new Selector(
...
 ) { Label = "Follow" }

adding your idea wouldn't be much work, but maybe you can work already with the syntax we have?

@Wokarol
Copy link
Contributor Author

Wokarol commented Jun 1, 2020

Checked it briefly, it's fine but "Fake Decorator" would be still a little better due to information flow. Because then you have label next to actual node definition and not after this.
This could be added as a simple Utility Method, I might try doing it later.

@meniku
Copy link
Owner

meniku commented Jun 1, 2020

OK. We can maybe also go for the one to include it in the constructor, but it will increase amount of mandatory fields. I would like to keep it optional if possible...

With 2.0 I'll probably have to rethink the way we handle growing amounts of fields in the constructors. Not sure if there is any better way to describe the trees by Code. But I would like to also reduce the amount of constructors if possible.

Maybe we could prefer using named parameters somehow, but I'm not sure if it works with the composites so well. Maybe we could live with something like

new Selector( label: "Follow", children: [
...
] ),

@Wokarol
Copy link
Contributor Author

Wokarol commented Jun 2, 2020

Actual utility method would look more or less like this now that I look at it:

public static class NPBehaveUtils
{
  public static Node Label(string label, Node node)
  {
    node.Label = label;
    return node;
  }
}

Now, you can just add using static NPBehaveUtils on top of the script and done

Label("Steering", new Selector(
  ...
));

@Wokarol
Copy link
Contributor Author

Wokarol commented Jun 2, 2020

I also think, after second thought that it would be the best solution due to it being universal for every Node

@meniku
Copy link
Owner

meniku commented Jun 2, 2020

right. Let's keep it simple for now. For 2.0 we can rethink this

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

No branches or pull requests

2 participants