[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Follow-up Comment #14, bug #19029 (project freeciv): I plan to commit these latest patches (file #15321, file #15324, file #15325) this evening unless someone objects (or gets there first). ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Update of bug #19029 (project freeciv): Assigned to:None = jtn ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Update of bug #19029 (project freeciv): Status: Ready For Test = Fixed Open/Closed:Open = Closed ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Follow-up Comment #12, bug #19029 (project freeciv): (Need to think further about dropping unnecessary backwards compatibility for S2_4 and trunk before commit.) I'd commit it with savegame format version (2.4.0, 2.5.0) internal backward compatibility in place first. So for a while savegames created with latest freeciv would be usable with just recent freeciv. Later, when any recent freeciv can read the new-style known information, we can drop that compatibility saving, or rather move it to 2.4 - 2.3 savegame format compatibility functions. This *if* we retain ability to save in old formats. After fighting a while with gen-roads related savegame issues, I really wonder if being able to save in old format is so important feature that it's worth the huge maintenance work. How many people ever actually save in older format? ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Additional Item Attachment, bug #19029 (project freeciv): File name: known-24.sav.bz2 Size:34 KB ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Follow-up Comment #13, bug #19029 (project freeciv): Attached patches for S2_4 and trunk with backward compatibility measures (they're basically identical). This allows these versions to understand all 2.3.x savefiles while keeping a clean format for new savefiles. Using the savefile compat mechanisms, relative to the previous patch, these remove backward compatibility cruft from the core save/load functions (saving ~20 lines each). But they add ~5x that to loading, and ~2.5x that to saving. The latter can probably go one day, but the former will be around forever. It's a little hard to see this as a win... on the other hand, it means 2.4.x+ savefiles are noticeably smaller due to the new semantics of comment #11. * A general comment on the compatibility mechanism design: it is particularly hard to do complex transformations when loading secfiles, since standard functions can't be used as most data structures aren't set up (we don't even have map.xsize/ysize). It's hard to see how it could be otherwise while keeping the compat code away from the main code. Save compatibility is easier since we have the in-memory data structures (although there's a small chance of future disturbance of save compatibility code if the in-memory data structures change significantly, whereas ideally you'd want never to have to touch it again). The way I've done it is that from now on, S2_4/trunk kXX_ means the sane version, where before this patch it meant the broken version; kvbXX_ and knownv2 are not used at all. This means that old S2_4/trunk savegames with player indices less than 32 will load fine (because sane==broken in that case), but ones with =32 will silently break. As cazfi noted elsewhere, that case is probably rather theoretical. If anyone cares, such files can be recovered by saving them with saveversion=2.3.0 in an old S2_4/trunk build, then loading them into a new S2_4/trunk build. I've taken no measures to allow old trunk/S2_4 to load new savefiles, as that seems pointless. I've tested it every which way: starting with file #15322 and with a version that's been through a fixed S2_3, I've loaded that into a fixed S2_4, saved it back in 2.3 format and loaded it into broken and fixed S2_3s, both in the same S2_4 session and via a S2_4 savefile, which I also loaded into a fixed S2_4. In all cases I looked at the known info on the minimap for both players, and diffed/eyeballed the savefiles. Also I tested loading a new broken S2_4 savefile (file #15323) and verified that it did the right thing for player indices 32 when loaded into a fixed server. After fighting a while with gen-roads related savegame issues, I really wonder if being able to save in old format is so important feature that it's worth the huge maintenance work. How many people ever actually save in older format? I've also wondered why we've committed to maintaining that capability. What's the use case, beyond occasional convenience for developers? (But since it's there currently, this patch supports it -- it was not the hardest part by any means.) (file #15324, file #15325) ___ Additional Item Attachment: File name: S2_4-KnownSaveBitOrder-4.diff Size:12 KB File name: trunk-KnownSaveBitOrder-4.diff Size:12 KB ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Follow-up Comment #11, bug #19029 (project freeciv): Updated patch attached -- targeted at S2_3 but applies to all branches. (Need to think further about dropping unnecessary backwards compatibility for S2_4 and trunk before commit.) I've fixed a memory leak and moved to unsigned ints in more places (the existing bit-shifting with signed ints is not nice, although probably did the right thing in practice). I've added some comments acknowledging that the backwards compatibility mode deliberately overshifts (and thus invokes undefined behaviour). I'm sure some tool is going to castigate us for it at some future point, so a comment may save some head-scratching. Also, I've tweaked the semantics of the new kvb entries subtly, as suggested in comment #2: a halfbyte (covering 4 player slots) is only saved/loaded if at least one of those slots is actually in use. Previously, a game with 4 players would save/load map-known data for 8 full maps (32 player slots) -- 8x as much as needed. This should mitigate the increase in save time and save file size from this workaround, and reduce load times a bit (when loading new-format games). (I also said in that comment that the value of lines is too big, wasting memory, but I can't see what I meant now -- it looks fine to me.) Strictly, we could avoid bloating savefiles with the kvb stuff entirely for games of =40 player slots (i.e., most of them), since the existing k format is OK for such games. But the code for that would be nasty and would complicate removing backward compatibility in future, so shrug. A further possible refinement: for games with fog-of-war, it would be possible to use the information in the player maps instead, per comment #5. * Pro: for loading existing S2_3 saves, this would avoid relying on the potentially-platform-dependent undefined behaviour. * Pro: moving to only saving this information in one place in future savefile formats would decrease save time/size. * Con: this would mean that the global known save/load code would only be exercised in non-fogofwar games, which are not the default. So it could easily rot. * Con: player maps are loaded rather later than this known information. Messing around with the load sequence is fraught -- particularly on a stable branch -- due to complex dependencies between loading steps. Conclusion: probably not worth it, so I haven't tried to do this. Tested with S2_3 with a game with players in slots 0 and 125 (attached): loaded this old game into a new server, saved it, and loaded that into both a new server and an old server, checking that both players' known info is loaded OK and eyeballing the savefiles. (On x86_64; that doesn't guarantee that backward compatibility works as expected on Sparc, since we're talking about undefined behaviour here.) (file #15321, file #15322) ___ Additional Item Attachment: File name: KnownSaveBitOrder-4.diff Size:7 KB File name: idx-painted.sav.bz2Size:14 KB ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Follow-up Comment #10, bug #19029 (project freeciv): - Made p - l * 32 - p % 32 change requested in comment #1, with comment that should make difference between new bit order and old one in the previous lines clear. (file #15317) ___ Additional Item Attachment: File name: KnownSaveBitOrder-3.diff Size:3 KB ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Follow-up Comment #9, bug #19029 (project freeciv): - Changed prefix of new entries from kv2 to kvb. It's followed immediately by a number, and I don't want 2 to appear as first digit of that. (file #15226) ___ Additional Item Attachment: File name: KnownSaveBitOrder-2.diff Size:3 KB ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Update of bug #19029 (project freeciv): Status:None = Ready For Test ___ Follow-up Comment #8: Untested fix for S2_3 and S2_4. In those branches we have to save in a format that also earlier versions can load. In trunk we could (and eventually should) drop saving in old bit order. But we cannot commit this one to stable branches without trunk having at least same changes making it possible to load savegames from stable branches. Dropping saving in old bit order in trunk can wait for a new ticket, and this one should go in after testing in all three branches. (file #15201) ___ Additional Item Attachment: File name: KnownSaveBitOrder-S2_3.diffSize:3 KB ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #19029] Possible trouble saving/loading players' known territory for player indices =40
Update of bug #19029 (project freeciv): Summary: Trouble saving/loading players' known territory for player indices =40 = Possible trouble saving/loading players' known territory for player indices =40 ___ Reply to this item at: http://gna.org/bugs/?19029 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
