Adds a per-level persistence system.#5176
Conversation
1802990 to
1dd8215
Compare
|
Depends on #5175 |
| // Is this actually desirable? People moving around or modifying | ||
| // atoms across save could result in inconsistent data. | ||
| set waitfor = FALSE |
There was a problem hiding this comment.
i think you'd have to actually add if(_persistent_save_running) return hooks into move and other client-driven/verb things to make it block anyway, is the thing. i'm not sure waitfor = TRUE would be enough. so it'd only be a partial solution either way
There was a problem hiding this comment.
We could potentially suspend all subsystems and kick people back to the lobby during saves, maybe? Similar to how DMMS suspends fluids and such until the run finishes.
1dd8215 to
4d406a4
Compare
4d406a4 to
96e2512
Compare
|
I'm going to call The atom side of this appears to be working fine, I've tested with a debug item and it all seems pretty good. EDIT: Went ahead and just let it instance datums with the serde data as a list arg, in case it's needed in the future. |
aa31be7 to
2862dd3
Compare
|
Slipped and accidentally added turf serde, limited testing on Shaded Hills but needs proper testing before I sign off on it. |
|
Depends on #5182 |
|
Testing notes:
|
f491ffe to
3a905e3
Compare
| // persistent_data_location = "data/level_data" | ||
| VAR_PROTECTED/persistent_data_location | ||
| /// Decl handler, mostly forcing myself to keep this general so it can be optimized with a DB or something down the track. | ||
| VAR_PRIVATE/persistence_handler = /decl/serialization_handler/json |
There was a problem hiding this comment.
Similarly as my last comment, shouldn't the way the level is saved really be stored in the level_data? Because, it doesn't really make sense to save each levels with a different serializer. It should be something decided at the start of the serialization imo.
There was a problem hiding this comment.
The serialization logic doesn't really care where the data comes from, once it's in DM it's all in the same format. I don't really think it matters much. The alternative would be forcing the entire game to use the same handler, which to me limits the ability for people to mix and match or run their own local changes alongside general changes from upstream.
What would the benefit be to forcing a single handler for all cases?
There was a problem hiding this comment.
i could see using a different handler for, specifically, custom ships vs a main map
There was a problem hiding this comment.
The serialization logic doesn't really care where the data comes from, once it's in DM it's all in the same format. I don't really think it matters much. The alternative would be forcing the entire game to use the same handler, which to me limits the ability for people to mix and match or run their own local changes alongside general changes from upstream.
Yeah, but why should levels each individually chose to save in a different format? I don't really see a point to that? Because, you really want the same kind of data to be in the same format for coherence's sake and maintainability.
Also, what if you host a server, and you used json to store the save, but now your setup changed and you wanna use a DB. You'd need to change every single levels to the db handler and recompile. While, if you'd use all the same handler for levels you could easily make it a config entry, or call the serialization proc with a different handler specified and not have to mess with the code or do mass replace and risk making mistakes.
Unless I really don't understand what you mean/intend to do here?
What would the benefit be to forcing a single handler for all cases?
It would be decided at the time the serialization proc is called ideally. And it would be less error prone, much more predictable, easier to maintain, simpler to back up, etc.. It would also allow for consolidation of saved level data later on.
There was a problem hiding this comment.
i could see using a different handler for, specifically, custom ships vs a main map
Yeah, but, depending on why you'd want a different handler, you probably would need extra metadata or extra processing for saving something to be used separately from the main save. Especially with anything multi-z.
Like, for example, I'll assume what you're thinking of is to move a ship and crew to another server via export or something else.
You'd need to make substantial changes to do that, and probably need a separate mechanism called on demand specifically for saving and moving ships and sending the save.
It would be less of a bother to just specify what handler to use when launching serialization, and what you want to serialize rather than making sure all ships being transferred have the right handler set and figure out what should even be done in the case they have the wrong handler?
There was a problem hiding this comment.
I am getting increasingly confused by some of these comments. Having the ability to specify handler per level really doesn't impose any overhead or constraints that you wouldn't have by having it as a param or such. If you decide to change your data format you have a lot more problems than changing a handful of compile time values (such as having to manually migrate all your data).
Making it a config value or such just means there's no capacity whatsoever to have variance. If I wanted to save custom ships to JSON so players could save and load their layout privately, that wouldn't be possible. If I wanted to have large level serde handled by a DB for performance but some other aspect handled by JSON for some other reason, that wouldn't be possible.
If you don't want to use multiple handlers, don't change the value ever and you're golden. I really cannot see a benefit to centralising and restricting things like you seem to be asking for here and earlier.
| if(populate) | ||
| // If preloaded from serde, handle expected list structure. | ||
| // Returns if preload is successful to skip populate_reagents() call. | ||
| FINALIZE_REAGENTS_SERDE_AND_RETURN(reagents) |
There was a problem hiding this comment.
think occasionally update_alpha_mask on the fluid overlay can run before this does, which is a little concerning. led to runtimes locally
There was a problem hiding this comment.
Should be fixable by istype(neighbor.reagents) in that proc
7d6d697 to
56e0c1b
Compare
|
I'm not really sure what to do about randomly generated levels. I might pull the reordering logic out of this PR and just prevent placing more mobs if we deserialized. Otherwise we end up with mask turfs, etc. left on the map. |
should we not just automatically mark those turfs as serialized? |
56e0c1b to
47f36b1
Compare
|
Current outstanding issue with this one is that I cannot for the life of me seem to get map templates placed during init to flag the turfs as changed. Generating on Shaded Hills and restarting leaves template-shaped sections of mask turfs. Not sure what the issue is, it happened even after I made all ChangeTurf() calls and relevant sections of DMMS flag the turfs directly. I am assuming that something is newing /turf directly and not flagging. |
does |
Genuinely no idea why I didn't think of this. Thank you :( |
|
This is now working as far as I can tell. Main things I am worried about in live testing is accidentally cooking legacy persistence data, but it should at least write out a backup before it does any kind of migration of that data. I am not committing any changes that actually use this system in full yet, I'm going to save that for after we've done playtesting (probably via Pyrelight or maybe Scav) |
|
Realised belatedly that I need to get area serde in before this will work as intended, due to map templates setting area, but I will do that in a second PR, I don't want to fiddle with this one further currently. |
out-of-phaze
left a comment
There was a problem hiding this comment.
i frankly really don't like that we still have the legacy persistence system, it really doesn't work for anything but papers and dirt (the generic 'filth' stuff was extremely annoying). but i guess some maps want a middle-ground re: persistence.
once this is in i'll try to make it faster
| /// Used to avoid unnecessary refstring creation in Destroy(). | ||
| var/tmp/has_state_machine = FALSE | ||
| /// Var for holding a unique-to-this-run identifier for a serialized datum. | ||
| VAR_PRIVATE/__run_uid |
There was a problem hiding this comment.
this could use /tmp/ possibly
There was a problem hiding this comment.
Yes please. I really wish /tmp was used more. It helps make transient vars a whole lot more obvious.
out-of-phaze
left a comment
There was a problem hiding this comment.
was approved prior to conflict resolution, has conflicts again (my bad), if you fix conflicts i'll merge
Description of changes
/atom/Serialize()to return an assoc list of relevant values suitable for saving out./atom/Preload()and/atom/PreloadData()and hooks in SSatoms flush to deserialize atoms during flush./datum/level_data/load_persistent_data()and/datum/level_data/save_persistent_data()as entrypoints for per-level persistence.General flow of level persistence:
TODO
Implement area serde.Test level generator reordering (split into 2nd PR?)Reverted these changes.Future work
Completely integrateended up largely doing this in this PR./decl/persistence_handlerinto this system instead of the bespoke serde on SSpersistence.Why and what will this PR improve
Adds a framework for handling persistence in the future.
Authorship
Myself.
Changelog
Nothing player-facing.