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 BTEvaluateExpression #43

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

Rubonnek
Copy link
Contributor

Implements #12.

@Rubonnek Rubonnek marked this pull request as draft February 17, 2024 21:13
@Rubonnek Rubonnek force-pushed the add-bt-evaluate-expression branch 4 times, most recently from a008747 to 9526e73 Compare February 18, 2024 00:31
@Rubonnek Rubonnek marked this pull request as ready for review February 18, 2024 00:59
@Rubonnek
Copy link
Contributor Author

This PR should be ready for review now.

I should say I'm not entirely sure how to concisely name set_expression_string-- set_expression sounds confusing since Expression exists but I don't mind changing it to the latter at all, or to something else.

Same goes for input_include_delta -- include_delta on its own sounds confusing to me within this context.

@limbonaut
Copy link
Owner

Excellent work! It'll make a great addition for 1.0 release. I'll check it out tomorrow, we'll see if I can break it.
Should change those copyrights for yours and also the filenames in the first line.

bt/tasks/utility/bt_evaluate_expression.cpp Outdated Show resolved Hide resolved
bt/tasks/utility/bt_evaluate_expression.cpp Show resolved Hide resolved
bt/tasks/utility/bt_evaluate_expression.cpp Outdated Show resolved Hide resolved
bt/tasks/utility/bt_evaluate_expression.cpp Outdated Show resolved Hide resolved
bt/tasks/utility/bt_evaluate_expression.cpp Outdated Show resolved Hide resolved
bt/tasks/utility/bt_evaluate_expression.h Show resolved Hide resolved
bt/tasks/utility/bt_evaluate_expression.h Outdated Show resolved Hide resolved
bt/tasks/utility/bt_evaluate_expression.h Outdated Show resolved Hide resolved
doc_classes/BTEvaluateExpression.xml Outdated Show resolved Hide resolved
tests/test_evaluate_expression.h Show resolved Hide resolved
@limbonaut
Copy link
Owner

This PR should be ready for review now.

I should say I'm not entirely sure how to concisely name set_expression_string-- set_expression sounds confusing since Expression exists but I don't mind changing it to the latter at all, or to something else.

Same goes for input_include_delta -- include_delta on its own sounds confusing to me within this context.

The input_include_delta is a bit long, but it's fine. In the inspector, it'll be Input --- Include Delta. Doesn't look confusing to me. Plus we need that input_ for ADD_GROUP().
With the set_expression..., whichever you prefer. Both names look good to me.

@Rubonnek Rubonnek force-pushed the add-bt-evaluate-expression branch 2 times, most recently from a313788 to db95e48 Compare February 18, 2024 15:43
@Rubonnek
Copy link
Contributor Author

Rubonnek commented Feb 18, 2024

With the set_expression..., whichever you prefer. Both names look good to me.

I've left it as set_expression_string then.

Looks like the first argument in input_vars is skipped when delta is included

I've added a test for this -- should be fixed now.

Should change those copyrights for yours

Made the change but I'm conflicted about it -- BTEvaluateExpression is mostly a copy-paste of BTCallMethod.

I'm not a copyright law expert, but I wouldn't mind if you did the same as Godot by tracking contributors/donors in AUTHORS.md. We'll have to update the copyright notice to include that file and in the LimboAI editor list yourself and the contributors inside an About section, maybe under Misc, but I'm fine with however you want to handle it.

@Rubonnek Rubonnek force-pushed the add-bt-evaluate-expression branch from db95e48 to 5182694 Compare February 18, 2024 15:56
@Rubonnek
Copy link
Contributor Author

I think this PR is ready for review again.

@limbonaut
Copy link
Owner

With the set_expression..., whichever you prefer. Both names look good to me.

I've left it as set_expression_string then.

Looks like the first argument in input_vars is skipped when delta is included

I've added a test for this -- should be fixed now.

Should change those copyrights for yours

Made the change but I'm conflicted about it -- BTEvaluateExpression is mostly a copy-paste of BTCallMethod.

I'm not a copyright law expert, but I wouldn't mind if you did the same as Godot by tracking contributors/donors in AUTHORS.md. We'll have to update the copyright notice to include that file and in the LimboAI editor list yourself and the contributors inside an About section, maybe under Misc, but I'm fine with however you want to handle it.

Not a copyright expert myself. Perhaps in that case, the copyright notice should be appended to mine:

Copyright YEAR mine
Copyright 2024 yours

Need to take a look at how Godot is doing it. Seems like a good idea with the AUTHORS.md.

@Rubonnek Rubonnek force-pushed the add-bt-evaluate-expression branch from 5182694 to 578f8c4 Compare February 18, 2024 16:50
@Rubonnek
Copy link
Contributor Author

Rubonnek commented Feb 18, 2024

Not a copyright expert myself. Perhaps in that case, the copyright notice should be appended to mine:

Copyright YEAR mine
Copyright 2024 yours

Need to take a look at how Godot is doing it. Seems like a good idea with the AUTHORS.md.

This sounds much better to me. I've made that change.

I don't mind replacing my name on the copyright header with a reference to AUTHORS.md later.

@Rubonnek Rubonnek force-pushed the add-bt-evaluate-expression branch from 578f8c4 to fb958c7 Compare February 18, 2024 17:20
@limbonaut
Copy link
Owner

Awesome, thanks!

@limbonaut limbonaut merged commit 0caa236 into limbonaut:master Feb 18, 2024
16 checks passed
@Rubonnek Rubonnek deleted the add-bt-evaluate-expression branch February 18, 2024 18:44
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