Don't dispose of sequences when unnecessary #21351
Conversation
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
it looks like SequenceEqual is also using a for loop so I think this implementation is cleaner than adding an IEqualityComparer
There was a problem hiding this comment.
I see no null checks on key in the constructors. Since this is a lower level generic object. Perhaps it would be worth doing a null check on key.
There was a problem hiding this comment.
Same for value. Value can never be null?
There was a problem hiding this comment.
Well it didn't crash on any of the maps I tried to run
| @@ -162,10 +166,26 @@ public List<MiniYamlNode>[] GetRulesYaml() | |||
| return Manifest.Rules.Select(s => MiniYaml.FromStream(DefaultFileSystem.Open(s), s, stringPool: stringPool)).ToArray(); | |||
| } | |||
|
|
|||
| public SequenceSet GetSequences(IReadOnlyFileSystem fileSystem, string tileset, MiniYaml sequenceDefinitions) | |||
There was a problem hiding this comment.
Reusing the map specific file-system across different maps seems risky - what if two maps ship different sprites but have the same tileset and sequences?
There was a problem hiding this comment.
IMO we should ignore that edge case.
Hashing all files in the map and checking whether they are used is too expensive IMO, and It should be incredibly rare to have custom assets but with no custom sequences
There was a problem hiding this comment.
and it's not reusing the filesystem because it's a one time load and done thing
There was a problem hiding this comment.
though I suppose a modder that is taking full advantage of map directory tracker could encounter it fairly often 🤔
There was a problem hiding this comment.
Is it possible to have a mod and map distinction here? That would avoid my worry about running into a scenario where we reuse assets between two maps that shouldn't share them. I'm also guessing the mod assets would be a one-off cost we can pay at startup time. Then map assets are presumably small and reloading them for each game would be okay?
There was a problem hiding this comment.
That was the old loading which #20597 got rid of as too much RAM was in use. It kept the base files always loaded and had map files loaded separately. I dunno if removing this system was the most optimal approach to fixing RAM usage, but we certainly can't just revert back to an identical system
There was a problem hiding this comment.
I see. The description suggests a map with custom assets causes two copies of the assets in memory. I can see how that's a problem.
I'm still not a fan of crossing our fingers here regarding the filesystems across different maps loading the same data.
Perhaps a different suggestion: If we can detect a map adds no additional assets, then we can preserve the SequenceSet since we know it has only mod assets in it. If we're switching to or from a map with custom assets, we reload the SequenceSet. So when switching between maps that use vanilla mod assets we get the benefit here. Only switching, to, between, or from custom maps requires a reload.
There was a problem hiding this comment.
The main benefactor should be missions. They do have custom rules, but most or maybe all don't contain custom sequences. The only issue with missions is RA which has a desert shellmap, meaning missions don't get any benefit with this PR. (I mention this in the PR's description)
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
I see no null checks on key in the constructors. Since this is a lower level generic object. Perhaps it would be worth doing a null check on key.
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Same for value. Value can never be null?
| if (!sequencesLoaded) | ||
| { | ||
| map.Sequences.LoadSprites(); | ||
| sequencesLoaded = true; |
There was a problem hiding this comment.
sequencesLoaded feel somewhat dirty.
Wouldn't have been cleaner to maintain lastSequences/sequencesLoadedCache loaded and then reuse that at the moment you want to load the new set if they match.
But perhaps this is about GetSequences being called prior to PrepareMap.
There was a problem hiding this comment.
loaders are delayed to PrepareMap. I could move the flag to SpriteCache
#20597 got rid of defaultSequences as to not keep multiple copies of assets floating around. This freed up on RAM but now on every game start all sequences need to be reloaded, which in games with larger asset files is a massive problem.
This PR delays the disposal of loaded assets until we can check wether they can be reused. This is a fairly simple fix
A possible future optimisation would be to load tileset specific assets into a separate SequenceSet, so that in a common scenario where sequences match and tileset doesn't, we could avoid reloading everything