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

Add ability to create new difficulties from within the editor #16754

Merged
merged 17 commits into from Feb 4, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
95 changes: 95 additions & 0 deletions osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs
Expand Up @@ -90,5 +90,100 @@ public void TestAddAudioTrack()

AddAssert("track length changed", () => Beatmap.Value.Track.Length > 60000);
}

[Test]
public void TestCreateNewDifficulty()
{
string firstDifficultyName = Guid.NewGuid().ToString();
string secondDifficultyName = Guid.NewGuid().ToString();

AddStep("set unique difficulty name", () => EditorBeatmap.BeatmapInfo.DifficultyName = firstDifficultyName);
AddStep("save beatmap", () => Editor.Save());
AddAssert("new beatmap persisted", () =>
{
var beatmap = beatmapManager.QueryBeatmap(b => b.DifficultyName == firstDifficultyName);
var set = beatmapManager.QueryBeatmapSet(s => s.ID == EditorBeatmap.BeatmapInfo.BeatmapSet.ID);

return beatmap != null
&& beatmap.DifficultyName == firstDifficultyName
&& set != null
&& set.PerformRead(s => s.Beatmaps.Single().ID == beatmap.ID);
});
AddAssert("can save again", () => Editor.Save());

AddStep("create new difficulty", () => Editor.CreateNewDifficulty(new OsuRuleset().RulesetInfo));
AddUntilStep("wait for created", () =>
{
string difficultyName = Editor.ChildrenOfType<EditorBeatmap>().SingleOrDefault()?.BeatmapInfo.DifficultyName;
return difficultyName != null && difficultyName != firstDifficultyName;
});

AddStep("set unique difficulty name", () => EditorBeatmap.BeatmapInfo.DifficultyName = secondDifficultyName);
AddStep("save beatmap", () => Editor.Save());
AddAssert("new beatmap persisted", () =>
{
var beatmap = beatmapManager.QueryBeatmap(b => b.DifficultyName == secondDifficultyName);
var set = beatmapManager.QueryBeatmapSet(s => s.ID == EditorBeatmap.BeatmapInfo.BeatmapSet.ID);

return beatmap != null
&& beatmap.DifficultyName == secondDifficultyName
&& set != null
&& set.PerformRead(s => s.Beatmaps.Count == 2 && s.Beatmaps.Any(b => b.DifficultyName == secondDifficultyName));
});
}

[Test]
public void TestCreateNewBeatmapFailsWithBlankNamedDifficulties()
{
Guid setId = Guid.Empty;

AddStep("retrieve set ID", () => setId = EditorBeatmap.BeatmapInfo.BeatmapSet!.ID);
AddStep("save beatmap", () => Editor.Save());
AddAssert("new beatmap persisted", () =>
{
var set = beatmapManager.QueryBeatmapSet(s => s.ID == setId);
return set != null && set.PerformRead(s => s.Beatmaps.Count == 1 && s.Files.Count == 1);
});

AddStep("try to create new difficulty", () => Editor.CreateNewDifficulty(new OsuRuleset().RulesetInfo));
AddAssert("beatmap set unchanged", () =>
Comment on lines +148 to +149
Copy link
Member

Choose a reason for hiding this comment

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

The test case states that creating a new beatmap fails with blank difficulty name, but I think this is missing an Editor.Save() step here to confirm that it would fail? (or was the test case intended to ensure CreateNewDifficulty doesn't affect the databased beatmap set when Save is not performed?)

{
var set = beatmapManager.QueryBeatmapSet(s => s.ID == setId);
return set != null && set.PerformRead(s => s.Beatmaps.Count == 1 && s.Files.Count == 1);
});
}

[Test]
public void TestCreateNewBeatmapFailsWithSameNamedDifficulties()
{
Guid setId = Guid.Empty;
const string duplicate_difficulty_name = "duplicate";

AddStep("retrieve set ID", () => setId = EditorBeatmap.BeatmapInfo.BeatmapSet!.ID);
AddStep("set difficulty name", () => EditorBeatmap.BeatmapInfo.DifficultyName = duplicate_difficulty_name);
AddStep("save beatmap", () => Editor.Save());
AddAssert("new beatmap persisted", () =>
{
var set = beatmapManager.QueryBeatmapSet(s => s.ID == setId);
return set != null && set.PerformRead(s => s.Beatmaps.Count == 1 && s.Files.Count == 1);
});

AddStep("create new difficulty", () => Editor.CreateNewDifficulty(new OsuRuleset().RulesetInfo));
AddUntilStep("wait for created", () =>
{
string difficultyName = Editor.ChildrenOfType<EditorBeatmap>().SingleOrDefault()?.BeatmapInfo.DifficultyName;
return difficultyName != null && difficultyName != duplicate_difficulty_name;
});

AddStep("set difficulty name", () => EditorBeatmap.BeatmapInfo.DifficultyName = duplicate_difficulty_name);
AddStep("try to save beatmap", () => Editor.Save());
AddAssert("beatmap set not corrupted", () =>
{
var set = beatmapManager.QueryBeatmapSet(s => s.ID == setId);
// the difficulty was already created at the point of the switch.
// what we want to check is that both difficulties do not use the same file.
return set != null && set.PerformRead(s => s.Beatmaps.Count == 2 && s.Files.Count == 2);
});
}
}
}
38 changes: 37 additions & 1 deletion osu.Game/Beatmaps/BeatmapManager.cs
Expand Up @@ -73,7 +73,9 @@ protected virtual WorkingBeatmapCache CreateWorkingBeatmapCache(AudioManager aud
new BeatmapModelManager(realm, storage, onlineLookupQueue);

/// <summary>
/// Create a new <see cref="WorkingBeatmap"/>.
/// Create a new beatmap set, backed by a <see cref="BeatmapSetInfo"/> model,
/// with a single difficulty which is backed by a <see cref="BeatmapInfo"/> model
/// and represented by the returned usable <see cref="WorkingBeatmap"/>.
/// </summary>
public WorkingBeatmap CreateNew(RulesetInfo ruleset, APIUser user)
{
Expand Down Expand Up @@ -105,6 +107,40 @@ public WorkingBeatmap CreateNew(RulesetInfo ruleset, APIUser user)
return imported.PerformRead(s => GetWorkingBeatmap(s.Beatmaps.First()));
}

/// <summary>
/// Add a new difficulty to the beatmap set represented by the provided <see cref="BeatmapSetInfo"/>.
/// The new difficulty will be backed by a <see cref="BeatmapInfo"/> model
/// and represented by the returned <see cref="WorkingBeatmap"/>.
/// </summary>
public virtual WorkingBeatmap CreateNewBlankDifficulty(BeatmapSetInfo beatmapSetInfo, RulesetInfo rulesetInfo)
{
// fetch one of the existing difficulties to copy timing points and metadata from,
// so that the user doesn't have to fill all of that out again.
// this silently assumes that all difficulties have the same timing points and metadata,
// but cases where this isn't true seem rather rare / pathological.
var referenceBeatmap = GetWorkingBeatmap(beatmapSetInfo.Beatmaps.First());

var newBeatmapInfo = new BeatmapInfo(rulesetInfo, new BeatmapDifficulty(), referenceBeatmap.Metadata.DeepClone());

// populate circular beatmap set info <-> beatmap info references manually.
// several places like `BeatmapModelManager.Save()` or `GetWorkingBeatmap()`
// rely on them being freely traversable in both directions for correct operation.
beatmapSetInfo.Beatmaps.Add(newBeatmapInfo);
newBeatmapInfo.BeatmapSet = beatmapSetInfo;

var newBeatmap = new Beatmap { BeatmapInfo = newBeatmapInfo };
foreach (var timingPoint in referenceBeatmap.Beatmap.ControlPointInfo.TimingPoints)
newBeatmap.ControlPointInfo.Add(timingPoint.Time, timingPoint.DeepClone());

beatmapModelManager.Save(newBeatmapInfo, newBeatmap);

workingBeatmapCache.Invalidate(beatmapSetInfo);
return GetWorkingBeatmap(newBeatmap.BeatmapInfo);
}

// TODO: add back support for making a copy of another difficulty
// (likely via a separate `CopyDifficulty()` method).

/// <summary>
/// Delete a beatmap difficulty.
/// </summary>
Expand Down
16 changes: 15 additions & 1 deletion osu.Game/Beatmaps/BeatmapMetadata.cs
Expand Up @@ -7,6 +7,7 @@
using osu.Framework.Testing;
using osu.Game.Models;
using osu.Game.Users;
using osu.Game.Utils;
using Realms;

#nullable enable
Expand All @@ -16,7 +17,7 @@ namespace osu.Game.Beatmaps
[ExcludeFromDynamicCompile]
[Serializable]
[MapTo("BeatmapMetadata")]
public class BeatmapMetadata : RealmObject, IBeatmapMetadataInfo
public class BeatmapMetadata : RealmObject, IBeatmapMetadataInfo, IDeepCloneable<BeatmapMetadata>
{
public string Title { get; set; } = string.Empty;

Expand Down Expand Up @@ -57,5 +58,18 @@ private BeatmapMetadata()
IUser IBeatmapMetadataInfo.Author => Author;

public override string ToString() => this.GetDisplayTitle();

public BeatmapMetadata DeepClone() => new BeatmapMetadata(Author.DeepClone())
Copy link
Sponsor Member

@peppy peppy Feb 3, 2022

Choose a reason for hiding this comment

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

I'm not confident on us implementing these manually all over the place. One of the things I wanted to look at is replacing IDeepCloneable with automapper. Is that something you could get behind?

That said, I think this is fine for the time being to get things working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given the fact that I've messed up writing these clones no less than twice in this pull alone, you won't hear me protest against using automapper for these for sure.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Just to confirm, the clones are to avoid reference issues at the detached side correct? And have nothing to do with realm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The metadata clone was originally to ensure 1:1 BeatmapInfo to BeatmapMetadata correspondence as you requested in an earlier version of this. It will not be actually necessary anymore for that original purpose, but probably safest to have for sanity to avoid reference issues as you say.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

If it's not required I'd argue we omit it to keep things as simple as possible... not sure.

Copy link
Collaborator Author

@bdach bdach Feb 3, 2022

Choose a reason for hiding this comment

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

I'll also note that the metadata directly associated with the BeatmapInfo here will become managed upon the beatmap being added, which is another reason to use a temporary clone for that.

I'll try removing but I feel somewhat safer with it.

{
Title = Title,
TitleUnicode = TitleUnicode,
Artist = Artist,
ArtistUnicode = ArtistUnicode,
Source = Source,
Tags = Tags,
PreviewTime = PreviewTime,
AudioFile = AudioFile,
BackgroundFile = BackgroundFile
};
}
}
13 changes: 9 additions & 4 deletions osu.Game/Beatmaps/BeatmapModelManager.cs
Expand Up @@ -46,10 +46,9 @@ public BeatmapModelManager(RealmAccess realm, Storage storage, BeatmapOnlineLook
/// <param name="beatmapInfo">The <see cref="BeatmapInfo"/> to save the content against. The file referenced by <see cref="BeatmapInfo.Path"/> will be replaced.</param>
/// <param name="beatmapContent">The <see cref="IBeatmap"/> content to write.</param>
/// <param name="beatmapSkin">The beatmap <see cref="ISkin"/> content to write, null if to be omitted.</param>
public virtual void Save(BeatmapInfo beatmapInfo, IBeatmap beatmapContent, ISkin? beatmapSkin = null)
public void Save(BeatmapInfo beatmapInfo, IBeatmap beatmapContent, ISkin? beatmapSkin = null)
{
var setInfo = beatmapInfo.BeatmapSet;

Debug.Assert(setInfo != null);

// Difficulty settings must be copied first due to the clone in `Beatmap<>.BeatmapInfo_Set`.
Expand All @@ -72,6 +71,12 @@ public virtual void Save(BeatmapInfo beatmapInfo, IBeatmap beatmapContent, ISkin

// AddFile generally handles updating/replacing files, but this is a case where the filename may have also changed so let's delete for simplicity.
var existingFileInfo = setInfo.Files.SingleOrDefault(f => string.Equals(f.Filename, beatmapInfo.Path, StringComparison.OrdinalIgnoreCase));
string targetFilename = getFilename(beatmapInfo);

// ensure that two difficulties from the set don't point at the same beatmap file.
if (setInfo.Beatmaps.Any(b => b.ID != beatmapInfo.ID && string.Equals(b.Path, targetFilename, StringComparison.OrdinalIgnoreCase)))
throw new InvalidOperationException($"{setInfo.GetDisplayString()} already has a difficulty with the name of '{beatmapInfo.DifficultyName}'.");

if (existingFileInfo != null)
DeleteFile(setInfo, existingFileInfo);

Expand Down Expand Up @@ -103,9 +108,9 @@ private static string getFilename(BeatmapInfo beatmapInfo)

public void Update(BeatmapSetInfo item)
{
Realm.Write(realm =>
Realm.Write(r =>
{
var existing = realm.Find<BeatmapSetInfo>(item.ID);
var existing = r.Find<BeatmapSetInfo>(item.ID);
item.CopyChangesToRealm(existing);
});
}
Expand Down
11 changes: 10 additions & 1 deletion osu.Game/Database/RealmObjectExtensions.cs
Expand Up @@ -58,7 +58,16 @@ public static class RealmObjectExtensions
if (existing != null)
copyChangesToRealm(beatmap, existing);
else
d.Beatmaps.Add(beatmap);
{
var newBeatmap = new BeatmapInfo
{
ID = beatmap.ID,
BeatmapSet = d,
Ruleset = d.Realm.Find<RulesetInfo>(beatmap.Ruleset.ShortName)
};
d.Beatmaps.Add(newBeatmap);
copyChangesToRealm(beatmap, newBeatmap);
}
}
});

Expand Down
3 changes: 2 additions & 1 deletion osu.Game/Graphics/UserInterface/DrawableOsuMenuItem.cs
Expand Up @@ -117,14 +117,15 @@ public TextContainer()
{
NormalText = new OsuSpriteText
{
AlwaysPresent = true, // ensures that the menu item does not change width when switching between normal and bold text.
Anchor = Anchor.CentreLeft,
Origin = Anchor.CentreLeft,
Font = OsuFont.GetFont(size: text_size),
Margin = new MarginPadding { Horizontal = MARGIN_HORIZONTAL, Vertical = MARGIN_VERTICAL },
},
BoldText = new OsuSpriteText
{
AlwaysPresent = true,
AlwaysPresent = true, // ensures that the menu item does not change width when switching between normal and bold text.
Alpha = 0,
Anchor = Anchor.CentreLeft,
Origin = Anchor.CentreLeft,
Expand Down
6 changes: 5 additions & 1 deletion osu.Game/Models/RealmUser.cs
Expand Up @@ -2,12 +2,14 @@
// See the LICENCE file in the repository root for full licence text.

using System;
using osu.Game.Database;
using osu.Game.Users;
using osu.Game.Utils;
using Realms;

namespace osu.Game.Models
{
public class RealmUser : EmbeddedObject, IUser, IEquatable<RealmUser>
public class RealmUser : EmbeddedObject, IUser, IEquatable<RealmUser>, IDeepCloneable<RealmUser>
{
public int OnlineID { get; set; } = 1;

Expand All @@ -22,5 +24,7 @@ public bool Equals(RealmUser other)

return OnlineID == other.OnlineID && Username == other.Username;
}

public RealmUser DeepClone() => (RealmUser)this.Detach().MemberwiseClone();
}
}