search

nkzw-tech / athena-crisis

Athena Crisis is an Advance Wars inspired modern-retro turn-based tactical strategy game. Athena Crisis is open core technology.
https://athenacrisis.com/open-source
Other
1.37k stars 105 forks source link

Add optional win condition amount feature #47

Open sookmax opened 3 weeks ago

sookmax commented 3 weeks ago

This PR is my attempt at #35.

The implementation kinda got bloated to cover the case where optional objective amount fails due to one of the target objectives is no longer achievable as discussed extensively in #35 with @cpojer.

I feel like there are still a lot of rough edges to be ironed out in the PR, but thought it'd be better to share the work early than to keep pulling my hair out, thinking about some potential edge cases by myself.

cpojer commented 3 weeks ago

Sorry for the late response, it took me a moment to get back to this PR. I left a few comments – am I right in assuming the biggest challenge is making sure which players to check for denied conditions? What else do you think is a rough edge?

sookmax commented 3 weeks ago

I left a few comments – am I right in assuming the biggest challenge is making sure which players to check for denied conditions? What else do you think is a rough edge?

Yeah pretty much. By rough edges I think I meant more of my general uncertainties around the implementation of DeniedOptionalObjective (not knowing the full implications of this implementation on other parts of the project), not something specific I wanted to discuss.

cpojer commented 3 weeks ago

Got it! It looks very similar to how I would have implemented it, so I'll try to take a look through all the possibilities of where things could go wrong with this condition before merging. Would you mind updating the tests?

sookmax commented 3 weeks ago

Would you mind updating the tests?

I guess the charge values have been updated? This is done in 2bdeb16