Add support for LayeredHitSounds skin option by Fire937 · Pull Request #8683 · ppy/osu

Fire937 · 2020-04-08T14:28:35Z

The current behaviour of osu!lazer is to add the hitnormal sample on top of the other samples for every single skin/beatmap/ruleset/hitobject, even in osu!mania, that is what causes the issue. Moreover osu!stable uses a LayeredHitSounds skin property that tells osu! whether it should add the hitnormal sample artificially or not based on the skin used (defaults to yes). LayeredHitSounds is also ignored when in osu!mania mode: osu!stable never adds the hitnormal sample artificially in osu!mania mode.

Currently the LayeredHitSounds property is included in LegacySkinConfiguration, however i am not sure about whether it belongs here or not. Maybe it should be moved to SkinInfo.
A ruleset can override the value of AllowsLayeredHitSounds in its ConvertHitObjectParser to disable the LayeredHitSounds property.

Depends on Do not use beatmap hitsounds on timing points without custom sample bank #8649

The 5th argument of a timing point was not read properly. When equal to 0 the sample used should be osu!'s default hitsound. This commit resolves this problem by adding an 'IsCustom' attribute to HitSampleInfo.

@bdach

As suggested by @bdach it makes much more sense to test the IsCustom attribute to LegacySkin, that way SkinnableSound doesn't need to know how the skin is supposed to behave.

The LayeredHitSounds option if set to 1 will force hitnormal to be played along all the other hitsounds on each hitobject. This option is disabled in osu:mania. In order to implement this behaviour, an AllowsLayeredHitSounds attribute was added to the ConvertHitObjectParser so that it knows if it has to artificially add the hitnormal sample or not. LegacySkin can then verify if a hitsample was artificially added thanks to HitSampleInfo's IsLayered attribute. It can then decide to return the hitsample or not based on the value of LayeredHitSounds.

…rk-mania These two branches are tightly related for the most part because of the changes to LegacySkin:GetSample.

LayeredHitSounds was previously in SkinInfo but that caused some issues so it was moved to LegacySkinConfiguration.

The use of a LayeredHitSounds property in Configuration was redundant. The value of the LayeredHitSounds property could already be retrieved from SkinConfiguration:ConfigDictionary.

The LayeredHitSounds property can now be accessed via the LegacySetting enum rather than by string.

bdach

I really dislike how complicated LegacySkin.GetSample() is starting to be. I'd say it needs unit tests at the least, and a refactoring on top to decrease complexity if possible.

bdach · 2020-04-08T18:45:43Z

-                }
-            };
+                    CustomSampleBank = bankInfo.CustomSampleBank,
+                    IsLayered = !type.HasFlag(LegacyHitSoundType.Normal)


I feel like this is backwards. Shouldn't this just use AllowsLayeredHitSounds? If both the flag is set and AllowsLayeredHitSounds is true, won't this wrongly consider the sample non-layered?

The thing is that the beatmap could actually explicitly indicate that the sample to be played (among others) should be hitnormal. In this case, whether AllowLayeredHitSounds is true or false should not affect the fact that the sample is not layered. That means it should always be played even in osu!mania. That's why this problem doesn't appear in other beatmap as they use hitnormal as opposed to this map that uses hitfinish and the others but not hitnormal.
I'm actually concerned about the naming of this flag, maybe IsForced would make more sense?

With application of the above comments, the outer condition can once again be removed and this can remain here.

This test makes sure the layered hitsounds are handled properly by LegacySkin:GetSample. - If the LayeredHitSounds skin property is set to true, the sample should be played whether it is layered or not. - If the LayeredHitSounds skin property is set to false, the sample should be played only if it is not layered.

Makes sure that LegacySkin.GetSample() returns the right samples according to the IsCustom flag.

…o-rework-mania

…-mania

smoogipoo · 2020-04-16T04:57:25Z

+                case LegacySkinConfiguration.LegacySetting legacySetting when legacySetting == LegacySkinConfiguration.LegacySetting.Version:
+                    return SkinUtils.As<TValue>(new Bindable<decimal>(Configuration.LegacyVersion ?? LegacySkinConfiguration.LATEST_VERSION));


this is less readable than the original, and there may be even more legacy settings. please revert.

smoogipoo · 2020-04-16T04:59:51Z

+        /// <summary>
+        /// Whether the sample is custom or if it uses osu!'s default hitsounds.
+        /// </summary>
+        public bool IsCustom { get; set; }


This should not exist. Please update this branch using the changes made in #8756

smoogipoo · 2020-04-16T05:00:36Z

+                // 0 means osu!'s default hitsounds
+                baseInfo.IsCustom = CustomSampleBank != 0;
+


Same here, can be removed

smoogipoo · 2020-04-16T05:03:29Z


        protected bool FirstObject { get; private set; } = true;

+        protected bool AllowLayeredHitSounds { get; set; } = true;


This cannot exist in the parser - it should only exist within the legacy skin itself.

The mania override can be done inside LegacyManiaSkinTransformer.

That is to say, layered hitsounds should always be added.

smoogipoo · 2020-04-16T05:04:00Z

+        /// True if the sample is a <c>hitnormal</c> sample added artificially due to the skin having
+        /// the <see cref="osu.Game.Skinning.LegacySkinConfiguration.LegacySetting.LayeredHitSounds"/> option set.
+        /// </summary>
+        public bool IsLayered { get; set; }


This must only exist within ConvertHitObjectParser.LegacyHitSampleInfo.

smoogipoo · 2020-04-16T05:05:39Z

-                }
-            };
+                    CustomSampleBank = bankInfo.CustomSampleBank,
+                    IsLayered = !type.HasFlag(LegacyHitSoundType.Normal)


With application of the above comments, the outer condition can once again be removed and this can remain here.

smoogipoo · 2020-04-16T05:06:05Z

        public ConvertHitObjectParser(double offset, int formatVersion)
            : base(offset, formatVersion)
        {
+            AllowLayeredHitSounds = false;


See other comments.

smoogipoo · 2020-04-16T05:07:45Z

+namespace osu.Game.Tests.Skins
+{
+    [HeadlessTest]
+    public class TestSceneLegacySkin : OsuTestScene


This testing is inadequate because it doesn't test the result of gameplay loading. You want to add a test to TestSceneHitObjectSamples.

I'm fairly certain the implementation in this PR (inside LegacySkin) will fail, since SkinnableSound will fall back to the default lazer sample store if null is returned.

smoogipoo · 2020-04-16T05:08:52Z

-            foreach (var lookup in sampleInfo.LookupNames)
+            var hsi = sampleInfo as HitSampleInfo;
+
+            if (hsi?.IsLayered == true && GetConfig<LegacySkinConfiguration.LegacySetting, bool>(LegacySkinConfiguration.LegacySetting.LayeredHitSounds)?.Value == false)


This isn't being read from the skin configuration. You need to add a value to the LegacySetting switch above.

peppy · 2020-06-03T02:29:26Z

@Fire937 just looping back – are you intending to apply the remaining review?

Fire937 · 2020-06-05T08:24:52Z

@peppy yes I will try and commit in not too long.

bdach · 2020-06-18T18:17:47Z

Seeing that there stlll has been no changes here in a while, I would be looking to supersede this PR over the coming days. I don't usually do that but this one would take too much work to fix up - I would basically have to revert all of the changes here, and even then there are major issues left unaddressed (such as how to make the sample fallback logic stick to a silent sample if layered hit sounds are to be disabled).

Fire937 added 10 commits April 6, 2020 18:04

Make Beatmap Decoder consistent with osu-stable

f0ba07f

The 5th argument of a timing point was not read properly. When equal to 0 the sample used should be osu!'s default hitsound. This commit resolves this problem by adding an 'IsCustom' attribute to HitSampleInfo.

Format code and rewrite innacurate comments

e3bd22c

Format code

d67681d

Fix code style

e3506ff

Merge branch 'master' into audio-rework

70e94e6

Move the verification of a sample's customness to LegacySkin

ec7907a

As suggested by @bdach it makes much more sense to test the IsCustom attribute to LegacySkin, that way SkinnableSound doesn't need to know how the skin is supposed to behave.

Merge branch 'audio-rework' of github.com:Fire937/osu into audio-rework

2eaccab

Merge branch 'audio-rework' of github.com:Fire937/osu into audio-rewo…

9ce8aea

…rk-mania These two branches are tightly related for the most part because of the changes to LegacySkin:GetSample.

Move LayeredHitSounds atttribute

9004c8d

LayeredHitSounds was previously in SkinInfo but that caused some issues so it was moved to LegacySkinConfiguration.

pull-request-size Bot added the size/M label Apr 8, 2020

bdach added the area:skinning label Apr 8, 2020

frenzibyte requested changes Apr 8, 2020

View reviewed changes

Comment thread osu.Game/Skinning/LegacySkinConfiguration.cs Outdated

Comment thread osu.Game/Skinning/LegacySkin.cs Outdated

Comment thread osu.Game/Skinning/LegacySkin.cs Outdated

Comment thread osu.Game/Audio/HitSampleInfo.cs Outdated

Fire937 added 2 commits April 8, 2020 17:32

Make doc more consistent and enhance code style

a4ec96c

Add LayeredHitSounds setting to LegacySetting

958f1f4

frenzibyte reviewed Apr 8, 2020

View reviewed changes

Comment thread osu.Game/Skinning/LegacySkin.cs Outdated

Fire937 added 2 commits April 8, 2020 19:47

Remove LayeredHitSounds from Configuration

d685dd4

The use of a LayeredHitSounds property in Configuration was redundant. The value of the LayeredHitSounds property could already be retrieved from SkinConfiguration:ConfigDictionary.

Add LayeredHitSounds to LegacySetting

21f0e95

The LayeredHitSounds property can now be accessed via the LegacySetting enum rather than by string.

bdach requested changes Apr 8, 2020

View reviewed changes

Fire937 added 6 commits April 9, 2020 03:20

Format code

501e68d

Fix typo

546bdc1

Add test for handling non custom samples

8d6db26

Makes sure that LegacySkin.GetSample() returns the right samples according to the IsCustom flag.

Merge branch 'audio-rework-mania' of github.com:Fire937/osu into audi…

7c6479f

…o-rework-mania

Merge branch 'master' of https://github.com/ppy/osu into audio-rework…

41c9a66

…-mania

Fire937 mentioned this pull request Apr 13, 2020

Do not use beatmap hitsounds on timing points without custom sample bank #8649

Closed

smoogipoo requested changes Apr 16, 2020

View reviewed changes

pull-request-size Bot added size/L and removed size/M labels Apr 17, 2020

smoogipoo mentioned this pull request Jun 8, 2020

mania hitsounds do not work properly #8145

Closed

bdach mentioned this pull request Jun 22, 2020

Add support for layered hit sounds skin option #9340

Merged

1 task

smoogipoo closed this in #9340 Jun 26, 2020

		case LegacySkinConfiguration.LegacySetting legacySetting when legacySetting == LegacySkinConfiguration.LegacySetting.Version:
		return SkinUtils.As<TValue>(new Bindable<decimal>(Configuration.LegacyVersion ?? LegacySkinConfiguration.LATEST_VERSION));

		// 0 means osu!'s default hitsounds
		baseInfo.IsCustom = CustomSampleBank != 0;


		protected bool FirstObject { get; private set; } = true;

		protected bool AllowLayeredHitSounds { get; set; } = true;

Uh oh!

Conversation

Fire937 commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bdach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peppy commented Jun 3, 2020

Uh oh!

Fire937 commented Jun 5, 2020

Uh oh!

bdach commented Jun 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fire937 commented Apr 8, 2020 •

edited

Loading