Freeciv

A different suggestion thread just reminded me: Obviously, all the local-ranged requirement types that don't currently refer to the tile, but could be given such a meaning (e.g. UnitType) should also be included in Adjacent and CAdjacent ranges (and possibly some others that refer to tiles).

This would really enable all kinds of fixes and workarounds to oppressive restrictions and inability to properly specify certain behaviours. I hope someone will do this!

I've been delaying this practically since effects, requirements, and their ranges got introduced in freeciv-2.0 for the assumed slight performance hit from handling additional range for the only benefit of having slightly more readable interface. It's 202x now, and we're casually taking much bigger performance hits elsewhere, so that argument is not valid any more.
Still, not sure if we should get this to 3.1, i.e., to make this big a change to 3.1 ruleset formats at this point, or postpone to 3.2.

I'm starting work on the first part of this – introducing the new Tile range and moving requirement types from Local to it as appropriate (i.e. when they're referring to the target tile). I'll raise a separate ticket for the followup re: unit-related requirements at Tile/(C)Adjacent range.

Marko Lindqvist wrote:

Still, not sure if we should get this to 3.1, i.e., to make this big a change to 3.1 ruleset formats at this point, or postpone to 3.2.

So, are we still targetting 3.1, or pushing this back? A quick git diff S3_1..master -G"REQ_RANGE_" suggests almost no relevant changes (in code, at least) between the two branches, so making patches for both will not be much additional work; i.e. on the implementation side, this seems to be mainly a question of where to put rscompat stuff.

Could we avoid duplicating (and hardcoding) range names in rscompat_req_range_3_2(), i.e., to use their enum values instead?

Neither rscompat_req_range_3_2() or its call checks only if we are allowed to update a ruleset (compat_mode), but not if the ruleset loaded is already of 3.2 format. We should only convert those ranges when loading 3.1 ruleset, not when loading 3.2 ruleset. Granted it's currently harmless as there's no new meaning for "Local" range for any of those requirement types that we would need to preserve in 3.2 rulesets.

About testing: Are you aware of any reason why this could alter autogames, i.e., does it make sense to do regression testing for this with autogames? (running a game with the same seeds with and without the patch, and check that final savegames are identical, short of things like event cache time stamps)

Marko Lindqvist wrote:

Could we avoid duplicating (and hardcoding) range names in rscompat_req_range_3_2(), i.e., to use their enum values instead?

Will do. Presumably this goes for the requirement / universal types as well?

Marko Lindqvist wrote:

Neither rscompat_req_range_3_2() or its call checks only if we are allowed to update a ruleset (compat_mode), but not if the ruleset loaded is already of 3.2 format. We should only convert those ranges when loading 3.1 ruleset, not when loading 3.2 ruleset. Granted it's currently harmless as there's no new meaning for "Local" range for any of those requirement types that we would need to preserve in 3.2 rulesets.

I thought about that too (and checked how S3_1 rscompat does it, which didn't help); the problem is that requirements are used in multiple different ruleset files – which file's version should be checked? All possibly relevant ones (and hope they don't disagree)? Just one of them (game? effects?) and hope the others are the same? Infer it based on the name of the file passed to lookup_req_list() (which sounds bodgy and complicated)? Pass it as an additional parameter to lookup_req_list() (which seems like quite a significant interface change for something that won't be relevant most of the time)?

Marko Lindqvist wrote:

About testing: Are you aware of any reason why this could alter autogames, i.e., does it make sense to do regression testing for this with autogames? (running a game with the same seeds with and without the patch, and check that final savegames are identical, short of things like event cache time stamps)

I don't believe anything should change – if everything works as intended, this is purely a refactoring (from the perspective of the game).

Alina L. wrote:

which file's version should be checked? All possibly relevant ones (and hope they don't disagree)?

Now that you mention that, I've long thought that an attempt to support individual files from different versions does not really make sense. There might be some theoretical use-case for it, but in practice mixed files would always indicate an error somewhere.

Opened https://osdn.net/projects/freeciv/ticket/43708 about what I think should be done to that general problem.

Now, should we consider that new ticket a dependency for this one, to be able resolve above issue cleanly, or do we have a good way to resolve it without?

Alina L. wrote:

Marko Lindqvist wrote:

About testing: Are you aware of any reason why this could alter autogames, i.e., does it make sense to do regression testing for this with autogames? (running a game with the same seeds with and without the patch, and check that final savegames are identical, short of things like event cache time stamps)

I don't believe anything should change – if everything works as intended, this is purely a refactoring (from the perspective of the game).

Can you do such test runs with a couple of different rulesets, or do you want me to do it?

Marko Lindqvist wrote:

Alina L. wrote:

Marko Lindqvist wrote:

About testing: Are you aware of any reason why this could alter autogames, i.e., does it make sense to do regression testing for this with autogames? (running a game with the same seeds with and without the patch, and check that final savegames are identical, short of things like event cache time stamps)

I don't believe anything should change – if everything works as intended, this is purely a refactoring (from the perspective of the game).

Can you do such test runs with a couple of different rulesets, or do you want me to do it?

I'll give it a shot.

Alina L. wrote:

I'm not exactly keen on having a patch lying around (and periodically making sure no concurrent changes broke it)

Yeah. This patch touches quite a many places, so it has relatively high risk of catching bitrot.

In the new patch, I changed it to check all relevant versions and only apply when all of them are old; i.e. soft-unsupporting mixed versions – if a ruleset author is using mixed versions, that's on them and they have to deal with it. It's not exactly a "good" way to resolve it, but it could be worse.

While I've not yet looked at this new version, my personal opinion is that mixed version support is already broken, and this patch is not increasing the breakage.

So this new patch could be used, but I also understand if you'd rather wait on the other one – I for one am glad not to be the one who has to make that final call.

As long as the thing has not been escalated to Maintainers group, i.e. remains within freeciv-dev, your vote has the same weigh as anybody else's.

Alrighty then.

Regression tests passed for each of the supplied (non-modpack) rulesets (I did one pair of 7-player, 250-turn games each).

Alina L. wrote:

A different suggestion thread just reminded me: Obviously, all the local-ranged requirement types that don't currently refer to the tile, but could be given such a meaning (e.g. UnitType) should also be included in Adjacent and CAdjacent ranges (and possibly some others that refer to tiles).

This is very exciting to remove some weird disabilities and handicaps that ruleset designers currently suffer. It feels VERY close to the #1 wishlist item of every ruleset designer, so forgive me if I bring it up...

Let's take the case of Civ1 and Civ2, but also this is a case that frustrates EVERY ruleset designer in similar ways...

Fortified units in cities don't get the fortify bonus if there are City Walls but they DO get the fortify bonus if the attacker of the city ignores city walls (e.g., Howitzer).
Currently, there is just no way to test reqs for the attacker type and the defender type in the same effects.

So if we split some stuff into Tile req, I would sure like to take part in another req called "Counterpart", it means the OTHER unit (attacker if defender, defender if attacker).