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 per-ruleset skin layout customisation #22388

Closed
peppy opened this issue Jan 24, 2023 · 5 comments · Fixed by #24387
Closed

Add per-ruleset skin layout customisation #22388

peppy opened this issue Jan 24, 2023 · 5 comments · Fixed by #24387
Assignees
Labels
area:skin-editor area:skinning epic A feature of core importance (and also requiring considerable effort and thought). priority:1 Very important. Feels bad without fix. Affects the majority of users.

Comments

@peppy
Copy link
Member

peppy commented Jan 24, 2023

Right now, a skin layout is saved globally for a skin. This doesn't work great for multiple rulesets, where the playfield may be attached to an edge of the screen and cause overlap. Also, in the new "argon" designs, UI elements have already been customised to often be attached to the playfield – this is also the case with classic skins like the osu!mania default.

This epic replaces existing requests for this:

This could be used to solve issues like:

It would both allow for applying per-ruleset defaults, and give users the option to further customise from those defaults. There should also likely be facilities in the editor to copy full layouts between rulesets (maybe not required if we have clipboard support).

@peppy peppy added area:skinning epic A feature of core importance (and also requiring considerable effort and thought). priority:1 Very important. Feels bad without fix. Affects the majority of users. area:skin-editor labels Jan 24, 2023
@frenzibyte
Copy link
Member

Related: #20694

@peppy
Copy link
Member Author

peppy commented Feb 3, 2023

Pulling out of my recent skin structural write-up for visibility:

Currently we store configuration in individual json files, per target container. This likely won't scale well once we add ruleset rules in (and it also makes migrating over version changes harder).

My initial thoughts is that we should move the full skin configuration into a single json structured file, with "layer" objects that container local information as to how/when they should be used. We could then have four of the same layers specified, each with a different ruleset tag to define when they should be used.

This could also allow for the case where multiple rulesets share a layout (ie. there are no ruleset tags), or at very least allow for migration from global to per-ruleset with less fuss.

@peppy
Copy link
Member Author

peppy commented Feb 6, 2023

@frenzibyte looking again at #20694, I think your first approach is closest to what I had in mind. Did you want to revisit that and see how things play out? I think if you revisit it, also keep in mind what I mentioned in my last comment here. It'll probably be enough to make sure you can easily hook the new structure up to the undo/redo system (#22506).

@peppy
Copy link
Member Author

peppy commented Feb 14, 2023

As an update, frenzi mentioned elsewhere that he's a bit busy at the moment so I'm continuing to work through this.

I've rebased approach 1 at https://github.com/ppy/osu/compare/master...peppy:osu:ruleset-skinning-frenzi-approach-1-rebase?expand=1 and it seems to be working well.

@peppy
Copy link
Member Author

peppy commented Feb 24, 2023

The base work for this has been completed and merged. Remaining tasks are to use this implementation in skin transformers to update the default layouts to match expectations for legacy skins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:skin-editor area:skinning epic A feature of core importance (and also requiring considerable effort and thought). priority:1 Very important. Feels bad without fix. Affects the majority of users.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants