search

Sesu8642 / FeudalTactics

Strategy game with countless unique and challenging levels.
GNU General Public License v3.0
79 stars 20 forks source link

Add ability to combine two spearmen (issue #77) #78

Closed mwadle closed 2 months ago

mwadle commented 2 months ago

Rework unit combination logic to allow two spearmen to be combined.

Not sure if two spearmen are supposed to be able to be combined, but if so this will allow for it.

Sesu8642 commented 2 months ago

Very elegant, as always :)

I think the bots should be able to do this too, mostly for completeness' sake. Do you mind extending their behavior accordingly? Probably the relevant part of the BotAI class could be refactored in a similar manner but I can do that later.

mwadle commented 2 months ago

Very good point. I'll work on adding the behavior to the bots this weekend.

mwadle commented 2 months ago

I pushed the change. I don't really know a reliable way to test this, but I figure its a straightforward change.

If you want, I could take a stab at trying to refactor part of the botAI class sometime this week. I think I see a way to try to make the AI more efficient at using existing units rather than purchasing new ones.

mwadle commented 2 months ago

I say it seems straightforward, but I messed it up. Pushed a new commit that is hopefully better.

Also if I refactor I might also try to normalize the calculation to figure out if the kingdom can afford the upgraded unit, and check it more consistently.

Sesu8642 commented 2 months ago

I ran some unit tests and could at least test that your code was called and nothing blew up, which is good enough for me ;)

Feel free to do any refactoring in the BotAi class and send a PR. Not sure why the income is not checked in all cases. Maybe because combining existing units is not as impactful on the income as buying new ones. It's been a while since i wrote it.

Sesu8642 commented 2 months ago

Thanks for your contribution!