[Bug] Infinite loop while getting contours

Hi @aron_granberg,
I believe I’ve found a bug causing an infinite loop. The issue surfaces when trying to get the contours for a nodes in layered grid graph (probably a grid graph too) with one-way connections in them. I know that probably one-way connections aren’t the greatest thing, but I don’t believe that such cases shouldn’t be unhandled and lead to an infinite loop.

I’ve prepared the reproduction project for you and will share it directly with you.

Please let me know if I can be of any help.

EDIT: I’ve sent you a DM with the link & reproduction steps. Hope that helps!

EDIT 2: I am using beta 4.3.60 in 2021.3.15f1 if that’s relevant.

hi @aron_granberg,
sorry for the bump, but were you able to reproduce this issue?

Sorry for the delay.
Yes, I managed to reproduce it!

I have now implemented a check so that if any one-way connections are discovered, the method will throw an exeption instead of causing an infinite loop. I think this is sensible, because the contour is not really well-defined if one way connections exist.
This fix will be included in the next beta update.

Thanks! I am glad that you were able to reproduce it.

But this got me thinking about one potential issue:
Am I correct, that when updating only part of the graph (for example via GraphUpdateScene) the context.data holds only that part, right?

If so - won’t that lead to some unexpected behavior sometimes? I, for example, have a custom rule that removes connections between nodes if there is a specific collider between them. I wonder if that is the root cause of my problem since I cannot check for the collider properly because the data isn’t complete (I cannot access the neighbor node to verify the existence of the collider both ways). It won’t produce proper results if the part of the graph that is being passed to the rule is too limited.

I don’t know if I made myself clear enough - please let me know if you understand what I am trying to say :slight_smile:

The graph update will actually recalculate slightly larger region.

Say if the desired bounds to recalculate is NxN nodes. Then it will actually recalculate a region of (N+K)x(N+K) nodes (where K is 1 + the grid graphs erosion iterations).
It will, however, read the existing data from the nodes into the data arrays. So if your rule calculated something valid when the whole graph was scanned, then your rule should continue to generate valid data.

oh ok. So it won’t be an issue in my case as I only need (N+1)x(N+1) region and it seems to be the minimum.

thanks for the detailed explanation!

1 Like

I have just looked at the changes related to that. Why is this check present only in UNITY_EDITOR?

Because calculating the contours on a graph with one way connections is not well-defined, so it shouldn’t be done. But the check is also not free. Therefore, I opted for only doing the check in the editor, since it is very likely that if any one-way connections are being used, the game designer will catch it while testing the game in the editor.

1 Like

that makes perfect sense, thanks!

I have a small feature request. Would it be possible to add an extra filter function to GraphUtilities.GetContours with a signature like (fromNode, toNode) to perform some custom filtering of the connections similar to what is done in TraversalProviders. Not all connections are present in the graph in my case, I am using some additional, external checks sometimes and it would be awesome if I would still be able to use non-modified code rather than copy it to my codebase and modify it by hand.

Something like this:

Awesome! As I glimpsed over a screenshot, for a brief second I thought that it was already available and I was making a fuss without looking at the source :smiley:

Anyway, I find the API with signature GraphNode, GraphNode instead of GraphNode, and Direction more convenient to use, but if that matters the direction should be okay also. But wouldn’t it be more concise if it followed the same approach as in the traversal provider?

1 Like

That’s fair enough. I’ll change it to that.

1 Like