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.
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.
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.
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?
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.
alienvalkyrie 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.)
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.
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)
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.
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)
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.
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;
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)
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.
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.
Now should work properly.
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.
Rebased and a bit reworded the commit. Likely, #44936 does not need rebasing.
If you switch a technology to one not researched at turn start and then give_bulbs method provides you with some bulbs,
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.