Update eventSyncRequest() docs, and x,y params -> tile units

[aubergineagain]

The docs for eventSyncRequest() did not specify the first parameter (fromPlayerId), or the units used for x,y params, and incorrectly stated that the object params were id when they are actual game objects (or null if not defined).

Additionally, the x,y params were not being converted back to tile units before being passed to the JS API.

I've checked every mod and script I can find and haven't found a single place where anyone was using this event, most likely because the original docs were super-confusing.

EDIT: See discussion below, it was being used, but in strange ways.

Fixes #4738
Fixes #4737
Fixes #3193

[aco4]

// Convert x,y from world coordinates to tile coordinates
int tile_x = map_coord(x);
int tile_y = map_coord(y);
...
instance->handle_eventSyncRequest(from, req_id, tile_x, tile_y, psObj, psObj2);

I like the documentation changes but I don't understand why this breaking change is being made? It seems completely unnecessary and will break all existing implementations of syncRequest and eventSyncRequest, which do exist both privately and publicly. Some public examples here: https://forums.wz2100.net/viewtopic.php?t=16829 and https://forums.wz2100.net/viewtopic.php?t=16776

[aubergineagain]

I first learnt about the issues with the x,y being in world units on the event handler side from wanjia (who's posts you reference above). Their type script hints explains the problems with the current setup:

bugfix:
fixed return type of special cases
fixed markdown document of eventSyncRequest:

at version 4.4.2, if player 0 calls syncRequest(1,2,3)
the expected event is eventSyncRequest(1,2,3,null,null)
but, in fact it is eventSyncRequest(0,1,256,384,null,null), different to document.
argument 0 is "from" player, other arguments is shifted right. x and y are multiplied by 128.

That post also references an issue with similar report: #3193 (now added to Fixes list in OP of this PR)

As far as I can tell, only wanjia has ever done anything with syncRequest() and eventSyncRequest() (my searches didn't pick up the zip files in the forums), and they are both extremely fringe situations. Serializing strings via the x int argument is a huge kludge and bloats network traffic, as wanjia notes in the mod's forum post:

its implement is inefficient. theoretically, a syncRequest can carry 24 bytes of data (req_id, x, y of type number in float64. object and object2 have constraints and unable to carry data),
but the implement only use the req_id to carry data, the charcode of every character of the json serialized argument, which take 1 byte at most cases.
the x is also used. it carry the position of charcode. because it's not sure if the eventSyncRequest called in order.
it also have no validation. it assumes there is no packet loss or errors, cheating. validation should be implemented in the function that to be synchronized.

Maybe we should add a new, optional, string parameter so it's much easier to just send a string in a single packet?

For example:

const str = JSON.stringify(someObj);
syncRequest(x, y, null, null, str);

//...

function eventSyncRequest(from, x, y, o1, o2, str)
{
    const text = JSON.parse(str);
}

[aubergineagain]

I couldn't find if wanjia had a github acct so posted in linked forum posts asking for feedback on breaking change to the x/y params.

[aubergineagain]

@aco4 Are you using syncRequest in similar way to wanjia1's mods (encoding abstract data in to the integer params)?

If so, would it be better to add a new function/event pair: syncString(str) -> GAME_SYNC_REQUEST_STRING -> eventSyncString(fromId, str) so that mods could have a few versions to port to that prior to fixing up the eventSyncRequest() ?

[aco4]

@aubergineagain I am indeed. Yes, syncString() is a great idea.

About "fixing" eventSyncRequest() — I really don't see any problem with its current form. The documentation was wrong, but once that is updated, the event will work without issue. Why change the API for something so trivial?

[aubergineagain]

Added PR for syncString() / eventSyncString(): #4821

As for fixing eventSyncRequest(), it's just removal of technical debt. Everyone who touched that event mentioned the same issues, including wanjia1 in the forums. It's not even being used in the way it was designed to be used.