Improve support for directed graphs in A*; docs update included #30556

ayuusweetfish · 2019-07-13T09:08:11Z

Closes #30520. A bidirectional parameter is added to methods are_points_connected() and disconnect_points(). The documentation has been updated to match current method signatures and to be more specific. (ref: #8146)

Current implementation takes about 2x memory for storing edges. This can be optimized if concerns arise on memory usage.

~~I have used ERR_EXPLAINC to report warnings, but don't see them anywhere. DEBUG_ENABLED is defined. Is that normal?~~ Fixed by using WARN_PRINT, rebased already.

Here is a test. Should it be added to main tests?

var a = AStar.new()
a.add_point(1, Vector3(0, 0, 0))
a.add_point(2, Vector3(0, 1, 0))
a.add_point(3, Vector3(1, 1, 0))
a.add_point(4, Vector3(2, 0, 0))

a.connect_points(1, 2, true)
a.connect_points(1, 3, true)
a.connect_points(1, 4, false)
print('------')
print(a.get_point_connections(1))	# [2, 3, 4]
print(a.get_point_connections(2))	# [1]
print(a.get_point_connections(3))	# [1]
print(a.get_point_connections(4))	# []
print(a.are_points_connected(2, 1))	# True
print(a.are_points_connected(4, 1))	# True
print(a.are_points_connected(2, 1, false))	# True
print(a.are_points_connected(4, 1, false))	# False

a.disconnect_points(1, 2)
print('------')
print(a.get_point_connections(1))	# [3, 4]
print(a.get_point_connections(2))	# []
print(a.get_point_connections(3))	# [1]
print(a.get_point_connections(4))	# []

a.disconnect_points(4, 1, false)
print('------')
print(a.get_point_connections(1))	# [3, 4]
print(a.get_point_connections(2))	# []
print(a.get_point_connections(3))	# [1]
print(a.get_point_connections(4))	# []

a.disconnect_points(4, 1, true)
print('------')
print(a.get_point_connections(1))	# [3]
print(a.get_point_connections(2))	# []
print(a.get_point_connections(3))	# [1]
print(a.get_point_connections(4))	# []

a.connect_points(2, 3, false)
print('------')
print(a.get_point_connections(1))	# [3]
print(a.get_point_connections(2))	# [3]
print(a.get_point_connections(3))	# [1]
print(a.get_point_connections(4))	# []

a.connect_points(2, 3, true)
print('------')
print(a.get_point_connections(1))	# [3]
print(a.get_point_connections(2))	# [3]
print(a.get_point_connections(3))	# [1, 2]
print(a.get_point_connections(4))	# []

a.disconnect_points(2, 3, false)
print('------')
print(a.get_point_connections(1))	# [3]
print(a.get_point_connections(2))	# []
print(a.get_point_connections(3))	# [1, 2]
print(a.get_point_connections(4))	# []

a.remove_point(3)
print('------')
print(a.get_point_connections(1))	# []
print(a.get_point_connections(2))	# []
print(a.get_point_connections(4))	# []

Would love to hear opinions ^ ^

bojidar-bg · 2019-07-15T11:03:30Z

Does remove_point still work?
Also, get_closest_position_in_segment would be twice slower after the change, maybe make it assert that s.from < s.to?

ayuusweetfish · 2019-07-15T11:23:15Z

Does remove_point still work?

unlinked_neighbours are kept valid so I think yes. Maybe an extensive test should be created to confirm that and prevent regression.

Also, get_closest_position_in_segment would be twice slower after the change

Thanks for pointing out! Taking performance and memory use into account, now I think it's better to return to the p_from < p_to assertion and just add another flag denoting its direction (either way or bidirectional). I will make changes soon.

ayuusweetfish · 2019-07-15T17:07:49Z

Hi! Worked intermittently on this and I think it's good now.

I have added both manual tests and random batch tests, hope they are strong enough 😝

bojidar-bg · 2019-07-18T09:03:05Z

core/math/a_star.cpp

+	Set<Segment>::Element *element = segments.find(s);
+	s.direction = bidirectional ?
+						  Segment::BIDIRECTIONAL :
+						  (s.u == p_id ? Segment::FORWARD : Segment::BACKWARD);


Can't this expression be replaced with if (bidirectional) s.direction = Segment::BIDIRECTIONAL?
Also, segments.find(s) should probably be moved to after fixing the segment direction.

bojidar-bg · 2019-07-18T09:04:44Z

core/math/a_star.cpp

+	const Set<Segment>::Element *element = segments.find(s);
+	int direction = bidirectional ?
+							0 : // Use 0 to accept any existing segment
+							(s.u == p_id ? Segment::FORWARD : Segment::BACKWARD);


Can't s.u == p_id ? Segment::FORWARD : Segment::BACKWARD be replaced by s.direction?

bojidar-bg · 2019-07-18T09:05:50Z

Apart from those comments, I think it is good (assuming it works).

ayuusweetfish · 2019-07-18T09:44:13Z

Thanks for your careful review! I have pushed changes and re-run tests.

assuming it works

Currently the list of neighbours are checked, and that should be enough for now since _solve() only depends on them. If desirable, I can add a simple test to generate random graphs and test our A* implementation against Floyd–Warshall.

ayuusweetfish · 2019-07-23T14:57:11Z

I have added an intensive stress test against Floyd–Warshall, which has helped to catch a little bug (diff: original and current). I also manually merged changes in #30708 and fixed a compilation error on CI. (All changes are amended onto their respective commits and the codebase is kept from one stable state to another.)

Hope it's ready now 🚀🚀

(Update: fixed CI warnings again)

doc/classes/AStar.xml

StraToN · 2019-07-25T08:27:24Z

Just 1 tiny little error in documentation, all good otherwise for docs.

profan · 2019-09-09T10:15:07Z

@kawa-yoiko I just happened upon this PR that would be quite nice to have indeed, there's some conflicts with changes I've made since but the tests you've written especially would be amazing to have around!

Any chance you'll pick it back up?

ayuusweetfish · 2019-09-09T16:06:03Z

@profan Sure! I probably will check this ~~tomorrow~~ the day after (sorry... T-T). And thanks for the compliment ^ ^

ayuusweetfish · 2019-09-11T09:57:17Z

@profan It ends up more complicated than I had expected because of bugs in OAHashMap... I shall try to understand and fix them soon. Meanwhile, in case you are interested, the rebased branch so far (where tests fail) is here.

profan · 2019-09-11T10:25:09Z

@kawa-yoiko Heh, yes this is part of what I was afraid of, that there would be things that are still subtly broken.. (OAHashMap is after all only employed in A* and the Godot CSG stuff and then basically nowhere else) so it has had quite little testing overall..

and hopefully the current OAHashMap bugs are not of my doing when trying to fix it last time 🤣.. a robust set of tests is probably necessary there too (in the sense that the current ones pass, but given that your astar one here does not means there must be a problem still)

profan · 2019-09-20T21:54:04Z

@kawa-yoiko I'm gonna take a look at your rebased branch now btw and try to fix it (OAHashMap), hopefully won't take longer than the weekend, with your battery of tests hopefully it can be worked out 👍

ayuusweetfish · 2019-09-21T05:38:09Z

@profan Such a coincidence that I was just looking into it yesterday and had managed to fix the problem! Will push very soon 😊

ayuusweetfish · 2019-09-21T07:06:25Z

The fix → #32230. Let's wait for that first and rebase from there, so that the engine is 'brought from one stable state to another' 😝

ayuusweetfish · 2019-09-28T08:31:18Z

Rebased and all tests are passing now.

profan · 2019-09-30T01:44:33Z

CC @akien-mga

akien-mga · 2019-11-07T11:30:47Z

core/math/a_star.cpp

@@ -227,10 +250,13 @@ PoolVector<int> AStar::get_point_connections(int p_id) {
 	return point_list;
 }

-bool AStar::are_points_connected(int p_id, int p_with_id) const {
+bool AStar::are_points_connected(int p_id, int p_with_id, bool bidirectional) const {


Should be p_bidirectional.

akien-mga · 2019-11-07T11:31:01Z

core/math/a_star.cpp

-	ERR_FAIL_COND(!segments.has(s));
-
-	segments.erase(s);
+void AStar::disconnect_points(int p_id, int p_with_id, bool bidirectional) {


Same here, p_bidirectional.

akien-mga · 2019-11-07T11:33:21Z

We're passed release freeze now, but since it was well reviewed and includes tests, I guess we can make an exception to still merge this. I left a nitpick about the naming of a parameter, but let's merge as is to get more extensive testing as early as possible.

akien-mga · 2019-11-07T11:33:31Z

Thanks!

ayuusweetfish · 2019-11-07T15:41:01Z

Ohh glad to see this finally settled! Should there be further issues spotted I shall still be ready to help >ᴗ<

As for parameter naming, will the maintainers team fix it or do I open a new simple PR?

akien-mga · 2019-11-07T15:56:42Z

As for parameter naming, will the maintainers team fix it or do I open a new simple PR?

There are quite a few occurrences where the convention is not well respected, so we'll do a pass at fixing everything while refactoring code for 4.0 I think.

ayuusweetfish requested a review from a team as a code owner July 13, 2019 09:08

YeldhamDev added documentation feature proposal topic:core labels Jul 13, 2019

ayuusweetfish force-pushed the astar-directed branch from 8cbc7a3 to 2cad664 Compare July 14, 2019 09:20

akien-mga requested a review from bojidar-bg July 15, 2019 06:08

akien-mga added this to the 3.2 milestone Jul 15, 2019

ayuusweetfish force-pushed the astar-directed branch from 9638b33 to 9a1bc0b Compare July 16, 2019 02:09

bojidar-bg reviewed Jul 18, 2019

View reviewed changes

ayuusweetfish force-pushed the astar-directed branch from 9a1bc0b to 2e9df83 Compare July 18, 2019 09:26

ayuusweetfish force-pushed the astar-directed branch 3 times, most recently from d377dbb to 76cc331 Compare July 23, 2019 14:44

ayuusweetfish force-pushed the astar-directed branch from 76cc331 to 3d4a93c Compare July 24, 2019 14:24

StraToN requested changes Jul 25, 2019

View reviewed changes

doc/classes/AStar.xml Outdated Show resolved Hide resolved

ayuusweetfish force-pushed the astar-directed branch from 3d4a93c to ac67fd7 Compare July 25, 2019 12:35

akien-mga requested review from StraToN and bojidar-bg July 29, 2019 15:14

akien-mga added the for pr meeting label Jul 29, 2019

Improve support for directed graphs in AStar

9813641

Revise and update AStar documentation

add0004

ayuusweetfish mentioned this pull request Sep 21, 2019

Implement backward shift deletion for OAHashMap #32230

Merged

ayuusweetfish force-pushed the astar-directed branch from ac67fd7 to ba6436a Compare September 28, 2019 07:53

ayuusweetfish added 2 commits September 28, 2019 16:17

Reduce memory usage for edges in A* and add tests

c2b8246

Add stress test between A* and Floyd-Warshall

0c35994

ayuusweetfish force-pushed the astar-directed branch from ba6436a to 0c35994 Compare September 28, 2019 08:18

akien-mga reviewed Nov 7, 2019

View reviewed changes

akien-mga merged commit ed373a6 into godotengine:master Nov 7, 2019

qarmin mentioned this pull request Nov 8, 2019

Godot crash when using AStar2D::disconnect_points(int, int) #33442

Closed

Calinou removed the for pr meeting label Nov 21, 2021

theraot mentioned this pull request Mar 1, 2022

Godot 3.x and 4.x - Include in AStar2D the possibility to check if a connection is unidirectional or bidirectional, making it works the same as the 3D version of AStar. godotengine/godot-proposals#4142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve support for directed graphs in A*; docs update included #30556

Improve support for directed graphs in A*; docs update included #30556

ayuusweetfish commented Jul 13, 2019 •

edited

Loading

bojidar-bg commented Jul 15, 2019

ayuusweetfish commented Jul 15, 2019

ayuusweetfish commented Jul 15, 2019

bojidar-bg Jul 18, 2019 •

edited

Loading

bojidar-bg Jul 18, 2019

bojidar-bg commented Jul 18, 2019

ayuusweetfish commented Jul 18, 2019

ayuusweetfish commented Jul 23, 2019 •

edited

Loading

StraToN commented Jul 25, 2019

profan commented Sep 9, 2019

ayuusweetfish commented Sep 9, 2019 •

edited

Loading

ayuusweetfish commented Sep 11, 2019

profan commented Sep 11, 2019 •

edited

Loading

profan commented Sep 20, 2019

ayuusweetfish commented Sep 21, 2019

ayuusweetfish commented Sep 21, 2019

ayuusweetfish commented Sep 28, 2019 •

edited

Loading

profan commented Sep 30, 2019

akien-mga Nov 7, 2019

akien-mga Nov 7, 2019

akien-mga commented Nov 7, 2019

akien-mga commented Nov 7, 2019

ayuusweetfish commented Nov 7, 2019

akien-mga commented Nov 7, 2019

Improve support for directed graphs in A*; docs update included #30556

Improve support for directed graphs in A*; docs update included #30556

Conversation

ayuusweetfish commented Jul 13, 2019 • edited Loading

bojidar-bg commented Jul 15, 2019

ayuusweetfish commented Jul 15, 2019

ayuusweetfish commented Jul 15, 2019

bojidar-bg Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

bojidar-bg Jul 18, 2019

Choose a reason for hiding this comment

bojidar-bg commented Jul 18, 2019

ayuusweetfish commented Jul 18, 2019

ayuusweetfish commented Jul 23, 2019 • edited Loading

StraToN commented Jul 25, 2019

profan commented Sep 9, 2019

ayuusweetfish commented Sep 9, 2019 • edited Loading

ayuusweetfish commented Sep 11, 2019

profan commented Sep 11, 2019 • edited Loading

profan commented Sep 20, 2019

ayuusweetfish commented Sep 21, 2019

ayuusweetfish commented Sep 21, 2019

ayuusweetfish commented Sep 28, 2019 • edited Loading

profan commented Sep 30, 2019

akien-mga Nov 7, 2019

Choose a reason for hiding this comment

akien-mga Nov 7, 2019

Choose a reason for hiding this comment

akien-mga commented Nov 7, 2019

akien-mga commented Nov 7, 2019

ayuusweetfish commented Nov 7, 2019

akien-mga commented Nov 7, 2019

ayuusweetfish commented Jul 13, 2019 •

edited

Loading

bojidar-bg Jul 18, 2019 •

edited

Loading

ayuusweetfish commented Jul 23, 2019 •

edited

Loading

ayuusweetfish commented Sep 9, 2019 •

edited

Loading

profan commented Sep 11, 2019 •

edited

Loading

ayuusweetfish commented Sep 28, 2019 •

edited

Loading