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

Refactor structure and naming of skin-related classes #22648

Merged
merged 19 commits into from Feb 17, 2023

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Feb 15, 2023

This is a full pass on the skin namespace, focused on renaming, documenting and generally knocking some (final?) sense into the structure and understandability before I attempt to make things more complicated.

Note that I've intentionally avoided changing any naming or namespaces which would break existing usages / serialisation.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Difficult diff to get through but that was to be expected. Generally feel that this is a net gain in legibility. Left some minor comments, nothing too major - whether they're actionable is mostly up to you.

osu.Game/Skinning/ISkinSource.cs Outdated Show resolved Hide resolved
osu.Game/Skinning/ISerialisableDrawable.cs Outdated Show resolved Hide resolved
osu.Game/Skinning/SerialisableDrawableExtensions.cs Outdated Show resolved Hide resolved
osu.Game/Skinning/SkinComponentsContainer.cs Show resolved Hide resolved
osu.Game/Skinning/LegacyBeatmapSkin.cs Outdated Show resolved Hide resolved
osu.Game/Skinning/GameplaySkinComponentLookup.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented Feb 16, 2023

I think I'm more or less fine with this now, save for the one remaining review thread - but just so I know for the near future (and can maybe expedite this a bit faster without having to go through the usual call-and-response), does this review request mean that you want an approval from @frenzibyte specifically here before merging?

@peppy
Copy link
Sponsor Member Author

peppy commented Feb 17, 2023

does this review request mean that you want an approval from @frenzibyte specifically here before merging?

I was hoping for one, but I don't think it should hold things up so feel free to bypass it. This is going to be a dependency for all my forward work, so I'd prefer it merged ASAP so I can continue moving forward unencumbered.

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

Reads pretty well to me. Nice one 👍🏻. @bdach requesting a final review from you since you've reviewed this before.

@bdach bdach enabled auto-merge February 17, 2023 19:59
@bdach bdach merged commit 23a636b into ppy:master Feb 17, 2023
@peppy peppy deleted the skinnable-clean-up-documentation branch February 20, 2023 02:55
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.

None yet

3 participants