Implement skinning for ruleset components + mania's ColumnStart
#27773
Conversation
Ruleset is encoded into the files themselves. For better or worse, this restores behaviour for container lookups (e.g. per-ruleset Playfield lookup).
| protected override string RulesetPrefix => "catch"; // todo: use CatchRuleset.SHORT_NAME; | ||
|
|
||
| protected override string ComponentName => Component.ToString().ToLowerInvariant(); |
There was a problem hiding this comment.
In all of these GameplaySkinComponentLookup classes, these properties are unused - they were likely from a different time.
| // Lift TestRuleset and ToolboxInvisibleComponent into an isolated assembly containing both types at a top level. | ||
| AssemblyBuilder asmBuilder = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName(Guid.NewGuid().ToString()), AssemblyBuilderAccess.Run); | ||
| ModuleBuilder moduleBuilder = asmBuilder.DefineDynamicModule(asmBuilder.GetName().Name!); | ||
|
|
||
| moduleBuilder.DefineType($"{nameof(ToolboxInvisibleComponent)}", TypeAttributes.Public | TypeAttributes.Class, typeof(ToolboxInvisibleComponent)).CreateType(); | ||
| TypeBuilder rulesetType = moduleBuilder.DefineType("MockRuleset", TypeAttributes.Public | TypeAttributes.Class, typeof(TestRuleset)); | ||
| Ruleset ruleset = (Ruleset)Activator.CreateInstance(rulesetType.CreateType())!; |
There was a problem hiding this comment.
All of these skin editor tests are not very flexible for anything other than integration testing. The intention here was to create an isolated context with the expectation being that this was to be used in more tests from the start, however that didn't end up happening.
However, this is still required specifically because of the "containing both types at a top level" part. See the following code:
osu/osu.Game/Skinning/SerialisedDrawableInfo.cs
Lines 120 to 128 in 94cbe18
IsPublic == false for nested types, such as ToolboxInvisibleComponent. The alternative is to change this to also check for IsPublicNested, but I wasn't sure whether we want that.
|
Just as a starting position, without even looking at the code very much, but only from interacting with the branch: it was my expectation that this was going to allow for a much narrower set of controls. Things that feel incorrect include:
I'll probably skim the code now and see how the structure looks, but generally I'd also be more confident in this being the correct direction if I knew the structure was extendable to something even more constrained than the above (i.e. for instance, in my opinion, osu! hitcircles should be selectable but have no movement/resize/rotation controls at all). |
|
Just for the record, I'm going to also look at your second implementation and make a followup post on that as well, but I do not wish to discuss either in this PR. As I've mentioned to you privately, the way in which this entire thing was approached has been totally backwards. It would have been better to discuss this and what we want out of the skinning system first rather than misunderstanding intentions and implementing something completely orthogonal. |
|
I didn't focus that much on making attempt 1 work because attempt 2 was far better (as I hoped to touch on). I've since fixed multiple issues up with that so would appreciate you giving it a look.
Yeah just leftover, I've cleaned this up in the second attempt branch.
Can you elaborate on this one? It does work, so does it conceptually not work for you?
I was aiming to keep things simple without having a lookup type, but this can be added back. The main thing I was trying to demonstrate was the structure and storage I'd see working. I can imagine multiple ways we could handle multiple key layouts (highly depending on how we want it to be presented to the user), but I don't believe this was covered in this PR and I wasn't focusing on that. |
Attempt 2I'm not going to spend much time on this. Again, it's handling one very specific thing but doesn't do anything to actually support skinning (just configuration). This is not going to work as soon as there's an Also, now there's another layer that does things in a totally different way. Not only do we have |
Can you explain why it won't work? Because it did seem to work in testing.
Yep, that's intentional in the way I structured the proposal. The existing system defines "targets" ( The new system defines "components" which can be configured and (in some cases) moved. These components can exist anywhere (although to keep things simple, let's assume all are within The configuration layer feels like it will be far simpler to handle separately because the goals are different. It feels we're still not seeing eye-to-eye on the goals of the system, because your feedback isn't making too much sense to me. Maybe it would be good to start from a clean slate and outline how we want the system to work, and then take things from there? I'm open to a voice call to go through this. |
Attempt 1
It's "simple" until you have to change every other file in skinning in order to do that. My implementation can actually handle this very simply, because from the very beginning the point has always been to treat That's where my implementation comes from - I'm not doing anything new, I'm using existing structures and everything else comes naturally.
Conceptually this doesn't work for me.
When I think of skin configuration, I believe there has to be a legacy skin implementation that deals very specifically with the legacy configuration and legacy components. Attempt 2
I can tell you that it's not going to work just from reading the code. I haven't actually run it. Here's what I'm referring to: This is your constructor, which iterates once and once ever through all Also, how is this going to work when |
This was just for simplicity, I'm more intending to iron out the storage and structure than the intricacies of implementation. But let's put that aside – again my branches aren't the "way forward", just intended to show potential structures. To clarify, the case you're imagining is: diff --git a/osu.Game.Rulesets.Osu/Skinning/Default/MainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Default/MainCirclePiece.cs
index bcea33f63c..cb75da7bd3 100644
--- a/osu.Game.Rulesets.Osu/Skinning/Default/MainCirclePiece.cs
+++ b/osu.Game.Rulesets.Osu/Skinning/Default/MainCirclePiece.cs
@@ -9,11 +9,12 @@
using osu.Game.Rulesets.Objects.Drawables;
using osu.Game.Rulesets.Osu.Objects;
using osu.Game.Rulesets.Osu.Objects.Drawables;
+using osu.Game.Skinning;
using osuTK.Graphics;
namespace osu.Game.Rulesets.Osu.Skinning.Default
{
- public partial class MainCirclePiece : CompositeDrawable
+ public partial class MainCirclePiece : CompositeDrawable, IConfigurableDrawable
{
private readonly CirclePiece circle;
private readonly RingPiece ring;
@@ -114,5 +115,12 @@ protected override void Dispose(bool isDisposing)
if (drawableObject.IsNotNull())
drawableObject.ApplyCustomUpdateState -= updateStateTransforms;
}
+
+ public bool UsesFixedAnchor { get; set; }
+
+ public void ApplyDefaults()
+ {
+ throw new System.NotImplementedException();
+ }
}
}
where individual skin types (legacy, argon, triangles) have different settings from each other? Are there any other cases you have in mind? These will help build a list of requirements. |
|
I don't necessarily want to butt in too much here and derail this discussion further but I do want to highlight one thing:
We've kinda already talked about this and to a degree I still don't find myself agreeing with that premise which I think may be a factor as to why reaching a conclusion may be difficult. I think HUD skinning and gameplay element skinning are fundamentally different usecases because while users are free to plaster their HUD with whatever they want on there (which is what I think that position is consistent with all of my prior reviews and requests therein - I was willing to give this PR a go as a compromise given that it looked like it was viable to bend the existing system into a such a constrained shape, but only as long as there was universal support from all of us here. |
What I brought up in voice is that I believe rulesets should be able to define their own If you want to consider fairness then I honestly think we should just scrap skinning from gameplay entirely and make it only configuration knobs. Players are able to inject their own arbitrary textures into hitcircles and cheat with things like taikomania, but players adding sprites to static layers on the screen is the sudden blocker for fairness? I can't accept that. In osu!stable we have combo bursts for mania, every X combo a pippi (or maria?) pops out from the sides of the screen. I imagine a future in which a player is able to do that:
No comment. |
Rather than defining a separate `DefaultStageConfiguration` and `LegacyStageConfiguration`, we can create a single-instance `SkinnableDrawable` which is reused. The implementation of `reload()` is a little bit temporary at this stage. The follow-up is to add a `Skin.ConfigureComponent()` or `Skin.GetConfiguration()`. Furthermore, the implementations of `Add()` and `Remove()` are no longer (/were never?) required because the types in this "container" never change. Undo/redo is handled directly by `SkinEditorChangeHandler`.
|
I'm not satisfied with either of the two implementations provided above, so I will be continuing this PR. I was hoping to take things in incremental steps rather than refactoring the entire game in one go, but this is what I've been brought to. I will also begin splitting things out where I find appropriate. |
The most contentious thing in the PR has been the introduction of the `base.GetDrawableComponent()` calls. This follows in the footsteps of "attempt 2" and adds a method specific to configuration. One question I had to answer is "what about the defaults". Consider the following: - The classic skin is curently selected which positions ManiaPlayfield with TopLeft anchors and somewhere towards the left of the screen. - The argon skin is selected which wants Centre anchors and a zeroed position. - A third hypothetical, is that the triangles skin is selected that wants totally separate defaults from all of those. Attempt 2 handled this by exposing an `ApplyDefaults()` method on the `IConfigurableDrawable` (in this case, `ManiaPlayfield` itself). This method was implemented something as such: ``` Anchor = Anchors.TopLeft; Origin = Anchors.TopLeft; Position = Vector2.Zero; Rotation = 0; ``` Now I ask you, dear reader, to tell me why these specific properties need to be reset. Why should `Scale` not also? What about any of the many other properties of `Drawable`? This logic presents an issue to me - that there is something, _somewhere_, doing magical things and we somehow need to be omniscient of that. In this case, we need to know what each skin transformer wants, as well as what what the `SkinEditor` is capable of doing. It turns out `Scale` not being reset is a mistake. Whoops! I foresee this going wrong in the future as well. If the `SkinEditor` receives some additional functionality, we will conscious of the fact that _every single implementation_ will need to be updated to reset that property. Every usage is _required_ to have this implementation. A second minor issue, to me, is that this looks very awkward. I imagine the question would come up at some point of why we need this when we have the constructor in the first place. I think that's a very good question, for which the only answer I have is that "it be like it do". One good property about my original implementation in this PR is that the `Drawable` was always reconstructed and had control of its own defaults. This commit is still not quite as "neat" as that, in my opinion, but at least it avoids the bigger issue above.
|
I'm definitely glad to see the base calls go as they were quite scary to understand complexity-wise but I'll attempt to cast a wider net here because I probably failed to do so the last time properly. To sum up, if I'm getting the intention correctly, with the latest commit the state of affairs is such that from a skin, you can get:
Structurally this feels definitely less bendy-breaky than what was here before but I do find myself having a few questions still. Possibly rhetorical, possibly obvious, possibly ones for later.
After that, my attention is drawn to the callers of The last thing I wanna highlight here - because this is probably overlong already - is how this interfaces with the skin editor. The This is going to work for an ever-present drawable like the mania stage, but is probably not going to work for ephemeral objects like hitcircles or mania notes or even like explosions or whatever - because not only is it going to miss new instances as they appear on screen, but every single instance will get its own dropdown entry which feels pretty wrong given that HUD layer containers are single targets grouping multiple skinnables. So I do wonder how this is going to match expectations in those areas, because I'd expect this new skinning system to service these usecases (and, to be clear, the lack of dynamically instantiated skinnable support is already a qualm I had and continue to have with the I feel like I've typed up a way overlong nerd essay that won't be read but feedback was requested so here's feedback...? Hopefully it's useful? |
|
Not going to add further review just yet, but
I'm not sure we want the skin editor to work for these cases. To edit such elements, my plan was to add new "scene"s to the skin editor which have each of the elements on screen (always) to allow editing, rather than having the user try to click them during gameplay. A ruleset would be able to create one or more scenes to show game components in a grid or otherwise where the user can easily make configuration changes to them. |
|
One reason I was pushing for interfaces (which I mentioned on voice but want to convey in text as I think it is quite important) is that I really dislike relying on base classes / inheritance as a requirement for behaviours. As one example, I was trying to proof-of-concept adding a setting to |
You do it just like you add settings to any other (Keep in mind that |
|
Also, I don't want to harp on too much longer than I already have, but this is very similar to how you could also do this with I hope this illustrates two things:
|
Encountered this when flipping back and forth between an implementating using `SkinnableDrawable` and `SkinnableContainer` via the same lookup key.
|
I guess if that works, just feels a bit ugly (and not how I really intended |
|
What do you want me to say? That less code is prettier? Sure? But does it work? This PR's taken the energy and motivation I've had to work on this away from me, and I think there are still issues with things like revert to default that would've been handled correctly by my original changes, that I don't possibly see how to handle nicely here. I'm going to close this PR to reduce my own stress. |
Resolves #20252
ISerialisableDrawablesthat are part ofSkinnableDrawbles can now be skinned.2024-04-03.04-41-42.mp4
To demonstrate, I've implemented (and bundled) legacy and non-legacy skinning of mania's
ColumnStartconfig. Note that this implementation is implemented as an overlay component because theStageitself is not easily skinnable.I created this diagram to visualise the structure. Maybe it'll be useful to someone else...
