how exactly should it be done? in my opinion there are 3 logically separate ruleset parts being configured, the auto performers which we for now only expose as auto attack, the actions themselves in a actions section and action enablers, i separated the 3 things in only one function, so for anything besides try to load all from one and then if it can't find something load from the other i will need to change my current code, which i can do, however in the case we don't want to force everything to be in the same ruleset file how should we separate?
can the actions section be split between the ruleset files? that seems difficult to do as we sanity check immediately after loading, so it may need a lot of changes to split sections. on the other side splitting the action enabler iteration into its own function seems more easily doable.
so my suggestion would be to check if actions has both auto_attack and actions sections, if not try to find those sections in game.ruleset and then load action enablers from both game.ruleset and actions.ruleset. is that acceptable?
You're the one who has had the close look at it, and I don't think it's a problem if the ruleset author needs to move everything at the same time.
Reply To cazfi
You're the one who has had the close look at it, and I don't think it's a problem if the ruleset author needs to move everything at the same time.
ok if the transition can be at once. this here should do it
Breakage:
$ ./tests/rulesets_not_broken.sh
...
rulesetdir is civ2civ3 but stub was expected.
Minor issues:
- Make the likes of "actions_section = secfile_section_by_name(gamefile, "actions") == NULL;" clearer as: "actions_section = (secfile_section_by_name(gamefile, "actions") == NULL);
- In "TODO: in 3.3" comment you could have a magic word 'RSFORMAT_3_2' as that's what we are going to search when looking for the code to remove
Reply To cazfi
Breakage:
$ ./tests/rulesets_not_broken.sh
... rulesetdir is civ2civ3 but stub was expected. Minor issues:
- Make the likes of "actions_section = secfile_section_by_name(gamefile, "actions") == NULL;" clearer as: "actions_section = (secfile_section_by_name(gamefile, "actions") == NULL);
- In "TODO: in 3.3" comment you could have a magic word 'RSFORMAT_3_2' as that's what we are going to search when looking for the code to remove
didn't know we had tests, i was checking each ruleset manually. Does the parenthesis really make it clearer? well you asked so i added.
i have nmot problem requiring action enablers
We could require them on master, but not on S3_1. This solution breaks on stub ruleset updated from S3_1, and I see no way, that would make sense, to add a "dummy" enabler on update.
The update can be tested by setting FREECIV_DATA_PATH to point to S3_1 src/data + master src/data (for comments-3.2.txt), and then running master ./tests/rulesets_upgrade.sh (I just figured this one out myself - these tests have been implemented for CI, but with some recent changes elsewhere they now work also on normal development environment)
My invocation:
$ FREECIV_DATA_PATH="/fast/freeciv/S3_1/src/data:/fast/freeciv/master/src/data" ./tests/rulesets_upgrade.sh
Can also test the "loading current rulesets in compat mode" with:
$ FREECIV_DATA_PATH="/fast/freeciv/master/src/data" ./tests/rulesets_upgrade.sh
i made the patch better now only auto_attack and actions section need to be moved simultaneously to actions.ruleset action enablers can be moved partially. this results in not breaking stub, as there is no check for action enablers
Having both this and #45065 applied, ./tests/rulesets_save.sh fails. We forgot the actual ham of this ticket - to support actions in the actions.ruleset?
At least, to me the error seems like the test first loads (repo version) + saves (-> actions to actions.ruleset) ruleset fine, but then either loading that ruleset or saving it again for comparison does not work correctly.
I started looking at what's the problem, but immediately noticed #46196 that needs to be cleared first.
The old patch didn't apply any more, and I came up to so many things to fix that I ended writing a new patch from scratch.
Testing with #45065 reveals some issue with "paradrop_to_transport"
"paradrop_to_transport" issue resolved for now - not really happy to peek at game.ruleset secfile when loading actions.ruleset, but any alternative would have been more work (maybe something for future tickets?)
Split from #41572
For easing the migration of rulesets from the old format where actions are defined in the game.ruleset to the new format where they live in actions.ruleset, I'd like the code to support both for a while (as it shouldn't be hard to code). So people would have a window of doing that relatively big change to their ruleset, and not one exact freeciv commit after which it should be done.
Basically the ruleset loading code should try to load actions from one of game.ruleset / actions.ruleset first, and if there's zero enablers there, then try to load from the other.