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
Extract distance field computation from rcErodeWalkableArea #121
base: main
Are you sure you want to change the base?
Conversation
… separate function
const int nidx = (int)chf.cells[nx+ny*w].index + rcGetCon(s, dir); | ||
if (chf.areas[nidx] != RC_NULL_AREA) | ||
const int nidx = (int)chf.cells[nx + ny*w].index + rcGetCon(s, dir); | ||
if (chf.areas[nidx] != fromAreaID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this part is entirely correct. This will set the distance of a span to 0 not only when it has at least one neighbor with fromAreaID
, but also when at least one of the neighbors is missing (rcGetCon
returns RC_NOT_CONNECTED
). That makes sense in the original use-case ("expanding" the unwalkable area), but isn't what we want when computing a general distance field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you mention that, I'm not sure how this code does not potentially end up indexing invalid area indices, because RC_NOT_CONNECTED is defined as 0x3f, so I would have expected us to end up with invalid high nidx values. Are you sure that rcGetCon() is actually capable of returning RC_NOT_CONNECTED in this situation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, no, this block is only entered if rcGetCon
does not return RC_NOT_CONNECTED
(see line 57 in your commit), so nidx
is safe to use. However, the variable nc
can only be incremented in this block; in other words, the only way it can equal 4 on line 69 is if all four neighbors are connected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, now I get you. I've changed it to only do this missing-neighbour check if the field is being based on RC_NULL_AREA
.
…ting is based on RC_NULL_AREA
Pull it out into a separate function to make it easier for people to do custom erosion behaviour based on the intersection of multiple distance fields (e.g. correct handling of 'crawlspace' areas when eroding)