Skip to content
This repository has been archived by the owner on Jul 30, 2020. It is now read-only.

Multi-scene support for storage systems #201

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

Conversation

SugoiDev
Copy link
Collaborator

This is a initial working version of a multi-scene storage system.
FI's backup and storage systems are broken with multi-scene. This PR brings initial support for multi-scene setups.

I would appreciate some code review and ideas for dealing with Unity's cross-scene weirdness, if anyone has know-how in that area.

Warning

  • This pull-request is not considered ready, and should not be merged to master. The underlying systems seem to be working under my limited testing, but it could have edge cases that aren't account for. There's also rough spots (see below) with Unity's cross-scene references.

  • Those changes are probably not compatible with older Unity versions. I only tested on Unity 5.5.2.

Overview of changes

  • abstracted the ideas for storages that were used only for the backups system into a new abstract type fiBaseStorageComponent<T>. This object has methods to refresh the storage references, as well as create new ones if they become stale or corrupted. It does not perform cleanup based on name.

  • fiStorageManager in the BackupService has a member of that type, as well as fiPersistentEditorStorage.

  • for fiPersistentEditorStorage, create a fiPersistentEditorStorageComponent that is just a MonoBehavior shell so we can use FindObjectsOfType.

Most other changes are just related to API change (like using instance IDs in certain places, since the actual references would be dead when exiting playmode or changing serialization type). Also adding a null-guard for the only usage of Read method, that can now return a default(T).

What isn't good yet

Note: Unity will show warnings when we move a FI object from a scene to another. This is because we're storing a reference to the object in the scene storage(s), and Unity forbids cross-scene references (will show a "scene mismatch" message on the object's field, as well as warnings in the console).

  • I managed to get the references from the fiBackupSceneStorage properly (Unity will still show warnings), but I found no way to migrate the references stored in the fiPersistentEditorStorage.

  • This is a potential problem-case that I didn't test: In playmode, the user can load scenes in-game, then backup a object in the newly loaded scene. When exiting playmode, we would not be able to migrate that object's backups. I imagine that, with the current way the fiStorageManager.MigrateStorage() is called, the migration would occur the next time the scene is loaded. Again, I did not test this.

@jacobdufault
Copy link
Owner

How has this been working out for you? If you have a more recent version feel free to repush and ping and I'll take a deeper look at the changes.

@SugoiDev
Copy link
Collaborator Author

SugoiDev commented Jun 8, 2017

I've been using it ever since without issues in a multi-scene heavy setup. I don't do a love of cross-scene objects moving, but that was the only thing with some issues still (unity warnings for cross-scene refs).

This cross-scene reference madness is probably because Unity lacks actual persistent object IDs in its pseudo-ECS.

@SugoiDev
Copy link
Collaborator Author

SugoiDev commented Jun 8, 2017

I might have some extra changes here, so give me a few days before you review/merge.

@SugoiDev
Copy link
Collaborator Author

SugoiDev commented Jun 9, 2017

Alright. I've added a few missing files from my current project and fixed the unicode characters in the backup context menus.

Copy link
Owner

@jacobdufault jacobdufault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of small style nits; I can add comments if you're interested in fixing them. Overall looks good.

}
}

//latodo this should be on the PR as well (the DontDestroyOnLoad check)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

}

for (var i = 0; i < SceneManager.sceneCount; i++) {
var scene = SceneManager.GetSceneAt(i);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: var should only be used when the type is obvious (ie, already in the line) - otherwise it decreases readability.

SugoiDev added 2 commits June 23, 2017 00:21
The latodo is for an internal todo tracking I use. It slipped through.

I changed other occurrences of GetSceneAt's result using `var` instead of the actual type `Scene` in `fiStoragesManager`.
Added cached `fiUnityObjectReference` to use on `OnInspectorGUI()` in `fiCommonSerializedObjectEditor`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants