待辦事項 #45064

Support action enablers in actions.ruleset

啟用日期: 2022-07-08 16:33 最後更新: 2022-12-21 17:41

回報者:
負責人:
類型:
狀態:
關閉
元件:
里程碑:
優先權:
5 - 中
嚴重程度:
5 - 中
處理結果:
修正
檔案:
5

細節

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.

Ticket History (3/22 Histories)

2022-07-08 16:33 Updated by: cazfi
  • New Ticket "Support action enablers in actions.ruleset" created
2022-07-17 04:34 Updated by: dark-ether
評語

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?

(Edited, 2022-07-17 07:28 Updated by: dark-ether)
2022-07-18 17:39 Updated by: 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.

2022-07-19 04:32 Updated by: dark-ether
評語

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

2022-07-20 18:03 Updated by: 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

2022-07-20 23:08 Updated by: dark-ether
評語

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.

2022-07-21 06:33 Updated by: cazfi
評語

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

2022-07-22 07:57 Updated by: dark-ether
評語

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

2022-07-22 15:21 Updated by: cazfi
  • 負責人 Update from (無) to cazfi
  • 處理結果 Update from to Accepted
2022-07-22 15:51 Updated by: cazfi
評語

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.

2022-08-01 12:04 Updated by: cazfi
評語

Reply To cazfi

Having both this and #45065 applied, ./tests/rulesets_save.sh fails.

Can you investigate which one of the patches is in fault?

2022-12-05 05:33 Updated by: cazfi
評語

I started looking at what's the problem, but immediately noticed #46196 that needs to be cleared first.

2022-12-17 04:32 Updated by: cazfi
評語

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.

2022-12-19 02:21 Updated by: cazfi
評語

Testing with #45065 reveals some issue with "paradrop_to_transport"

2022-12-19 02:21 Updated by: cazfi
  • 處理結果 Update from Accepted to
2022-12-19 11:48 Updated by: cazfi
  • 處理結果 Update from to Accepted
評語

"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?)

2022-12-21 17:41 Updated by: cazfi
  • 狀態 Update from 開啟 to 關閉
  • 處理結果 Update from Accepted to 修正

Attachment File List

編輯

Please login to add comment to this ticket » 登入