Add map preview cache #630
Conversation
|
Nightly build for this pull request:
|
| return null; | ||
| } | ||
|
|
||
| private async Task MapTextureLoadingService(CancellationToken cancellationToken) |
There was a problem hiding this comment.
don't think the functions should be called like so, the functions is "do something", not "a thing"
|
|
||
| private readonly ConcurrentDictionary<Map, Image> mapTextures = []; | ||
|
|
||
| private readonly ConcurrentDictionary<Map, byte> missedMaps = []; |
There was a problem hiding this comment.
- Why is this a dictionary when this should be a set? If the reason is that there's no concurrent set it's better to use some empty type like
System.Reactive.Unit, for instance, as it better conveys the purpose. - IMO this should be brought higher and renamed to
pendingMaps, it's notmissing, the maps are just waiting to be processed.
There was a problem hiding this comment.
So what do you suggest? I think we can't introduce RX here right now just because of introducing the unit type
There was a problem hiding this comment.
Well, I don't see a big problem with introducing Rx, but I guess you could make an empty type or just copy the unit type from System.Reactive, and later we could just replace it when we introduce Rx.
| if (mapTextures.TryGetValue(map, out Image image)) | ||
| return image; | ||
|
|
||
| if (missedMaps.Count < MaxCacheSize) |
There was a problem hiding this comment.
Isn't this check redundant? There seems to be no point in regulating the input size.
There was a problem hiding this comment.
You missed the point. The RAM usage regulation is going to happen in the main dictionary which actually holds the map previews. AFAIK the map list is already in memory, so there wouldn't be a point to also regulate this as the most it will save is a few bytes. Also I don't think hitting 100 maps limit in 100ms is possible at all, and the "incoming" dictionary is drained regularly.
There was a problem hiding this comment.
Then do you suggest also removing MaxCacheSize limits for the dictionary that holds bitmaps or not?
There was a problem hiding this comment.
No, only from the pending/missing maps.
| public void Dispose() => | ||
| cancellationTokenSource?.Cancel(); | ||
|
|
||
| public Image GetMapTextureIfAvailable(Map map) |
There was a problem hiding this comment.
| public Image GetMapTextureIfAvailable(Map map) | |
| public Image? GetMapTextureIfAvailable(Map map) |
There was a problem hiding this comment.
I will write #nullable enable on file MapTextureCacheManager.cs, if we decide to use nullable pattern for new files
There was a problem hiding this comment.
IMO nullables should be enabled across all projects.
There was a problem hiding this comment.
IMO nullables should be enabled across all projects.
Rans4cker once did that. And the majority objected because it introduces too many warning messages on existing codes. So to smoothly migrate (no one expects another .NET 6 migration PR) I suggest we enable that on new files as well as files being major revised
There was a problem hiding this comment.
IIRC the main complaint was too strict StyleCop warnings, but alright. Those warnings are actually useful in this case IMO.
There was a problem hiding this comment.
I know it's helpful. The point is there are too many warnings that would draw our attention from new warnings introduced
| } | ||
|
|
There was a problem hiding this comment.
huh? No changes were made
There was a problem hiding this comment.
Ugh, I tried to remove the extra newline. Seems GitHub ate that.
| @@ -43,7 +43,7 @@ public GameInformationPanel(WindowManager windowManager, MapLoader mapLoader) | |||
|
|
|||
| private GenericHostedGame game = null; | |||
|
|
|||
| private bool disposeTextures = false; | |||
| private bool disposeTextures = true; | |||
There was a problem hiding this comment.
This just caught my attention, I don't think there should be such a flag, just the no map preview texture should be set as a singleton, and then when disposing the texture should be compared to the no map preview one.
There was a problem hiding this comment.
The disposeTextures pattern is directly copied from MapPreviewBox class. In that class, PreloadMapPreviews is such a texture that should not be disposed.
I think we can leave this design as a future work since the client already has such a pattern for a long time?
There was a problem hiding this comment.
I thought that's a trivial change to do so though, isn't that right?
There was a problem hiding this comment.
Should be but it might require additional tests. Will try migrating it after I am available again
| } | ||
| else | ||
| { | ||
| mapTexture = AssetLoader.TextureFromImage(mapLoader.MapTextureCacheManager.GetMapTextureIfAvailable(map)); |
There was a problem hiding this comment.
Does this mean the first show of the preview will always show null texture? When will it be updated? IMO there should be a callback that will update the info upon loading the map.
There was a problem hiding this comment.
Yes. Currently it will be updated the second time it is visited.
Will trying implementing a callback after I am available again (~weeks perhaps)
| private readonly CancellationTokenSource cancellationTokenSource = new(); | ||
|
|
||
| public MapTextureCacheManager() => | ||
| Task.Run(() => MapTextureLoadingService(cancellationTokenSource.Token)); |
There was a problem hiding this comment.
There's no error handling and no task tracking. If your task errors out - there won't be any map previews anymore provided and the exception will not be caught.
There was a problem hiding this comment.
Will fix it by catching and recording exceptions in the while loop
There was a problem hiding this comment.
Actually hold on, maybe it would be better to use something like a weak reference for the cache instead of manually doing it?
Also I am not sure if clearing all of the maps when the limit is reached is a good idea, I would shave off the not used ones. Some maps are going to appear more frequently so they won't be deleted.
Thanks for letting me know |
No description provided.