Enhancement/unit availability and upgrades

[stavrosfa]

This PR removes the mechanism where we had a "Unique" unit for a civilization, and adds unit availability, or rather, producibility per civilization, and calculates the "true" unit upgrades on the fly.

This was tested agasint .biq, .sav and our own .json and I found everything to be working as they should. Let me know if you find anything out of the ordinary.

I am not quite happy with the quality of the code, but it kind of reflects how weirdly the original game handles upgrades.

A rule was also added to allow units that would be obsolete to be built. The whole thing can be expanded in the future, and as discussed a few times, manipulate some unit availability/upgrade behaviour on the lua level.

Let me know what you think

[ajhalme]

If most units are producible by all civs (within a given ruleset), would it make sense to have something like producibleByAll: true?

This might make the special case handling easier, as the common case is easy to shortcut.

Both existing and the proposed code is quite involved. Would it make sense to try and nail down the target behaviour with some unit tests? (heh!)

[stavrosfa]

I don't think even a single unit is available to all 32 civs (Warrior for example) EDIT: in the basic conquests game, which is what we mirror in the json, but regardless, I indeed thought if some constants like ALL, ALL_EXCEPT_BARBS, etc would be worth it, so I am glad you mention it. It might make parsing a bit more involved, but I am down to try it.
I don't want to add more fields in the class then needed though, and I fear that producibleByAll is a slipery slope. If anything I would add the ALL_EXCEPT_BARBS example, in the civ array as a regular civ name, and then interpret that as, all the civs, except the civ "A Barbarian Kingdom", if that makes sense.

Regarding unit tests, I did try to write some, and essentially I did, I just had to modify parts of existing unrelated code (an easy example would be, I had to change the access modifier of GameData for civilizations from internal to public), so I opted for not modifying other stuff, and put the "tested behind the scenes" code up for review.
Again, I am down to take the time to write some proper tests. I want to advocate more about unit tests anyway where it makes sense (exactly like here), I just kind of fear it will put some people off.

[ajhalme]

Hmm. If the unit rules are more complex than basic filtering can support (like the Explorer/Conquistador case mentioned in #863), maybe the parsing needs to be more sophisticated. One could have a little DSL for this to specify operators to apply. AllExceptBarbs, or AllExcept(['Egypt']), or even things like ReplaceWithIfResourceAvailable('Spain', 'Conquistador', 'Horses') etc., with some logic to apply the parsed operators.

I suppose what I'm really trying to say here is that the long lists of civ names attached to each prototype suggests to me that we might be missing out on a stronger abstraction.

--

A friend assembly designation can be used to make an internal class visible to a test project.

[stavrosfa]

I don't think we need to complicate this more than it's needed. I don't want to embed logic in the json, where we choose unit s based on resources, etc. If that were the case, I would prefer to go with Lua.

In any case, I want to achieve 2 things.
First, reduce any unnecessary repeatable large chunks in the json, and second, make it as hard as possible for someone to make a mistake while creating/modifying such a json.

This is not my first stub at this implementation, have a look at #857 where I tried to achieve this by having units being Unavailable. I don't think that's really intuitive though.
I think by having a middle ground where we have the option of adding any civ by hand, but at the same time being able to parse ALL, ALL_EXCEPT_CIV, etc, it's the best compromise.

Although, I am not sure how usefull this would be in other cases, since we have multiple cases where only 2 or 3 civs don't have a particular unit, what would we do? ALL_EXCEPT_CIV1_CIV2_CIV3? Or add 28-30 civs by hand?
The only positive thing about how the proposed json is, is that, is a 1-1 representation of the code and the logic behind it, and it only needs to be set once.

Or perhaps we are overthinking it, because at some point, there will be an editor, and manual error, where you type a civ's name wrong, will not be an issue.

A friend assembly designation can be used to make an internal class visible to a test project.

Thanks, I will have a look

[stavrosfa]

I wrote some units tests, I am still missing the .json saves though, but they require a different setup, so they will come later on.
The tests cover a very small portion of the civs/units, but I think it works as a proof of concept. If we find an edge case where the code is not working, we can fix the code and then add the case in the unit tests.

So far, besides any manual testing that I had already done, the tests cover most of what was mentioned in #623

@ajhalme thanks for the friend assembly tip, it worked!

I won't add any experimental constants regarding the civ availabilty that was discussed above at this point, a TODO might be enough for now, since I don't want to block any further development that might depending on this PR, and perhaps this feature warrants a PR of its own.

[esbudylin]

Could you add a comment explaining the logic here? I don't get why availability of an upgrade to an upgrade is relevant here.

[stavrosfa]

Hmm, good catch.

I don't remember why exactly I put this there, but I think it was a precursor to the GetUnitsThatUpgradeToThisUpgrade() in the UnitProttype class, and then I simply didn't touch it.

Removing this seems to have no side effects in the actual functionality, so I will be removing it completely.

Thanks for the feedback