yellowstonegames / SquidLib

Useful tools for roguelike, role-playing, strategy, and other grid-based games in Java. Feedback is welcome!
Other
448 stars 46 forks source link

FOVCache seems to fail on small maps #193

Closed Rakaneth closed 6 years ago

Rakaneth commented 6 years ago

There seems to be a minimum map size that FOVCache expects; it fails to cache FOV for a 30x30 map but succeeds for 31x31 with no issue. The error happens in FOVCache.java at line 1186.

tommyettinger commented 6 years ago

I've been meaning to re-investigate whether FOVCache actually can improve FOV calculation time. Last I checked, I was discouraged by how much time it takes to precalculate all FOV, even on level entry, compared to just running it as-needed when creatures move. In particular, methods were added to the FOV class that calculate field of vision and assign the result into an existing 2D array, which helps a lot with the load put on the garbage collector. I'll look at this now though; there may be a minimum map size but it shouldn't be 30x30...

tommyettinger commented 6 years ago

This seems to be related to the maxRadius of an FOVCache being too high for the size of the map; I can reproduce this when the size is 30x30 and the maxRadius is 15, but not when maxRadius is 14 or less on the same map. I think there may be some code that refuses to make 31 rows or columns for a 30x30 map, since the 31st row wouldn't refer to anything in the original map. Another possibility is that it isn't doing enough bounds checking, which also seems likely.

Rakaneth commented 6 years ago

That actually coincides with my findings - I have maxRadius and maxLOS set to 15 because that is the highest vision stat any singular character has in the current iteration of my game.

Rakaneth commented 6 years ago

I actually decided to try an FOVCache per map instead of an FOV per creature as proof against a large number of creatures. I am not sure how much in resources an FOV object takes, but I thought that placing something reusable on the map object would be more efficient.

tommyettinger commented 6 years ago

This bug is fixed in the latest commit, but FOV has gotten faster in the last few years, while retrieving compressed FOV maps from FOVCache has not. Here's the results of a benchmark on 60x60 dungeon maps, getting FOV for every non-wall cell:

Benchmark                           Mode  Cnt   Score   Error  Units
FOVCacheBenchmark.measureCachedFOV  avgt    4  16.934 ± 1.622  ms/op
FOVCacheBenchmark.measureFOV        avgt    4   8.070 ± 0.343  ms/op

The Score column shows that retrieving FOV maps from FOVCache takes twice as long as calculating them fresh every time. I may just remove FOVCache or swap it for a compatibility layer, since it takes a somewhat-annoying time to precalculate on smaller maps and a very long time on large maps.

tommyettinger commented 6 years ago

Also, re: FOV resource usage, none if you use the static reuseFOV methods, and not too much if you use the non-static methods. FOVCache uses a LOT, since it stores the compressed light map for every cell on the map, at every vision radius. FOVCache is fine to share between entities though, there's nothing to overwrite with a calculateFOV() result, since it already has calculated all of those for the map. The benchmarks do show that it's slower even in its optimal case (running field of vision calculations on every cell) than using FOV.calculateFOV(); FOV.reuseFOV() should be faster by not producing so much garbage. I suspect there may be something wrong with the benchmarks, because those numbers seem too low (the map may be all walls?).

tommyettinger commented 6 years ago

There probably was an issue with the last benchmark, but the results still stand testing on a 180x180 map.

Benchmark                           Mode  Cnt     Score     Error  Units
FOVCacheBenchmark.measureCachedFOV  avgt    4  3229.723 ± 559.561  ms/op
FOVCacheBenchmark.measureFOV        avgt    4  1501.769 ±  95.325  ms/op

Score is still half the time for FOV.calculateFOV instead of using FOVCache, and probably a lot of this time is in GC that FOV.reuseFOV() could help avoid.