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

Jump to bottom

Fix issues after storyboard resource lookup refactor #24863

Merged
merged 2 commits into from
Sep 20, 2023
Merged

Fix issues after storyboard resource lookup refactor #24863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba518e1
Fix `StoryboardResourceLookupStore` dying on failure to unmap path
bdach Sep 19, 2023
641e651
Fix `DrawableStoryboardVideo` attempting to unmap path once too much
bdach Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 24 additions & 6 deletions osu.Game/Storyboards/Drawables/DrawableStoryboard.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,32 @@ public StoryboardResourceLookupStore(Storyboard storyboard, RealmAccess realm, G
public void Dispose() =>
realmFileStore.Dispose();

public byte[] Get(string name) =>
realmFileStore.Get(storyboard.GetStoragePathFromStoryboardPath(name));
public byte[] Get(string name)
{
string? storagePath = storyboard.GetStoragePathFromStoryboardPath(name);

public Task<byte[]> GetAsync(string name, CancellationToken cancellationToken = new CancellationToken()) =>
realmFileStore.GetAsync(storyboard.GetStoragePathFromStoryboardPath(name), cancellationToken);
return string.IsNullOrEmpty(storagePath)
? null!
: realmFileStore.Get(storagePath);
}

public Stream GetStream(string name) =>
realmFileStore.GetStream(storyboard.GetStoragePathFromStoryboardPath(name));
public Task<byte[]> GetAsync(string name, CancellationToken cancellationToken = new CancellationToken())
{
string? storagePath = storyboard.GetStoragePathFromStoryboardPath(name);

return string.IsNullOrEmpty(storagePath)
? Task.FromResult<byte[]>(null!)
: realmFileStore.GetAsync(storagePath, cancellationToken);
}
Comment on lines +145 to +161
Copy link
Collaborator Author

@bdach bdach Sep 19, 2023 •
edited

Choose a reason for hiding this comment

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

The null!s here are slightly annoying. You would probably very wisely say to do this:

diff --git a/osu.Game/Storyboards/Drawables/DrawableStoryboard.cs b/osu.Game/Storyboards/Drawables/DrawableStoryboard.cs
index c2a58d46ef..631475cce8 100644
--- a/osu.Game/Storyboards/Drawables/DrawableStoryboard.cs
+++ b/osu.Game/Storyboards/Drawables/DrawableStoryboard.cs
@@ -128,7 +128,7 @@ private void updateLayerVisibility()
                 layer.Enabled = passing ? layer.Layer.VisibleWhenPassing : layer.Layer.VisibleWhenFailing;
         }
 
-        private class StoryboardResourceLookupStore : IResourceStore<byte[]>
+        private class StoryboardResourceLookupStore : IResourceStore<byte[]?>
         {
             private readonly IResourceStore<byte[]> realmFileStore;
             private readonly Storyboard storyboard;
@@ -142,21 +142,21 @@ public StoryboardResourceLookupStore(Storyboard storyboard, RealmAccess realm, G
             public void Dispose() =>
                 realmFileStore.Dispose();
 
-            public byte[] Get(string name)
+            public byte[]? Get(string name)
             {
                 string? storagePath = storyboard.GetStoragePathFromStoryboardPath(name);
 
                 return string.IsNullOrEmpty(storagePath)
-                    ? null!
+                    ? null
                     : realmFileStore.Get(storagePath);
             }
 
-            public Task<byte[]> GetAsync(string name, CancellationToken cancellationToken = new CancellationToken())
+            public Task<byte[]?> GetAsync(string name, CancellationToken cancellationToken = new CancellationToken())
             {
                 string? storagePath = storyboard.GetStoragePathFromStoryboardPath(name);
 
                 return string.IsNullOrEmpty(storagePath)
-                    ? Task.FromResult<byte[]>(null!)
+                    ? Task.FromResult<byte[]?>(null)
                     : realmFileStore.GetAsync(storagePath, cancellationToken);
             }

But the fun part is that at that point, there is a .NET library NRT fail:

image

and to make it shut up, you have to do this abomination:

1695148291

Yes you're reading that right, that's two shut-up operator occurrences. So I decided I'd rather leave well alone.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yeah. I've tried multiple times to fix nullability in resource stores but it needs to basically happen everywhere at once.


public Stream? GetStream(string name)
{
string? storagePath = storyboard.GetStoragePathFromStoryboardPath(name);

return string.IsNullOrEmpty(storagePath)
? null
: realmFileStore.GetStream(storagePath);
}

public IEnumerable<string> GetAvailableResources() =>
realmFileStore.GetAvailableResources();
Expand Down
7 changes: 1 addition & 6 deletions osu.Game/Storyboards/Drawables/DrawableStoryboardVideo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,7 @@ public DrawableStoryboardVideo(StoryboardVideo video)
[BackgroundDependencyLoader(true)]
private void load(IBindable<WorkingBeatmap> beatmap, TextureStore textureStore)
{
string? path = beatmap.Value.BeatmapSetInfo?.GetPathForFile(Video.Path);

if (path == null)
return;

var stream = textureStore.GetStream(path);
var stream = textureStore.GetStream(Video.Path);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This was a rebase conflict resolution failure 😞


if (stream == null)
return;
Expand Down