Personal bests history

[unnecessarilycomplex]

Hello Drek! Here's my temptative implementation for #61 (saving stats at every PB).

What do you think? I'm new to Openplanet/TM plugins so I'm not sure that I haven't f**ked up somewhere. Let me know if my code is not understandable... It does what's proposed in the Issue tracker.

I've tested it on the Openplanet School maps and I made it work on my side, hopefully it will work right away for you too. You can uncomment debug_print() in the new component if you want the logs.

A few specific points :

• It seems like I'm occasionally pulling some stale values from Map_GetRecord_v2() when checking it after a finish. Then the new time is "discovered" only at the next finish, which is a decent issue in terms of functionality. I haven't really looked into it ; have you run into this problem before?
• I've added an "unmonitored" qualifier for PBs that were not recorded when they were established, as it is the case on maps that have preexisting grind data and PBs.
• It currently only works with TMNEXT (it shouldn't crash, but the PB history will remain empty) ; adding the others to the current code should be straightforward
• At the moment I'm using the Json classes more than what the existing code does, this can change
• The logs show that there's occasionally multiple instances of the component running in parallel and detecting a new finish at the same time. I've done a bit of testing and I couldn't figure out where the rogue instances come from. It might have to do with the repeated plugin reloading ; honestly I don't know. They don't seem to interfere at all with what the "main" instance is doing though.

Cheers,

Nico

p.s. here's a sample savefile:

{
  "medals":"[{\"medal\":0,\"achieved\":false,\"achieved_time\":\"          0\"},{\"medal\":1,\"achieved\":false,\"achieved_time\":\"          0\"},{\"medal\":2,\"achieved\":false,\"achieved_time\":\"          0\"},{\"medal\":3,\"achieved\":false,\"achieved_time\":\"          0\"}]",
  "personal_bests":[
    {"finishes":2,"time_played":64911,"resets":7,"achieved_time":24652,"respawns":0},
    {"unmonitored":true,"finishes":1,"time_played":28137,"resets":5,"achieved_time":25133,"respawns":0}],
  "time":"      67912","finishes":"     2","resets":"     8","respawns":"     0"
}

[Fort-TM]

I can't speak for the rest of the code, but I find it weird to only create the "unmonitored" key if the value is true and vice versa. Feels a lot simpler to include the value regardless and read the bool afterwards

Same with doing uint64(double(value)) for the JSON values, I'm pretty sure there's no need to convert it to a double first

[unnecessarilycomplex]

I used uint64(double()) because Openplanet's Json only implements conversion to int (which is int32) rather than to int64. I wasn't sure if the current use of strings in the savefiles is to avoid overflow issues. Going through double is ~ similar to an int53 conversion

I understand what you mean for unmonitored. The idea is to just flag unreliable records in the special case when the GrindingStats PB tracking is incomplete and the current public PB doesn't match the PBs known to the plugin -- including all the savefiles predating the feature. But JSON requires values for all keys... we could also use null. I'm a bit reluctant to write "no exceptions here" everywhere. Functionally it makes no difference though

[unnecessarilycomplex]

I maybe have an idea regarding the "unmonitored" issue that Fort raised — how would you feel about witnessed = true/false ?

[drekdrek]

all uint64s should stored as strings due to the fact that the json implementation in OP abbreviates when it can into scientific notation (ie. 1.51e13) and this leads to a loss of accuracy.

[drekdrek]

Suggested change

	void debug_print(string s) {
	void debug_print(const string &in s) {

C:/users/steamuser/OpenplanetNext/Plugins/GrindingStats/src/Components/PersonalBests.as (148, 2) : WARN : Sanity check: Use 'const string &in s' to pass a string by reference (prefix the parameter name with an underscore to ignore this warning)

[drekdrek]

currently the handler routine is not stopping when i leave a map, im not sure why yet.

[unnecessarilycomplex]

Okay, I have implemented the changes discussed above.

currently the handler routine is not stopping when i leave a map, im not sure why yet.

I'm not sure I understand ; do you mean generally, or did you see that behavior with the PersonalBests handler specifically ?

[drekdrek]

the personal bests handler in specific

[unnecessarilycomplex]

I see. I'll try to look into it. How could you tell it's not stopping ?

[drekdrek]

in the debug menu of openplanet if you expand grinding stats you can see the list of all running coroutines

[unnecessarilycomplex]

Okay, this drove me nuts; I didn't take the problem by the right end and I mistakenly went down a rabbit hole of pointer shenanigans and why-isn't-my-destructor-called, but I've found the cause.

Actually the while(running) condition in the handlers doesn't do anything ; for any of the components. I'm not too sure how destruction is triggered in Openplanet Angelscript (perhaps destruction simply doesn't happen right away after the pointer isn't referenced, or the pointers to the components are copied and still referenced by other parts of the plugin like in render/recap), but AFAIK none of the destructors are reliably called so we pretty much never get running = false;.

The practical reason why all the other handlers do stop is because of the line if (map is null) return; inside the while loop (note how this differs from if(map is null) continue;). Effectively, the handler loop is while(true) if (GetApp().Rootmap is null) break;, meaning they stop when you return to the game menu.

For PersonalBests I had moved the bulk of the handler code to the check_for_finish() subfunction to get an early-return pattern, so I was getting the continue behavior instead of break.

In fact, none of the hanlders stop if you don't return to the game menu. For instance if you're in a campaign and hit the "next" button (but presumably also if you're on a map-rotation server), all the hanlders of the earlier maps you've played are likely still running. They're not referenced in the active DataManager object though so they don't create data corruption.

Actually while investigating the rogue instances last week, I was quite confused about running and I had started fiddling with the code... I have this commit (that isn't in the present pull request) unnecessarilycomplex@cb398f7. I think the clean solution is to let the map_handler() control the components' running flags through calls to start() and stop() when the map changes. I can create a new clean branch and make a separate pull request if you would like.

[drekdrek]

hmm, yeah cleaning up the handlers should be a separate PR, and should be merged before this one

[drekdrek]

i'd be happy to port these changes over to MP4 and TURBO after everything is how we want it

[unnecessarilycomplex]

hmm, yeah cleaning up the handlers should be a separate PR, and should be merged before this one

Sounds good. I'll draft up a separate branch

i'd be happy to port these changes over to MP4 and TURBO after everything is how we want it

That would be awesome :) I haven't played the previous games yet but it's really cool to see them live on despite tm2020

[unnecessarilycomplex]

I added a sleep(1000) before the call to Map_GetRecord_v2() to avoid pulling stale values from the API, which AFAIK was the last issue that hadn't been addressed

I'll see what I can do for the handlers and I'll come back here afterwards