Debbie #374
Debbie #374
Conversation
Preferences screen when the button is pressed. MenuScreen is inactive while Preferences is active. Clicking Back and Save buttons for the moment simply closes the Preferences screen and brings the user back to the main MenuScreen. Data structures for saving settings need to be added.
Conflicts: bubolo/src/bubolo/ui/MenuScreen.java Conflict resolved by discarding version on branch Debbie (ours) and using version on branch production (theirs).
- All meta-data for algorithm (f, g, pointers, etc.) are now stored in a separate per-tile data structure, but original Tile class is un-touched. - Calculations are now in floating point, because the tie-breaking heuristic uses a small fractional perturbation. - Complete structure of the algorithm is now in place. The code compiles without any errors or warnings. However, the movement costs are currently set to a constant of 1. TODO: - Movement cost should query tile properties and assign an appropriate cost. - Integration with tank/engineer controller.
- Several bug fixes.
- Algorithm now queries movement speed in Terrain class to scale
cost based on terrain properties.
- TODO:
- Need some corner-case checks. For example, if the goal is
at an un-reachable tile, the algorithm will probably go
into an infinite loop.
- Integration into controller.
|
Queued behind #373 |
|
This is queued behind #373, but I've done an initial review so you can address the issues:
I used the same source for pseudocode and descriptions of the algorithm when I first implemented A* last semester (Amit Patel's game programming pages). I found that it had by far the clearest description of the algorithm our of any of the sources I looked at. |
AStar pathfinding AI. Engineer can now be evicted from the tank. Clicking on a screen location invokes the pathfinding AI, and the engineer can be seen moving to the destination. There are still some bugs and corner cases that need to be resolved - clicking on certain locations causes the algorithm to crash.
|
I have fixed all issues except the test coverage. I have managed to get an Engineer running around using this algorithm, but some corner cases still need to be handled. Because we have so little time, I would like to complete that before writing any unit tests. Please feel free to cancel this pull request, I can create another one once the Engineer work is done, and you can integrate everything together. |
|
So you're not going to write the unit tests? |
…isbehaving the destination was unreachable. It now returns null and appears to be fully stable. - Added Network Move command for engineer. - Removed some debugging messages. - Updated allowable terrain/tiles for engineer eviction and pathfinding. Solid stationary elements are disallowed, all other elements are allowed. For terrain, the assumption is that the engineer can swim over shallow water but will drown over deep water.
|
Preliminary Review:
|
|
Will push some updates into my branch soon.
I don't see any warnings, I always try to revolve all warnings before pushing. If you could let me know the warnings you see, I can try to fix them.
Fixed.
Fixed.
Could not replicate, but above fix probably fixed this too.
Added a number of unit tests, including the earlier missing tests for AStar and AStarNode. This includes testing for trivial functions like getters and setters, but I did not count how many I actually added.
Latest merge with production did indeed result in conflicts. I have fixed them and pushed everything out into my branch. |
- Updated unit tests for Tank to include testing Engineer eviction - Added Mock Engineer Creator for testing - Fixed a bug where attempting to evict an engineer from an off-screen tank would cause the game to crash. - Fixed a bug where clicking on an unreachable destination while the engineer was already moving would cause the game to crash.
Conflicts: bubolo/src/bubolo/world/entity/concrete/Engineer.java bubolo/src/bubolo/world/entity/concrete/Tank.java bubolo/test/bubolo/world/entity/concrete/TankTest.java
|
Thank you for resolving many of the issues. Regarding the warnings: Are you using the preferences file from the tools directory? It was last updated on 3/21 to include warnings that were defined by our SQAP, but that were missed in the original preferences file. The asserts used in the A* tests are java asserts, which are ignored by JUnit and the JVM by default. Use JUnit's assertEquals, assertTrue, assertFalse, etc methods instead. Yes, getters and setter tests are trivial to write. But they're necessary since people make mistakes, especially when refactoring code in a multi-developer project. |
Done, thanks.
Fixed. |
|
Review:
The A* unit test looks to be an effective test. Nice work. |
…Test to fail. - Compliance fixes
…ering pathfinding and movement for both the local and remote engineers. This was because an AIEngineerController was being instantiated for each engineer. The fixes are: - Added a NetworkEngineerController (similar to NetworkTankController) - Added a CreateEngineer class (similar to CreateTank), which creates a NetworkEngineerController instead of an AIEngineerController. - Updated KeyboardTankController to call CreateEngineer() instead of CreateEntity(). - Added unit tests for NetworkEngineerController and CreateEngineer, and ran JUnit testing to make sure all tests passed.
if the clicked destination tile is on non-traversable terrain. Earlier the AStar algorithm would run on the entire map before it realized the destination was unreachable.
Fixed.
Fixed.
Fixed.
Removed.
Fixed.
Fixed. This is because I was not checking to see if the destination tile is even traversable before calling AStar. The algorithm was scanning the entire map before realizing there was no path. The check fixes the lag. However, note that clicking on a traversable tile (e.g. grass) that is unreachable because there is no path to it, will cause AStar to run on the full map and cause a lag. |
A* algorithm implemented. Does not impact any other code. Compiles cleanly, but correctness needs further testing.