Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor map loading & saving #5572

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

ElectroJr
Copy link
Member

@ElectroJr ElectroJr commented Dec 23, 2024

This PR reworks and tries to generalize entity serialization (i.e., map saving & loading). IMO the main shortcomings of the current system are that only files containing either a single root map or grid are properly supported and that there is no real support for null-space entities. We also need better multi-map support for full game saves. The current setup is also prone to introducing bugs due to the lack of distinction between whether a file is a "grid" or "map". This can lead people accidentally saving/loading maps as grids and vice versa, which has caused game breaking bugs that can remain undetected until a problematic file is loaded improperly.

This is a revival of a 1+ year old draft, and has had quite a few conflicts. I've tried to clean up any issues but I also wouldn't be too surprised if I missed something or accidentally reverted some serialization changes/bugfixes.

Requires space-wizards/space-station-14#34020 .

Overview of changes:

  • The server-side MapLoaderSystem and associated classes & structs have been moved to shared to support single player games and serialization of client-side entities.
  • Most of the serialization logic and methods have been moved out of the system and into new EntitySerializer and EntityDeserializer classes.
    • The system is effectively a wrapper around the new serializers, and provides specific methods for saving/loading grids & maps.
    • Other systems can and should use these classes directly if they need more control over serialization.
    • They can also be used to help inspect/read a map file. E.g., if you just want to figure out how many grids are in a file without spawning entities, though it still needs to parse the whole yaml file into a DataNode
  • The MapLoadOptions class has been replaced with MapLoadOptions, SerializationOptions, and DeserializationOptions structs
  • MapLoaderSystem's TryLoad() and Save() methods have been replaced with grid, map, generic entity variants. I.e, TrySaveGrid, TrySaveMap, TrySaveEntity, and TrySaveGeneric().
    • This is meant to encourage people to be more specific about the type of file they want to save/load, in an attempt to help prevent bugs around loading a grid as a map and vice versa.
    • Unlike before, you are no longer expected to create a dummy/placeholder map before calling TryLoad, and should instead use the various methods and their option structs to configure things like whether the map should be initialized or paused.
    • This hopefully also helps makes some of the deserialization logic clearer. and removes the sphagetti that was SwapRootNode().
  • The serializer classes replace the old MapSerializationContext.

File format changes

  • The created files now serialize postMapInit and paused information per-entity, instead of there being a single global "postMapInit" field.
    • This is needed to support files containing multiple maps & null-space entities
    • This shouldn't change pre-init maps, but will generally increase the size of any post-init files by 1-2 lines per entity.
  • Extra metadata has been added, including engine/fork version & fork id, and a new "category" enum, which specifies whether the file contains a single map, grid, entity, full-save, or something else.
    • If I'm not sure how useful the fork fields will be, seeing as mapping or re-saving maps with a local build will leave them empty, but might as well add them to try make it easier to track down a version of the game that can load a given file.
  • The yaml ids of maps, grids, and "orphaned" entities are now listed separately before entities.
    • The map & grid fields mainly just exist to make it easier to parse & inspect a file, but could be re-constructed by just parsing the entity information.
    • The "orphaned" list contains all entities that were serialized without their parent, and should be attached to a new parent while deserializing. This list is needed to avoid confusion with "true" null-space entities when serializing things like grids.
  • The file format version has changed from 6 to 7, and the minimum supported version was increased to 3.

Serialization auto-inclusion

Currently if an entity gets serialized, but has some component that references an EntityUid that is not being serialized, it will log an error and might lead to broken saves. The new default behaviour is that if the referenced entity is a non-map null-space entity, it will automatically include that entity in the save, otherwise it still logs an error.

This is intended to make it easier when saving a map/grid with entities that depends on null-space entities. The actual behaviour can be configured to also auto-include any referenced entity. So for example, saving a map that references a station or rule, which in turn references another map, would result in the file containing both maps.

Other Changes

  • Some more MapManager methods have been moved to the system or marked as obsolete.
  • Re-using a MapId will now log a warning. Ideally this shouldn't happen aside from when map ids are manually specified by the load map commands.
    when warnings are logged.
  • IServerEntityManagerInternal has been removed. AFAICT it was just being used by entity serialization, and was internal anyways.
  • MapChangedEvent has been marked as obsolete, and should be replaced with MapCreatedEvent and `MapRemovedEvent.
  • ITileDefinitionManager.AssignAlias and general tile alias functionality has been removed. TileAliasPrototype still exist, but is only used during entity deserialization. This and related serialization changes fixes Tile Migration is not working #5565
  • IEntitySystemManager now exposes the system IDependencyCollection, mainly so that you can use InjectDependencies(), which is used by the new serializer classes to resolve system dependencies.
  • BeforeSaveEvent has been replaced with BeforeSerializationEvent, and there is a new AfterSerializationEvent.

@ElectroJr ElectroJr added Area: Serialization Status: Requires Content PR This PR breaks content and requires both to be merged together. Status: Needs Review This PR needs to be reviewed before it can be merged. labels Dec 23, 2024
@metalgearsloth
Copy link
Contributor

shell yeah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Serialization Status: Needs Review This PR needs to be reviewed before it can be merged. Status: Requires Content PR This PR breaks content and requires both to be merged together.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tile Migration is not working
2 participants