Ticket #45685

Bulbs given during the turn behave oddly on switch

Eröffnet am: 2022-09-23 05:00 Letztes Update: 2022-10-18 22:05

Auswertung:
Verantwortlicher:
Typ:
Status:
Geschlossen
Komponente:
Meilenstein:
Priorität:
5 - Mittel
Schweregrad:
5 - Mittel
Lösung:
Gefixt
Datei:
5

Details

If you switch a technology to one not researched at turn start and then give_bulbs method provides you with some bulbs,

  • if you switch between non-saved techs, the added bulbs keep (not in the "got tech" case)
  • if you switch to the saved tech, you get the saved bulbs without addition
  • if you then switch to any other tech, the addition is lost.

UPD: The same happens with bulbs added by a caravan. Recategorizing the ticket from Scripting to Server. If you agree that it's a bug and not a feature, probably we should change the milestone to 3.0.x.

Maybe the solution would be separate stocks for free and non-free to switch bulbs, like we have for city shields.

Blocks #44936.

Ticket-Verlauf (3/30 Historien)

2022-09-23 05:00 Aktualisiert von: ihnatus
  • New Ticket "Lua: (Player):give_bulbs() given bulbs behave oddly on switch" created
2022-09-23 05:40 Aktualisiert von: ihnatus
  • Komponente Update from Scripting API to Server
  • Details Updated
  • Summary Updated
2022-09-24 01:31 Aktualisiert von: ihnatus
Kommentar

Likely, for d3f'd branches all what can be done is adding bonus bulbs also to saved bulbs and warning the players that caravan bulbs will be penalized on switching from a saved tech even if originally got towards another one. Clumsy, but people used to live with it and we have no place to save another bulbs.

2022-09-24 02:01 Aktualisiert von: cazfi
Kommentar

Patching d3f branches just as much we can is ok. This isn't a regression, after all. The important thing is to make it properly for S3_2-d3f.

2022-09-27 07:20 Aktualisiert von: ihnatus
Kommentar

Have not tested much but I thought about something like this for master. Maybe needs something more to write in savegame3.c and should have dff version up. Here, we abandon got_tech and got_tech_multi, instead we put teansferable (excess and bonus) bulbs into a separate counter.

2022-09-28 05:34 Aktualisiert von: ihnatus
Kommentar

Made a patch for 3.1 (probably works also for 3.0). That handles switching to saved tech after a caravan gets bonus (but often not from it). Maybe some documentation can be changed to tell about this effect?

2022-09-28 05:55 Aktualisiert von: cazfi
Kommentar

Reply To ihnatus

Made a patch for 3.1 (probably works also for 3.0). That handles switching to saved tech after a caravan gets bonus (but often not from it). Maybe some documentation can be changed to tell about this effect?

Haven't had time to look at this yet (sorry, just too many high priority things going on), but if that moves to some grey area of what is the right behavior and might change things from what people are used to / expect, this might be something you want opinions from forum also? People are complaining that they are not let to be involved when things move forward in development channels only.

Also, if it's any way relevant, civ/2 compatibility should be considered, for civ1 / civ2 rulesets.

2022-09-29 03:24 Aktualisiert von: alienvalkyrie
Kommentar

Without having looked at the patch in too much detail; how does it behave in multiresearch games / has that been considered? As in; are free_bulbs still used there, leaving some buffer that can be transferred between techs; or are all bulbs just directly, permanently put into whatever tech is being researched at the moment?
(I'm frankly not sure if either of these is necessarily better than the other, but either way I'd rather it be a conscious decision that's documented in the game rules.)

2022-09-30 10:16 Aktualisiert von: cazfi
Kommentar

Reply To ihnatus

Made a patch for 3.1 (probably works also for 3.0).

So targeting to 3.0.5

2022-10-06 09:09 Aktualisiert von: cazfi
Kommentar

Looked at the master patch. Looks good way to do it.

Just the handling of older savegames seem wrong. savegame3.c *requires* that the new field is there, and then would additionally check the old value. Basically savegame3.c should just require the new value, after a 3.1 -> 3.2 conversion ( compat_load_030200() ) in savecompat.c should turn the old style value to a new style value. Same for dev-save-compat in compat_load_dev() ('if (game_version < 3019300) {' block in the end) - in most cases its little more than copy&paste from the conversion between stable versions, and not that critical - it's going to get discarded before a release anyway.

2022-10-08 20:18 Aktualisiert von: cazfi
Kommentar

Patch for stable branches look good to me, applies cleanly (to both branches), and compiles.

Will add it to the next autogame runs of those branches. (I already have master branch in an autogame testing, even with the known issue of the savecompat)

2022-10-11 04:55 Aktualisiert von: ihnatus
Kommentar

Reply To cazfi

Just the handling of older savegames seem wrong. savegame3.c *requires* that the new field is there, and then would additionally check the old value. Basically savegame3.c should just require the new value, after a 3.1 -> 3.2 conversion ( compat_load_030200() ) in savecompat.c should turn the old style value to a new style value. Same for dev-save-compat in compat_load_dev() ('if (game_version < 3019300) {' block in the end) - in most cases its little more than copy&paste from the conversion between stable versions, and not that critical - it's going to get discarded before a release anyway.

Done something like what you've said. I'm not great in understanding savegame compatibility.

2022-10-11 04:56 Aktualisiert von: ihnatus
  • File 3.2-split_research_bulbs.patch (File ID: 10550) is attached
2022-10-11 09:36 Aktualisiert von: cazfi
Kommentar

Reply To ihnatus

Done something like what you've said. I'm not great in understanding savegame compatibility.

It's already close to what we want. Just a couple of issues:
- You currently insert the entry within the "if (... "got_tech" ...)" -block. Don't you want it also when !got_tech, but got_tech_multi? Thought we've never had a *stable* format properly saving got_tech_multi.
- In compat_load_dev() you are adding a new "if (game_version < 3019300)" -block *within* a "if (game_version < 3019200)" -block, so it's actually never run for savegames from 3.1.91. There already is proper "if (game_version < 3019300)" -block in the end of the function. You should extend that one
- "if(secfile_lookup_bool(loading->file, &got_tech," - missing space between "if" and "(", on both places where it occurs (conversion from stable format, conversion from development format)

2022-10-13 04:25 Aktualisiert von: ihnatus
  • File 3.2-split_research_bulbs.patch (File ID: 10550) is deleted
2022-10-13 04:25 Aktualisiert von: ihnatus
Kommentar

Reply To cazfi

- You currently insert the entry within the "if (... "got_tech" ...)" -block. Don't you want it also when !got_tech, but got_tech_multi? Thought we've never had a *stable* format properly saving got_tech_multi.

Like, "got_tech" should have always been saved. If we don't have it, the savefile is supposedly broken to start from, and we are indicating it via leaving no "free_bulbs". If it's a wrong way, please tell or do yourself a right one.

2022-10-14 05:59 Aktualisiert von: ihnatus
Kommentar

Bad. In multiresearch mode, free bulbs should not be given when there is any specific tech in research, or we get the bulbs doubled to some another tech if we switch. (That's current mechanics, we likely are not going to change it.)

By the way, what's this, just a check for a valid advance number?

advance_index_iterate(A_FIRST, j) {
    if (j == research->researching) {
      research->inventions[j].bulbs_researched_saved = research->bulbs_researched;
    }
  } advance_index_iterate_end;

(Edited, 2022-10-14 06:02 Aktualisiert von: ihnatus)
2022-10-14 19:07 Aktualisiert von: cazfi
Kommentar

Reply To ihnatus

Bad. In multiresearch mode, free bulbs should not be given when there is any specific tech in research, or we get the bulbs doubled to some another tech if we switch. (That's current mechanics, we likely are not going to change it.)

I don't quite get the complete idea of what you are saying.

Until going to details, let's see more immediate consequences:
Do you think that:
a) Your patches for all branches need fixing (nothing can be pushed)
b) Your patch for master need fixing for this part too (we could push the bug fix patch to all branches, open a new ticket about more complete master fix)
c) There's no regression (just noted existing flaw), and no fixes to patches are coming (all can be pushed, short of need to recheck the savecompat thing)

2022-10-14 21:45 Aktualisiert von: cazfi
Kommentar

Reply To cazfi

Until going to details, let's see more immediate consequences:

To open the reasons a bit: We've released 3.0.4 a week ago, i.e., we are in the early phase of a new release cycle. Now would be the best possible time to get S3_0 patch in, so that it would be exposed to testing for the maximum time before it gets released as part of 3.0.5.

2022-10-15 01:29 Aktualisiert von: ihnatus
Kommentar

Reply To cazfi

I don't quite get the complete idea of what you are saying.

It's only about the system introduced in 3.2 patch. That is not appropriate but I'll try to fix it in few hours.

2022-10-15 03:30 Aktualisiert von: ihnatus
Kommentar

Now should work properly.

2022-10-15 09:38 Aktualisiert von: cazfi
Kommentar

Reply To ihnatus

Reply To cazfi

- You currently insert the entry within the "if (... "got_tech" ...)" -block. Don't you want it also when !got_tech, but got_tech_multi? Thought we've never had a *stable* format properly saving got_tech_multi.

Like, "got_tech" should have always been saved. If we don't have it, the savefile is supposedly broken to start from, and we are indicating it via leaving no "free_bulbs". If it's a wrong way, please tell or do yourself a right one.

Ok, my mistake. I missed that one of the calls was secfile_lookup_bool_default() and the other was a secfile_lookup_bool(), and the return value semantics are completely different between those.

But the patch does not apply - likely just not rebased after #45671 went in a couple of days ago.

2022-10-17 03:54 Aktualisiert von: ihnatus
Kommentar

Rebased and a bit reworded the commit. Likely, #44936 does not need rebasing.

2022-10-17 04:53 Aktualisiert von: cazfi
  • Verantwortlicher Update from (Keine) to cazfi
  • Lösung Update from Keine to Accepted
2022-10-18 22:05 Aktualisiert von: cazfi
  • Status Update from Offen to Geschlossen
  • Lösung Update from Accepted to Gefixt

Dateianhangliste

Bearbeiten

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Anmelden