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

Added ability to change A-star cost function #8146

Merged
merged 1 commit into from
Apr 3, 2017
Merged

Added ability to change A-star cost function #8146

merged 1 commit into from
Apr 3, 2017

Conversation

supagu
Copy link
Contributor

@supagu supagu commented Mar 25, 2017

Added _estimate_cost and _compute_cost to allow for both C++ and gdscript inheritance to customize the A-star cost function.

See discussion here for further information:
#7932

This was referenced Mar 25, 2017

float AStar::_compute_cost(int p_from_id, int p_to_id) {
if (get_script_instance())
return get_script_instance()->call("_compute_cost", p_from_id, p_to_id);
Copy link

Choose a reason for hiding this comment

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

What if there is no GDScript implementation of _compute_cost?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be done somewhat as here, I guess:

if (get_script_instance()) {

(Note the use of SceneStringName too, it would make it an idea faster..)

@akien-mga
Copy link
Member

You have indentation issues, it doesn't pass our clang-format style check: https://travis-ci.org/godotengine/godot/jobs/214933818

@supagu
Copy link
Contributor Author

supagu commented Mar 27, 2017

what are the godot tab settings? I noticed this with my other PR also.

@akien-mga
Copy link
Member

One tab for indent, as you can see in all source files ;)

@supagu
Copy link
Contributor Author

supagu commented Mar 28, 2017

Fixed tabbing and checking for existence of methods before calling.

@bojidar-bg
Copy link
Contributor

Now you have to fix clang-format issues (check the travis build for details)

@supagu
Copy link
Contributor Author

supagu commented Mar 28, 2017

would someone help me work out what this job log is trying to tell me?

@bojidar-bg
Copy link
Contributor

@supagu it is in git patch format. Going through it reveals that you have some trailing spaces/tabs here and there around you code, which you should remove.

@supagu
Copy link
Contributor Author

supagu commented Mar 28, 2017

oh is the - at the start of the line the current line as it is, where the + is what it things it should be changed to?

@bojidar-bg
Copy link
Contributor

@supagu Yeah, exactly, thats the idea (-) is remove, (+) is add, when combined it becomes "change".

@ghost
Copy link

ghost commented Mar 29, 2017

@supagu, would you also be interested in adding some documentation to AStar overall in doc/base/classes.xml? Currently, the documentation for AStar as a whole is mostly lacking.

@bojidar-bg
Copy link
Contributor

@tagcup I think this should be left for another PR, since the classes.xml is currently in a sad state.

@ghost
Copy link

ghost commented Mar 29, 2017

OK, but I think at least _estimate_cost and _compute_test should be added (which the functions introduced in this PR), even without description.

@akien-mga
Copy link
Member

OK, but I think at least _estimate_cost and _compute_test should be added (which the functions introduced in this PR), even without description.

They are added automatically by -doctool when it's run, but right now we don't run it as it would lose a lot of information. @bojidar-bg is working on it.

@ghost
Copy link

ghost commented Mar 29, 2017

@akien-mga Fair enough, although I still think adding a few words what _estimate_cost and _compute_cost exactly are would be nice.

(even a brief code comment would be clarifying)

@akien-mga
Copy link
Member

Now documentation can be added, -doctool should be safe to use again. See http://docs.godotengine.org/en/stable/contributing/updating_the_class_reference.html

clang-format seems to still be unhappy with some trailing spaces btw: https://travis-ci.org/godotengine/godot/jobs/215899027 (not easily visible on the logs, but if you select the lines starting with - you'll see that they contain whitespace).
I will soon put some information in the docs about how to setup a dev environment to respect the clang-format style without hassle, but in the meantime you can check what I wrote in #7955.

@supagu
Copy link
Contributor Author

supagu commented Apr 1, 2017

all passing, finally! was there anything else I needed to do?

Copy link
Contributor

@bojidar-bg bojidar-bg left a comment

Choose a reason for hiding this comment

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

Looks good to me, thought it would be nice if someone else checks it as well...

@akien-mga akien-mga merged commit 5b09dde into godotengine:master Apr 3, 2017
@plucian
Copy link

plucian commented Apr 3, 2017

Could this be merged into the 2.1 branch as well ?

@akien-mga
Copy link
Member

Cherry-picked as 1a1e25b.

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

Successfully merging this pull request may close these issues.

5 participants