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.

Sign up for GitHub

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

Add map preview cache #630

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
+115 −12
Open

Add map preview cache #630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5cae363
Add map preview cache
SadPencil Jan 8, 2025
eee4261
Update MapTextureCacheManager.cs
SadPencil Jan 8, 2025
cc24bcb
Update MapTextureCacheManager.cs
SadPencil Jan 8, 2025
ec2aec8
Update MapTextureCacheManager.cs
SadPencil Jan 8, 2025
100d5bb
Merge branch 'develop' into feat-add-map-preview-cache
SadPencil Jan 8, 2025
dad5fdc
Update GameModeMap.cs
SadPencil Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion DXMainClient/DXGUI/GameClass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
using Rampastring.Tools;
using Rampastring.XNAUI;
using System;
using ClientGUI;

Check warning on line 13 in DXMainClient/DXGUI/GameClass.cs

View workflow job for this annotation

GitHub Actions / build-clients (Ares) 

The using directive for 'ClientGUI' appeared previously in this namespace

Check warning on line 13 in DXMainClient/DXGUI/GameClass.cs

View workflow job for this annotation

GitHub Actions / build-clients (TS) 

The using directive for 'ClientGUI' appeared previously in this namespace

Check warning on line 13 in DXMainClient/DXGUI/GameClass.cs

View workflow job for this annotation

GitHub Actions / build-clients (YR) 

The using directive for 'ClientGUI' appeared previously in this namespace
using DTAClient.Domain.Multiplayer;
using DTAClient.Domain.Multiplayer.CnCNet;
using DTAClient.DXGUI.Multiplayer;
Expand Down Expand Up @@ -257,7 +257,8 @@
.AddSingleton<TunnelHandler>()
.AddSingleton<DiscordHandler>()
.AddSingleton<PrivateMessageHandler>()
.AddSingleton<MapLoader>();
.AddSingleton<MapLoader>()
.AddSingleton<MapTextureCacheManager>();

// singleton xna controls - same instance on each request
services
Expand Down
25 changes: 18 additions & 7 deletions DXMainClient/DXGUI/Multiplayer/GameInformationPanel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public GameInformationPanel(WindowManager windowManager, MapLoader mapLoader)
DrawMode = ControlDrawMode.UNIQUE_RENDER_TARGET;
}

private MapLoader mapLoader;
private readonly MapLoader mapLoader;

private XNALabel lblGameInformation;
private XNALabel lblGameMode;
Expand All @@ -43,7 +43,7 @@ public GameInformationPanel(WindowManager windowManager, MapLoader mapLoader)

private GenericHostedGame game = null;

private bool disposeTextures = false;
private bool disposeTextures = true;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I thought that's a trivial change to do so though, isn't that right?

Copy link
Member Author

@SadPencil SadPencil Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Should be but it might require additional tests. Will try migrating it after I am available again

private Texture2D mapTexture = null;
private Texture2D noMapPreviewTexture = null;

Expand Down Expand Up @@ -194,17 +194,28 @@ public void SetInfo(GenericHostedGame game)

if (mapLoader != null)
{
mapTexture = mapLoader.GameModeMaps.Find(m => m.Map.UntranslatedName.Equals(game.Map, StringComparison.InvariantCultureIgnoreCase) && m.Map.IsPreviewTextureCached())?.Map?.LoadPreviewTexture();
Map map = mapLoader.GameModeMaps.Find(m => m.Map.UntranslatedName.Equals(game.Map, StringComparison.InvariantCultureIgnoreCase))?.Map;

if (map != null)
{
if (map.IsPreviewTextureAvailableAsFile())
{
mapTexture = map.LoadPreviewTexture();
disposeTextures = true;
}
else
{
mapTexture = AssetLoader.TextureFromImage(mapLoader.MapTextureCacheManager.GetMapTextureIfAvailable(map));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Currently it will be updated the second time it is visited.

Will trying implementing a callback after I am available again (~weeks perhaps)

disposeTextures = true;
Metadorius marked this conversation as resolved.
Show resolved Hide resolved
}
}

if (mapTexture == null && noMapPreviewTexture != null)
{
Debug.Assert(!noMapPreviewTexture.IsDisposed, "noMapPreviewTexture should not be disposed.");
mapTexture = noMapPreviewTexture;
disposeTextures = false;
}
else
{
disposeTextures = true;
}
}
}

Expand Down
75 changes: 75 additions & 0 deletions DXMainClient/DXGUI/Multiplayer/MapTextureCacheManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
using System;
using System.Collections.Concurrent;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

using DTAClient.Domain.Multiplayer;

using SixLabors.ImageSharp;

namespace DTAClient.DXGUI.Multiplayer
{
public class MapTextureCacheManager : IDisposable
{
public const int MaxCacheSize = 100;
public const int SleepIntervalMS = 100;

private readonly ConcurrentDictionary<Map, Image> mapTextures = [];

private readonly ConcurrentDictionary<Map, byte> missedMaps = [];
Copy link
Member

Choose a reason for hiding this comment

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

  1. 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.
  2. IMO this should be brought higher and renamed to pendingMaps, it's not missing, the maps are just waiting to be processed.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what do you suggest? I think we can't introduce RX here right now just because of introducing the unit type

Copy link
Member

Choose a reason for hiding this comment

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

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.


private readonly CancellationTokenSource cancellationTokenSource = new();

public MapTextureCacheManager() =>
Task.Run(() => MapTextureLoadingService(cancellationTokenSource.Token));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix it by catching and recording exceptions in the while loop


public void Dispose() =>
cancellationTokenSource?.Cancel();

public Image GetMapTextureIfAvailable(Map map)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public Image GetMapTextureIfAvailable(Map map)
public Image? GetMapTextureIfAvailable(Map map)

Copy link
Member Author

Choose a reason for hiding this comment

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

I will write #nullable enable on file MapTextureCacheManager.cs, if we decide to use nullable pattern for new files

Copy link
Member

Choose a reason for hiding this comment

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

IMO nullables should be enabled across all projects.

Copy link
Member Author

@SadPencil SadPencil Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

IIRC the main complaint was too strict StyleCop warnings, but alright. Those warnings are actually useful in this case IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it's helpful. The point is there are too many warnings that would draw our attention from new warnings introduced

{
if (mapTextures.TryGetValue(map, out Image image))
return image;

if (missedMaps.Count < MaxCacheSize)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this check redundant? There seems to be no point in regulating the input size.

Copy link
Member Author

Choose a reason for hiding this comment

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

Limiting RAM usage?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then do you suggest also removing MaxCacheSize limits for the dictionary that holds bitmaps or not?

Copy link
Member

Choose a reason for hiding this comment

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

No, only from the pending/missing maps.

missedMaps.TryAdd(map, 0);

return null;
}

private async Task MapTextureLoadingService(CancellationToken cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

don't think the functions should be called like so, the functions is "do something", not "a thing"

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix it

{
while (true)
{
cancellationToken.ThrowIfCancellationRequested();

// Clear cache if it's too big
if (mapTextures.Count > MaxCacheSize)
mapTextures.Clear();

if (!missedMaps.IsEmpty)
{
var missedMapCopy = missedMaps.ToArray();
foreach ((Map missedMap, _) in missedMapCopy)
{
if (mapTextures.Count > MaxCacheSize)
break;

missedMaps.TryRemove(missedMap, out _);

if (mapTextures.ContainsKey(missedMap))
continue;

Image image = await Task.Run(missedMap.ExtractMapPreview);
mapTextures.TryAdd(missedMap, image);
}

Comment on lines +66 to +67
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

huh? No changes were made

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, I tried to remove the extra newline. Seems GitHub ate that.

}

await Task.Delay(SleepIntervalMS);
}
}

}
}
3 changes: 3 additions & 0 deletions DXMainClient/Domain/Multiplayer/GameModeMap.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ public GameModeMap(GameMode gameMode, Map map, bool isFavorite)
IsFavorite = isFavorite;
}

public override string ToString()
=> $"{GameMode?.Name} - {Map?.Name}";

protected bool Equals(GameModeMap other) => Equals(GameMode, other.GameMode) && Equals(Map, other.Map);

public override int GetHashCode()
Expand Down
10 changes: 6 additions & 4 deletions DXMainClient/Domain/Multiplayer/Map.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
using Color = Microsoft.Xna.Framework.Color;
using Point = Microsoft.Xna.Framework.Point;
using Utilities = Rampastring.Tools.Utilities;
using static System.Collections.Specialized.BitVector32;
using System.Diagnostics;

namespace DTAClient.Domain.Multiplayer
Expand Down Expand Up @@ -680,9 +679,12 @@ private void ParseSpawnIniOptions(IniFile forcedOptionsIni, string spawnIniOptio
}
}

public bool IsPreviewTextureCached() =>
public bool IsPreviewTextureAvailableAsFile() =>
SafePath.GetFile(ProgramConstants.GamePath, PreviewPath).Exists;

public Image ExtractMapPreview() =>
MapPreviewExtractor.ExtractMapPreview(GetCustomMapIniFile());

/// <summary>
/// Loads and returns the map preview texture.
/// </summary>
Expand All @@ -694,7 +696,7 @@ public Texture2D LoadPreviewTexture()
if (!Official)
{
// Extract preview from the map itself
using Image preview = MapPreviewExtractor.ExtractMapPreview(GetCustomMapIniFile());
using Image preview = ExtractMapPreview();

if (preview != null)
{
Expand Down Expand Up @@ -917,6 +919,6 @@ private static Point GetIsoTilePixelCoord(int isoTileX, int isoTileY, string[] a

protected bool Equals(Map other) => string.Equals(SHA1, other?.SHA1, StringComparison.InvariantCultureIgnoreCase);

public override int GetHashCode() => SHA1 != null ? SHA1.GetHashCode() : 0;
public override int GetHashCode() => SHA1 != null ? SHA1.GetHashCode() : BaseFilePath.GetHashCode();
}
}
11 changes: 11 additions & 0 deletions DXMainClient/Domain/Multiplayer/MapLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@
using System.Linq;
using System.Text.Json;
using System.Threading.Tasks;

using ClientCore;

using DTAClient.DXGUI.Multiplayer;

using Rampastring.Tools;

namespace DTAClient.Domain.Multiplayer
Expand Down Expand Up @@ -55,6 +59,13 @@ public class MapLoader
/// </summary>
private string[] AllowedGameModes = ClientConfiguration.Instance.AllowedCustomGameModes.Split(',');

public readonly MapTextureCacheManager MapTextureCacheManager;

public MapLoader(MapTextureCacheManager mapTextureCacheManager)
{
MapTextureCacheManager = mapTextureCacheManager;
}

/// <summary>
/// Loads multiplayer map info asynchonously.
/// </summary>
Expand Down
Loading