Skip to content

Prevent rank display shown in skin editor toolbox from playing samples #33503

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

Merged
merged 2 commits into from
Jun 6, 2025

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 6, 2025

Add toggle for rank change sound playback to default rank display

Because some people don't seem to like it. Defaults to on still.

Prevent rank display shown in skin editor toolbox from playing samples

Closes #33456.

Not sure how to feel about that change but I don't see many other alternatives. Trying basically any solution with DI runs against the problem that the dependencies are borrowed wholesale from the screen that the component is supposed to work in, so trying things like implementing ISamplePlaybackDisabler on SkinEditor just straight up fails because resolving via that interface from toolbox buttons returns Player and not SkinEditor.

Because some people don't seem to like it. Defaults to on still.
@bdach bdach self-assigned this Jun 6, 2025
@bdach bdach added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Jun 6, 2025
@@ -45,6 +51,9 @@ private void load()
RelativeSizeAxes = Axes.Both
},
};

if (skinEditor != null)
Copy link
Member

Choose a reason for hiding this comment

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

I guess the alternative (which I would have chosen personally) is resolving Player. I'm probably fine with both, but did you consider this one?

Copy link
Collaborator Author

@bdach bdach Jun 6, 2025

Choose a reason for hiding this comment

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

As in, you mean inverting this logic and attempting to resolve Player and disabling playback if it's null?

That won't work because of the reason I already mentioned in the OP:

Trying basically any solution with DI runs against the problem that the dependencies are borrowed wholesale from the screen that the component is supposed to work in

Player will successfully resolve even if the display is being shown in the toolbox because of this borrowing.

Copy link
Member

Choose a reason for hiding this comment

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

I see.

Let's stick with this for now. Only other thing I'd propose is a dedicated interface, but we can adjust if there's more usages of this.

@bdach bdach force-pushed the rank-change-sample-toggling branch from fd3514c to e02cd28 Compare June 6, 2025 11:18
@peppy peppy merged commit 99f57d2 into ppy:master Jun 6, 2025
8 of 10 checks passed
@bdach bdach deleted the rank-change-sample-toggling branch June 6, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:skin-editor area:skinning audio next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rank change sfx is still playing even if you removed DefaultRankDisplay while in skin editor.
2 participants