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

(Maybe?) Some issues with the search #1

Open
unlut opened this issue Sep 11, 2021 · 2 comments
Open

(Maybe?) Some issues with the search #1

unlut opened this issue Sep 11, 2021 · 2 comments

Comments

@unlut
Copy link

unlut commented Sep 11, 2021

Hello, I don't know if you are still working on this. I was looking for A* implementations for comparison and after reading your code I think it has a few slight issues (or they might be my misunderstanding of c#).

1- You are always increasing g value by 1, however it should be sqrt(2) for diagonal moves.
2- If any of the cardinal moves are blocked, you are preventing all diagonal moves. Moving in SW direction should not be affected by whether east is blocked or not, only south and west
3- At this part you are checking if you reached a node that is in already open set with a lower f value:

else if (g + neighbour.H < neighbour.F) {

                        neighbour.G = g;
                        neighbour.F = neighbour.G + neighbour.H;
                        neighbour.Parent = node;

But you are not repushing the node to the queue or updating priority of the node in the queue via UpdatePriority. Lowering f value of a node may cause it to expand earlier than some other nodes. Although I can not think of a situation where would this be a problem in uniform-cost grids, it seems incomplete when you look at it.

@JamieG
Copy link
Owner

JamieG commented Apr 1, 2022

Hi, I've only just noticed this! You've probably moved on but thought I'd answer.

  1. I think you are right, it should be using the heuristic for the current to the proposed, I might have swapped it out to see what the effect it would have and forgot to put it back.
  2. This was intentional, if you think about it if your grid cells are the same size, you don't want to be able to move on the diagonal through touching corners of two cardinals. This could change from game to game so you could remove that if you actually want that behaviour.
  3. I think you're right, it's been a while since I've looked at this but yeah once the F value changes I don't think it will sort itself so should probably have a call to UpdatePriority.

If I get a sec I'll update it, thanks for the feedback.

@ziplock9000
Copy link

Did you make these changes? Also with optional diagonals?

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

No branches or pull requests

3 participants