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

Only show components in skin editor which are usable on the current screen #17279

Merged
merged 11 commits into from Mar 18, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
Expand Up @@ -18,9 +18,11 @@
using osu.Game.Rulesets.Osu;
using osu.Game.Rulesets.Osu.Skinning.Legacy;
using osu.Game.Rulesets.Scoring;
using osu.Game.Screens.Play;
using osu.Game.Screens.Play.HUD;
using osu.Game.Skinning;
using osu.Game.Storyboards;
using osu.Game.Tests.Beatmaps;

namespace osu.Game.Tests.Visual.Gameplay
{
Expand All @@ -37,6 +39,12 @@ public class TestSceneBeatmapSkinFallbacks : OsuPlayerTestScene
[Cached(typeof(HealthProcessor))]
private HealthProcessor healthProcessor = new DrainingHealthProcessor(0);

[Cached]
private GameplayState gameplayState = new GameplayState(new TestBeatmap(new OsuRuleset().RulesetInfo), new OsuRuleset());

[Cached]
private readonly GameplayClock gameplayClock = new GameplayClock(new FramedClock());

protected override bool HasCustomSteps => true;

[Test]
Expand Down
8 changes: 8 additions & 0 deletions osu.Game.Tests/Visual/Gameplay/TestSceneHUDOverlay.cs
Expand Up @@ -8,12 +8,14 @@
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Testing;
using osu.Framework.Timing;
using osu.Game.Configuration;
using osu.Game.Rulesets.Mods;
using osu.Game.Rulesets.Osu;
using osu.Game.Rulesets.Scoring;
using osu.Game.Screens.Play;
using osu.Game.Skinning;
using osu.Game.Tests.Beatmaps;
using osuTK.Input;

namespace osu.Game.Tests.Visual.Gameplay
Expand All @@ -30,6 +32,12 @@ public class TestSceneHUDOverlay : OsuManualInputManagerTestScene
[Cached(typeof(HealthProcessor))]
private HealthProcessor healthProcessor = new DrainingHealthProcessor(0);

[Cached]
private GameplayState gameplayState = new GameplayState(new TestBeatmap(new OsuRuleset().RulesetInfo), new OsuRuleset());

[Cached]
private readonly GameplayClock gameplayClock = new GameplayClock(new FramedClock());

// best way to check without exposing.
private Drawable hideTarget => hudOverlay.KeyCounter;
private FillFlowContainer<KeyCounter> keyCounterFlow => hudOverlay.KeyCounter.ChildrenOfType<FillFlowContainer<KeyCounter>>().First();
Expand Down
Expand Up @@ -5,11 +5,13 @@
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Testing;
using osu.Framework.Timing;
using osu.Game.Rulesets;
using osu.Game.Rulesets.Osu;
using osu.Game.Rulesets.Scoring;
using osu.Game.Screens.Play;
using osu.Game.Skinning.Editor;
using osu.Game.Tests.Beatmaps;
using osuTK.Input;

namespace osu.Game.Tests.Visual.Gameplay
Expand All @@ -22,6 +24,12 @@ public class TestSceneSkinEditorMultipleSkins : SkinnableTestScene
[Cached(typeof(HealthProcessor))]
private HealthProcessor healthProcessor = new DrainingHealthProcessor(0);

[Cached]
private GameplayState gameplayState = new GameplayState(new TestBeatmap(new OsuRuleset().RulesetInfo), new OsuRuleset());

[Cached]
private readonly GameplayClock gameplayClock = new GameplayClock(new FramedClock());

[SetUpSteps]
public void SetUpSteps()
{
Expand Down
Expand Up @@ -10,11 +10,13 @@
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Testing;
using osu.Framework.Timing;
using osu.Game.Rulesets;
using osu.Game.Rulesets.Mods;
using osu.Game.Rulesets.Osu;
using osu.Game.Rulesets.Scoring;
using osu.Game.Screens.Play;
using osu.Game.Tests.Beatmaps;
using osuTK.Input;

namespace osu.Game.Tests.Visual.Gameplay
Expand All @@ -29,6 +31,12 @@ public class TestSceneSkinnableHUDOverlay : SkinnableTestScene
[Cached(typeof(HealthProcessor))]
private HealthProcessor healthProcessor = new DrainingHealthProcessor(0);

[Cached]
private GameplayState gameplayState = new GameplayState(new TestBeatmap(new OsuRuleset().RulesetInfo), new OsuRuleset());

[Cached]
private readonly GameplayClock gameplayClock = new GameplayClock(new FramedClock());

private IEnumerable<HUDOverlay> hudOverlays => CreatedDrawables.OfType<HUDOverlay>();

// best way to check without exposing.
Expand Down
6 changes: 2 additions & 4 deletions osu.Game/Screens/Play/HUD/PerformancePointsCounter.cs
Expand Up @@ -42,12 +42,10 @@ public class PerformancePointsCounter : RollingCounter<int>, ISkinnableDrawable

private const float alpha_when_invalid = 0.3f;

[CanBeNull]
[Resolved(CanBeNull = true)]
[Resolved]
private ScoreProcessor scoreProcessor { get; set; }

[Resolved(CanBeNull = true)]
[CanBeNull]
[Resolved]
private GameplayState gameplayState { get; set; }

[CanBeNull]
Expand Down
7 changes: 5 additions & 2 deletions osu.Game/Screens/Play/SongProgress.cs
Expand Up @@ -73,9 +73,12 @@ public IEnumerable<HitObject> Objects
[Resolved(canBeNull: true)]
private Player player { get; set; }

[Resolved(canBeNull: true)]
[Resolved]
private GameplayClock gameplayClock { get; set; }

[Resolved(canBeNull: true)]
private DrawableRuleset drawableRuleset { get; set; }

private IClock referenceClock;

public bool UsesFixedAnchor { get; set; }
Expand Down Expand Up @@ -113,7 +116,7 @@ public SongProgress()
}

[BackgroundDependencyLoader(true)]
private void load(OsuColour colours, OsuConfigManager config, DrawableRuleset drawableRuleset)
private void load(OsuColour colours, OsuConfigManager config)
{
base.LoadComplete();

Expand Down
85 changes: 41 additions & 44 deletions osu.Game/Skinning/Editor/SkinComponentToolbox.cs
Expand Up @@ -2,24 +2,17 @@
// See the LICENCE file in the repository root for full licence text.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using osu.Framework.Allocation;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Framework.Input.Events;
using osu.Framework.Utils;
using osu.Game.Beatmaps;
using osu.Framework.Logging;
using osu.Game.Graphics;
using osu.Game.Graphics.Sprites;
using osu.Game.Graphics.UserInterface;
using osu.Game.Overlays;
using osu.Game.Rulesets;
using osu.Game.Rulesets.Difficulty;
using osu.Game.Rulesets.Mods;
using osu.Game.Rulesets.Scoring;
using osu.Game.Rulesets.UI;
using osu.Game.Screens.Edit.Components;
using osuTK;

Expand All @@ -29,26 +22,19 @@ public class SkinComponentToolbox : EditorSidebarSection
{
public Action<Type> RequestPlacement;

[Cached]
private ScoreProcessor scoreProcessor = new ScoreProcessor(new DummyRuleset())
{
Combo = { Value = RNG.Next(1, 1000) },
TotalScore = { Value = RNG.Next(1000, 10000000) }
};

[Cached(typeof(HealthProcessor))]
private HealthProcessor healthProcessor = new DrainingHealthProcessor(0);
private readonly CompositeDrawable target;

public SkinComponentToolbox()
public SkinComponentToolbox(CompositeDrawable target = null)
: base("Components")
{
this.target = target;
}

private FillFlowContainer fill;

[BackgroundDependencyLoader]
private void load()
{
FillFlowContainer fill;

Child = fill = new FillFlowContainer
{
RelativeSizeAxes = Axes.X,
Expand All @@ -57,40 +43,46 @@ private void load()
Spacing = new Vector2(2)
};

reloadComponents();
}

private void reloadComponents()
{
fill.Clear();

var skinnableTypes = typeof(OsuGame).Assembly.GetTypes()
.Where(t => !t.IsInterface)
.Where(t => typeof(ISkinnableDrawable).IsAssignableFrom(t))
.OrderBy(t => t.Name)
.ToArray();

foreach (var type in skinnableTypes)
{
var component = attemptAddComponent(type);

if (component != null)
{
component.RequestPlacement = t => RequestPlacement?.Invoke(t);
fill.Add(component);
}
}
attemptAddComponent(type);
}

private static ToolboxComponentButton attemptAddComponent(Type type)
private void attemptAddComponent(Type type)
{
try
{
var instance = (Drawable)Activator.CreateInstance(type);

Debug.Assert(instance != null);

if (!((ISkinnableDrawable)instance).IsEditable)
return null;
if (!((ISkinnableDrawable)instance).IsEditable) return;

return new ToolboxComponentButton(instance);
fill.Add(new ToolboxComponentButton(instance, target)
{
RequestPlacement = t => RequestPlacement?.Invoke(t)
});
Comment on lines +73 to +76
Copy link
Collaborator

Choose a reason for hiding this comment

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

So if I understand this correctly, this add is what actually holds the invariant that only usable components are shown on their corresponding screens, and it works because the synchronous add causes a synchronous load which fails after dependencies are not found in the borrowing container, and therefore the element is finally not added.

I guess it works but I feel some reservations towards that - it feels a bit too magic if I'm honest and needs to be properly xmldoc'd / signposted. Like right now SongProgress are visible when skin editing song select because they have nullable dependencies. The try-catch is also doing a lot of magic - any old other failure that isn't a DI failure will also make the component disappear. I'd be more fine with catching a DI-specific exception and logging any other unexpected one.

Dunno, probably going to wait for @smoogipoo's thoughts on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's okay for now? Definitely needs documentation though.

Copy link
Member

Choose a reason for hiding this comment

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

The try-catch is also doing a lot of magic - any old other failure that isn't a DI failure will also make the component disappear. I'd be more fine with catching a DI-specific exception and logging any other unexpected one.

I also agree with this bit for better safety, DependencyNotRegisteredException should be enough to indicate a DI failure.

}
catch (DependencyNotRegisteredException)
{
// This loading code relies on try-catching any dependency injection errors to know which components are valid for the current target screen.
// If a screen can't provide the required dependencies, a skinnable component should not be displayed in the list.
}
catch
catch (Exception e)
{
return null;
Logger.Error(e, $"Skin component {type} could not be loaded in the editor component list due to an error");
}
}

Expand All @@ -101,6 +93,7 @@ private class ToolboxComponentButton : OsuButton
public override bool PropagateNonPositionalInputSubTree => false;

private readonly Drawable component;
private readonly CompositeDrawable dependencySource;

public Action<Type> RequestPlacement;

Expand All @@ -109,9 +102,10 @@ private class ToolboxComponentButton : OsuButton
private const float contracted_size = 60;
private const float expanded_size = 120;

public ToolboxComponentButton(Drawable component)
public ToolboxComponentButton(Drawable component, CompositeDrawable dependencySource)
{
this.component = component;
this.dependencySource = dependencySource;

Enabled.Value = true;

Expand Down Expand Up @@ -143,7 +137,7 @@ private void load(OverlayColourProvider colourProvider, OsuColour colours)
RelativeSizeAxes = Axes.Both,
Padding = new MarginPadding(10) { Bottom = 20 },
Masking = true,
Child = innerContainer = new Container
Child = innerContainer = new DependencyBorrowingContainer(dependencySource)
{
RelativeSizeAxes = Axes.Both,
Anchor = Anchor.Centre,
Expand Down Expand Up @@ -186,14 +180,17 @@ protected override bool OnClick(ClickEvent e)
}
}

private class DummyRuleset : Ruleset
public class DependencyBorrowingContainer : Container
{
public override IEnumerable<Mod> GetModsFor(ModType type) => throw new NotImplementedException();
public override DrawableRuleset CreateDrawableRulesetWith(IBeatmap beatmap, IReadOnlyList<Mod> mods = null) => throw new NotImplementedException();
public override IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap) => throw new NotImplementedException();
public override DifficultyCalculator CreateDifficultyCalculator(IWorkingBeatmap beatmap) => throw new NotImplementedException();
public override string Description => string.Empty;
public override string ShortName => string.Empty;
private readonly CompositeDrawable donor;

public DependencyBorrowingContainer(CompositeDrawable donor)
{
this.donor = donor;
}

protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) =>
new DependencyContainer(donor?.Dependencies ?? base.CreateChildDependencies(parent));
}
}
}
31 changes: 14 additions & 17 deletions osu.Game/Skinning/Editor/SkinEditor.cs
Expand Up @@ -49,6 +49,7 @@ public class SkinEditor : VisibilityContainer

private Container content;

private EditorSidebar componentsSidebar;
private EditorSidebar settingsSidebar;

public SkinEditor()
Expand Down Expand Up @@ -145,16 +146,7 @@ private void load()
{
new Drawable[]
{
new EditorSidebar
{
Children = new[]
{
new SkinComponentToolbox
{
RequestPlacement = placeComponent
},
}
},
componentsSidebar = new EditorSidebar(),
content = new Container
{
Depth = float.MaxValue,
Expand Down Expand Up @@ -200,7 +192,15 @@ public void UpdateTargetScreen(Drawable targetScreen)
Scheduler.AddOnce(loadBlueprintContainer);
Scheduler.AddOnce(populateSettings);

void loadBlueprintContainer() => content.Child = new SkinBlueprintContainer(targetScreen);
void loadBlueprintContainer()
{
content.Child = new SkinBlueprintContainer(targetScreen);

componentsSidebar.Child = new SkinComponentToolbox(getFirstTarget() as CompositeDrawable)
{
RequestPlacement = placeComponent
};
}
}

private void skinChanged()
Expand All @@ -227,12 +227,7 @@ private void skinChanged()

private void placeComponent(Type type)
{
var target = availableTargets.FirstOrDefault()?.Target;

if (target == null)
return;

var targetContainer = getTarget(target.Value);
var targetContainer = getFirstTarget();

if (targetContainer == null)
return;
Expand Down Expand Up @@ -263,6 +258,8 @@ private void populateSettings()

private IEnumerable<ISkinnableTarget> availableTargets => targetScreen.ChildrenOfType<ISkinnableTarget>();

private ISkinnableTarget getFirstTarget() => availableTargets.FirstOrDefault();

private ISkinnableTarget getTarget(SkinnableTarget target)
{
return availableTargets.FirstOrDefault(c => c.Target == target);
Expand Down