search

TeamPorcupine / ProjectPorcupine

Project Porcupine: A Base-Building Game...in Space!
GNU General Public License v3.0
484 stars 280 forks source link

130 line method in Path_AStar needs splitting up [low priority] #869

Closed zwrawr closed 7 years ago

zwrawr commented 7 years ago

Path_AStar.Path_AStar(World world, Tile tileStart, Tile tileEnd, string objectType = null, int desiredAmount = 0, bool canTakeFromStockpile = false, bool lookingForFurn = false)

method goes from line 28 to line 160.

jok-dev commented 7 years ago

Link to method: https://github.com/TeamPorcupine/ProjectPorcupine/blob/master/Assets/Scripts/Pathfinding/Path_AStar.cs#L28-L156

I'm not sure I agree that this method needs splitting up unless you can come up with a good reason (like if the split up code is or can be reused elsewhere).

zwrawr commented 7 years ago

It would defiantly improve readability and would probably improve test ability. But your point stands that currently non of that code is reused.

Sommerbo commented 7 years ago

The pathfinding is pretty much a single unit. But there are a lot of blank lines and a few multiline comments that could be removed.

MattPotticary commented 7 years ago

I would love to pull out the stuff for finding furniture and items and just have the path a* deal with finding the path.

vogonistic commented 7 years ago

@zwrawr This is probably not a good idea.

The path finding needs a rewrite for speed, and I believe @WardBenjamin has started on a different algorithm entirely. Rewriting it for less complexity to read is going to be a temporary measure at best.

@mattpott is correct. The path finding should be a tool we use to find the walkable path or distance from A to B, not who we ask to find things. I know it's there so that we can find the guaranteed closest item, but it should still be the InventoryManager that asks, not the opposite.

vogonistic commented 7 years ago

I think this Issue cursed me. I looked at the code because I'm doing a refactoring and I can't stand that it has two different ways of operating. I'm splitting some off it up. I'll close this issue with that PR.

alexanderfast commented 7 years ago

Oh my I just realized that the A_Star class does so much more than an a star search....

vogonistic commented 7 years ago

It does too much, but yes.

kd7uiy commented 7 years ago

I recommend closing this.

vogonistic commented 7 years ago

Closing in favor of a new pathfinder coming in the close future.