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

Field of view does not always include internal points near diagonal jumps of the circular boundary #56

Closed
felix-clark opened this issue Jan 26, 2020 · 4 comments
Labels
awaiting_confirmation Believed fix, waiting for testing

Comments

@felix-clark
Copy link
Contributor

The field-of-view algorithm traces out a circle and scans lines to the center point. The problem is that this can miss points adjacent to the boundary that should be within the field-of-view.

#55 provides an alternate version of the Bresenham circle that jumps either in x or y (not both) to include these interior points in the set of locations to scan from, and makes some corresponding changes to tests. I did not want to replace the classic Bresenham algorithm, but it doesn't appear to be used elsewhere in the library so it might make sense to remove it and only use the new version.

@Zireael07
Copy link

How about a picture/graph explaining what do you mean?

@felix-clark
Copy link
Contributor Author

felix-clark commented Jan 26, 2020

For instance, the upper left quadrant for the FOV with range 5 appears to be (where X are the visible squares):

   XXX
  X XX
 X XXX
X XXXX
XXXXXX
XXXXX@

The blank squares inside should be within the view radius, but happen to not quite be included in any of the lines scanned from the edge to the center.

The suggested solution I linked is to change the circle traced out from this (now the edge is marked with # for emphasis):

   ###
  # XX
 # XXX
# XXXX
#XXXXX
#XXXX@

to this:

   ###
  ##XX
 ##XXX
##XXXX
#XXXXX
#XXXX@

so now these points are included in the visible range.

@thebracket
Copy link
Collaborator

I just merged in the PR you sent: #55
It should be fixed now?

@thebracket thebracket added the awaiting_confirmation Believed fix, waiting for testing label Jan 28, 2020
@felix-clark
Copy link
Contributor Author

Indeed this appears to be fixed in 0.6.1. Thanks for the quick review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting_confirmation Believed fix, waiting for testing
Projects
None yet
Development

No branches or pull requests

3 participants