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

Adding Recursive Shadowcasting which allows for 90/180 degree viewsheds #29

Merged
merged 12 commits into from
Mar 31, 2014

Conversation

thesnarky1
Copy link
Contributor

Good evening,

I kept this separate from the random number pull request because this one is larger and may require more time to vet. I figured it would be easier for you if they're separate changes.

I added an implementation of the Rogue basin recursive shadowcasting algorithm (http://www.roguebasin.com/index.php?title=FOV_using_recursive_shadowcasting) as a new FOV class: ROT.FOV.RecursiveShadowcasting. I needed the ability to render just a 90-degree view of a particular mob in a project I'm working on and figured that other authors might need the same some day. Rather than change the existing classes, this class was created and ported from another Github user's (@pushcx) Ruby implementation of the same algorithm.

For compatibility, this new class will behave exactly the same as the other FOV classes if calling compute (showing a 360-degree circle of omniscience) but it also allows for 90-degree quadrants (compute90) and 180-degree hemispheres (compute180). Both of those methods take just one extra argument to note the direction the FOV should be cast in.

Specific changes:

  • Added a new file in src/fov/recursive-shadowcasting.js to hold the new class and added it to the Makefile
  • Added a comment in src/fov/fov.js to make it clear that compute is for 360-degree viewsheds
  • Added tests for 360, 180, and 90-degree viewsheds to the fov.js test spec
  • Updated the fov.html page of the interactive manual to demo the various types of viewsheds available for this algorithm.
  • Recompiled rot.js and rot.min.js

This is a much bigger change than the other one and I'm in no rush. I do believe it adds more to this toolkit that is in line with common needs for roguelike games. Please look at it when you have time and if you have any suggestions/questions, let me know. I'm more than happy to make any further changes to this to get it up to your standards for the toolkit.

Thanks,

@ondras
Copy link
Owner

ondras commented Mar 29, 2014

Very neat, thanks! This is a welcome addition to rot.js, so I am definitely going to accept your code.

Automated merge is now not possible (probably due to conflicts in rot.js/rot.min.js w.r.t. your last pull request), but I will examine your new code and merge the stuff locally. Probably on monday/tuesday.

I have two small recommendations:

  1. please use tab-based indenting for the sake of consistency (tabs are used throughout the whole rot.js project),

  2. you might want to make most of your methods private, as long as they are not meant to be called from the outside. The "compute" and sub-variants "computeNNN" need to be public, obviously, but I believe that all other methods can be underscored to ease any further refactorings.

@pushcx
Copy link

pushcx commented Mar 29, 2014

I am totally delighted to see this code shared, right on. :)

…ithin the project. Updated method naming in RecursiveShadowcasting to use underscores in front of private methods. Recompiled.
@thesnarky1
Copy link
Contributor Author

Thanks for the feedback, I'm glad this will be useful and that I can contribute.

I've updated (I think) all the space indents to tab indents and have a local vimrc file so that any future changes I propose for the project will (hopefully) have the correct indentation.

I've updated all the methods not publicly called in RecursiveShadowcasting to be prefaced with an underscore. I left the OCTANTS variable "public" as it may come in handy for other computations.

I'm not very advanced with git, but I believe I merged your updates with this fov_updates branch. See if Github will let you auto merge it now. It definitely was unhappy with the changes to rot.js and rot.min.js, probably the dateline conflicting between the two branches. If in the future you'd prefer I submit just the src/whatever.js changes and not the compiled files, let me know. I'm happy to do whatever's easiest for you.

@ondras ondras merged commit 25be951 into ondras:master Mar 31, 2014
@ondras
Copy link
Owner

ondras commented Mar 31, 2014

Merged and cleaned up.

@ondras
Copy link
Owner

ondras commented Mar 31, 2014

(sending pull requests without rot.js and rot.min.js is probably the best way in the future...)

@thesnarky1 thesnarky1 deleted the fov_updates branch March 31, 2014 20:34
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

Successfully merging this pull request may close these issues.

None yet

3 participants