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.

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

Fix issues after storyboard resource lookup refactor #24863

Merged
merged 2 commits into from Sep 20, 2023

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Sep 19, 2023

Two regressions from #24809:

Fix StoryboardResourceLookupStore dying on failure to unmap path

Before the introduction of StoryboardResourceLookupStore, missing files would softly fail by use of null fallbacks:

if (!string.IsNullOrEmpty(resolvedPath))
return textureStore.Get(resolvedPath);
return null;

After the aforementioned class was added, however, the fallbacks would not work anymore if for whatever reason GetStoragePathFromStoryboardPath() failed to unmap the storyboard asset name to a storage path.

While that helped unearth the other regression (more about that in a second) by making it more loud, it's probably not something we want around due to reliability reasons.

Fix DrawableStoryboardVideo attempting to unmap path once too much

The StoryboardResourceLookupStore cached at storyboard level is supposed to already be handling that; no need for local logic anymore.

This is a baffling oversight on my part since the OP of the original PR mentioned the weird DrawableStoryboardVideo logic explicitly, but then I failed to spot that the PR was nto touching that class at all. Or test storyboards with videos. (I did test storyboards, but only ones without videos.........)

Before the introduction of `StoryboardResourceLookupStore`, missing
files would softly fail by use of null fallbacks. After the
aforementioned class was added, however, the fallbacks would not work
anymore if for whatever reason `GetStoragePathFromStoryboardPath()`
failed to unmap the storyboard asset name to a storage path.
The `StoryboardResourceLookupStore` cached at storyboard level is
supposed to already be handling that; no need for local logic anymore.
Comment on lines +145 to +161
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);
}
Copy link
Collaborator Author

@bdach bdach Sep 19, 2023

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.

@bdach bdach requested a review from peppy September 19, 2023 18:34
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 😞

@peppy peppy merged commit cd4651f into ppy:master Sep 20, 2023
17 checks passed
@bdach bdach deleted the storyboard-fail branch September 20, 2023 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on entering some beatmaps
2 participants