Freeciv

Heads up: If this ends up requiring changes to generate_packets.py, those will have to wait until after (the relevant part of) #45206 is done.

Also, if those changes have to do with an edge case in the non-delta versions of diff arrays (which it probably doesn't, realistically speaking), that'll get fixed along the way by itself (keeping the problem would require extra work).

Reproducible with desktop server + client configured with "--enable-debug --enable-json --disable-delta-protocol"

No matter which direction you try to move units, they always move up (direction 0, I think)

You think this is something you could debug while working on generate_packets.py & related stuff, alienvalkyrie?

I'll take a look at it, tho unless it ends up being a sufficiently obvious error in the generation script, I'll likely have to take some time to familiarize myself with the dataio_json internals.

The problem is actually larger - the entire unit_order struct is zeroed, not just the dir; it just so happens that (enum unit_orders)(0) is ORDER_MOVE, so the unit still moves, and that doesn't care about any of the other fields. Generated code looks clean, and so does dio_get_unit_order_json() ~> error lies deeper.

Likely culprit: plocation_read_elem() doesn't recurse properly. It does some sanity checks, and seems to recurse into nested arrays, but not into objects inside arrays – which is exactly the case in PACKET_UNIT_ORDERS. So we're trying to reinterpret the object as (in this case) an integer, and since json_integer_value() has no way of telling us about an error, we just silently get 0.

Note that the other dio_get_<struct>_json() functions (that are actually used), i.e. requirement, worklist, and action probability, all sidestep this problem by grabbing the object and manually reading from it using dio_get_*_internal(), rather than temporarily setting a sub_location on the location it gets and using public-facing dio_get_*() functions.

Attached patch makes plocation_read_elem() function analogously to plocation_read_field() (i.e. rolls back that part); tested and it seems to fix the issue. It does get rid of the checks that were there before, but I'm not convinced those checks were really necessary / sensible – if they are, we should bring them back and mirror them in plocation_read_field().

With this "--enable-debug --enable-json" (delta protocol not disabled) game does not launch at all: "1: ERROR: Unable to get uint8 from location: global_advances"

Without the patch it suffered from the same problem as the deltaless json once you are in the game.

Reply To cazfi

With this "--enable-debug --enable-json" (delta protocol not disabled) game does not launch at all: "1: ERROR: Unable to get uint8 from location: global_advances" Without the patch it suffered from the same problem as the deltaless json once you are in the game.

That seems like it might be a different problem with writing diff-arrays. Do note that with --enable-json --disable-delta-protocol, the global_advances array will not have really been read at all – that's the edge case in non-delta versions of diff arrays that I mentioned before (which will be passively fixed by #45219).

Looks like JSON array-diff put produces [[index, value], [i,v], 255], when it should be [[index, value], [i, v], [255]], i.e. the terminating index 255 doesn't get wrapped in an array like it should. Since it seems like that was also introduced in hrm # 745593, should we fold that into this ticket as "fix regressions", or make it a separate one?

Decided to go the "fold both into one patch" route. Still need to test that this didn't break the non-delta-protocol again.