Make NodeSamples editable #23443

OliBomby · 2023-05-08T14:20:31Z

This change allows editing and modding of the IHasRepeats.NodeSamples. Also SamplePointPiece has been upgraded to include a 'addition bank', achieving parity with stable.

Still can't edit the custom sample index for legacy samples though. Not sure if we want to include that in the editor.

This checks two boxes in #12020
Closes #22187
Closes #20595

Speykious · 2023-05-29T09:07:38Z

Given this new sample bank menu:

I guess this now needs to be a dropdown.

Speykious · 2023-05-29T09:11:24Z

The right click menu on a hitobject would need to show the two sample banks as well, right?

peppy · 2023-05-30T04:47:08Z

@OliBomby hoping to get to this soon, but just to confirm forward direction (I think we may have discussed this so forgive me):

This is adding parity back, but the end goal will be to split the "addition bank" to be applied on a per-sample level, correct? This will have implications on the UI when we get to that point (basically the stuff in this PR will be replaced), so I'm curious if you already explored that direction and confirmed it's going to be a much larger scoped change before we go ahead with this smaller piece.

…an implementing locally

peppy · 2023-05-30T05:08:40Z

A few initial things:

Please check 114f12a. I've moved the local logic of deciding which bank to use on creating new additions to the common method, since it felt awkward doing the same thing in multiple places when this method was intended to cover those cases.

Maybe a limitation of the implementation, but attempting to change the addition bank when there are no additions present will reset it back to the "normal" bank. Either this needs to be fixed, or the text box should be disabled when there are not yet any additions. I'd probably go with the latter as it feels like a better UX.

osu.2023-05-30.at.05.07.42.mp4

There's no test coverage of the new behaviour. It'd be nice to have at least a bit.

peppy

Initial pass, see attached feedback.

OliBomby · 2023-05-30T05:14:29Z

@OliBomby hoping to get to this soon, but just to confirm forward direction (I think we may have discussed this so forgive me):

This is adding parity back, but the end goal will be to split the "addition bank" to be applied on a per-sample level, correct? This will have implications on the UI when we get to that point (basically the stuff in this PR will be replaced), so I'm curious if you already explored that direction and confirmed it's going to be a much larger scoped change before we go ahead with this smaller piece.

Yes, the intention is to eventually replace this UI so you can apply banks on a per-sample level. The reason I didn't go that direction yet is because the file encoding hasn't quite caught up yet, so saving banks on a per-sample level isn't possible with the legacy encoder.

The purpose of this PR is to bring parity with stable so people can edit hitsounds just like on stable (minus the custom samples).

peppy · 2023-05-30T05:48:38Z

Cool, we're on the same page then.

Make sure to review my follow-up comment with some actionable feedback.

OliBomby · 2023-05-30T14:33:30Z

I guess this now needs to be a dropdown.

@Speykious I guess making it a dropdown is alright, since the purpose right now is only to bring parity with stable. Though eventually it will need to be a textbox again.

The right click menu on a hitobject would need to show the two sample banks as well, right?

I don't think this bank context menu is necessary in the first place. You can just use the hitsound sample point thingy below the object on the timeline.

Please check 114f12a. I've moved the local logic of deciding which bank to use on creating new additions to the common method, since it felt awkward doing the same thing in multiple places when this method was intended to cover those cases.

@peppy Thank you for reviewing my PR! I agree with the changes, this is much cleaner.

Maybe a limitation of the implementation, but attempting to change the addition bank when there are no additions present will reset it back to the "normal" bank. Either this needs to be fixed, or the text box should be disabled when there are not yet any additions. I'd probably go with the latter as it feels like a better UX.

Yeah it's impossible to add an additions bank when there are no additions to add the bank to. I agree with disabling the option in such case.

There's no test coverage of the new behaviour. It'd be nice to have at least a bit.

I'll add some simple test cases to make sure that using these controls actually updates the hitsounds.

@peppy Should I invert the colours of the sample point pieces on sliders? so the pieces on node samples are pink and the piece for the main hitobject Samples (controlling the sliderbody hitsound) is purple. I feel like that would be more intuitive since for sliders and circles the pink sample point pieces would then have the same practical effect.

peppy · 2023-05-30T14:44:25Z

Should I invert the colours of the sample point pieces on sliders?

Can you take a screenshot showing what you mean?

OliBomby · 2023-05-30T14:59:16Z

Can you take a screenshot showing what you mean?

^ Current implementation

^ Suggested change

I'm aware that there are always two sample point pieces overlapping at the sliderhead. I'm not really sure what to do about that. You could make them narrower and put them next to eachother like in my follow-up PR, or merge them into one piece somehow, or just remove the piece for the sliderbody bank entirely.

OliBomby · 2023-05-31T14:26:26Z

I just realized 114f12a broke some special logic in SamplePointPiece. It's actually supposed to prioritize inheriting the bank from the specific nodesample instead of the samples of the hitobject. If you only have h.CreateHitSampleInfo(sampleName) it will fetch the bank from the sliderbody hitsound when adding an addition to a slider repeat which is wrong.

Also I cant disable the additions bank textbox. When clicking a sample point piece with no additions you want the addition bank textbox to be disabled and show the value it would get once an addition is added in gray. However if you disable the textbox when the drawable is created it will throw an exception because the textbox tries to change the bindable value on its LoadCompleted method, because it has a text value given. I cant find a suitable timing to disable the textbox and not cause it to throw exceptions or be empty when you first see it.

bdach · 2023-09-07T07:21:31Z

@Tais993 work not on https://github.com/orgs/ppy/projects/13 is currently being heavily deprioritised. I hope you understand.

peppy · 2024-01-25T07:17:13Z

@OliBomby wanna fix conflicts on this one just to keep it up-to-date?

OliBomby · 2024-01-25T10:37:56Z

@OliBomby wanna fix conflicts on this one just to keep it up-to-date?

Sure

bdach · 2024-06-06T11:44:15Z

@OliBomby I fixed conflicts here, please double check resolution.

bdach · 2024-06-06T11:48:22Z

osu.Game/Rulesets/Objects/HitObject.cs

+            // As per stable, all non-normal "addition" samples should use the same bank.
+            if (sampleName != HitSampleInfo.HIT_NORMAL)
+            {
+                if (Samples.FirstOrDefault(s => s.Name != HitSampleInfo.HIT_NORMAL) is HitSampleInfo existingAddition)
+                    return existingAddition.With(newName: sampleName);
+            }


Can you explain why this change was made in this PR and what it impacts?

It does not seem immediately relevant or useful for the stated goal of it ("making node samples editable"), and I am going to have to go double-check stable to confirm that whatever this is claiming is true, which I'm not even sure how to do right now because my brain immediately shuts down at the word salad that is the comment attempting to explain this.

This change was made by @peppy

It seems to be a code duplication cleanup for logic that determines the correct bank to apply to a new hitsample. For example if you are making a new hitwhistle sample which is an 'addition', then this code tries to find an existing addition sample to take the bank name of.

In stable, all addition samples have the same bank, because you can only set one bank for all addition samples. This doesn't have to be true in lazer, but currently lazer behaves the same as stable for this in that you can only set one bank for all addition samples.

Duplication from where though? If it was deduplication I'd expect the same chunk of code to be gone from somewhere else. But I haven't seen any of that in this diff.

Read #23443 (comment) for the reasoning.
I think this PR introduced the need for the chunk of code that was duplicated and subsequently deduplicated, that's why you don't see it in the final diff.

Well the thing is that I'm not confident that it's a safe change to make. Yes CreateHitSampleInfo() is frequently used in editor contexts, but it also is frequently used in non-editor contexts. Such as:

osu/osu.Game.Rulesets.Osu/Objects/Spinner.cs

Line 94 in a8a2e54

: new SpinnerBonusTick { StartTime = startTime, SpinnerDuration = Duration, Samples = new[] { CreateHitSampleInfo("spinnerbonus") } });

osu/osu.Game.Rulesets.Taiko/UI/DrumSampleTriggerSource.cs

Lines 43 to 52 in a8a2e54

var baseSample = hitObject.CreateHitSampleInfo(hitType == HitType.Rim ? HitSampleInfo.HIT_CLAP : HitSampleInfo.HIT_NORMAL);

if (strong)

{

PlaySamples(new ISampleInfo[]

{

baseSample,

hitObject.CreateHitSampleInfo(hitType == HitType.Rim ? HitSampleInfo.HIT_WHISTLE : HitSampleInfo.HIT_FINISH)

});

}

So as far as I can tell this probably changes sample playback behaviour for some objects. So I'd like to know whether this was a change made specifically for the editor flows, or something that is supposed to be valid for everything everywhere.

@peppy mind providing context / commentary on this when able?

I did some analysis:

It seems the change allows SpinnerBonusTick to receive an addition bank if the spinner has additions.
This is probably incorrect because spinnerbonus-max and spinnerspin seem to both always use the primary bank.

For taiko Hit and StrongableHitObject, this change allows clap and finish samples added by the Type and IsStrong bindables to use the addition bank.
I think this change is desirable in this case because the user is able to change the addition bank (since this PR), so probably expects all the whistle, clap, and finish samples to have the same addition bank. It would be weird if for example the user sets the clap sample to the normal bank, sets IsStrong to true, and finds a finish sample with the soft bank from the primary bank.

For DrumSampleTriggerSource, this change makes it possible to play the rim and centre samples on different banks if the most valid object has different primary and addition banks. It adds a little bit of mapper intention but maybe we don't want this.

I'm not familiar with taiko hitsounding, but it seems like taiko always plays a single sample per hit so having two different banks on the same object might be a silly idea. If that's the case then we should probably go back and always take the bank of the hitnormal in taiko.

So I looked into this myself a bit further and well.....

Here's a test map that I did: Moderat - Rusty Nails - Copy.zip

The thing to note here is that I had all of the sample files just be a speech synthesizer that says the name of the bank. As such:

For the spinner thing, now I believe it to be an irrelevant concern because stable did not support banks for spinnerbonus at all. So the fact this even works is a lazer-only thing and not really advertised as supported anywhere, so I'm fine with leaving it as "undefined behaviour".

For the taiko thing... well let's just say neither master nor this PR appear to follow what stable does.

On stable, I can't get the drum to produce anything audible other than "normal". Which means stable does not appear to care about sample bank on individual objects at all. The only thing that appears to have an effect there is the "default sample settings" list thing in setup (it'll be all normal, all soft, or all drum, always).

On master, every object with additions will follow its "primary" / hitnormal bank,

while on this branch, every object with additions will follow its "addition" bank.

I'm not sure what to think of the taiko thing. I'll go ask the taiko NAT maybe for some advice because I'm stupid and know nothing about mapping, but I'm not sure this needs to be a blocking concern for this PR.

With that said I'm going to just probably be looking to get this in and address any issues later. @ppy/team-client any objections to that?

I'm not sure what to think of the taiko thing.

As far as I've experienced, taiko players they expect to hear a taiko drum sound and hitsounds either take a backseat being "whatever" or (as we did in lazer) are more present to the point of annoying them.

With that said I'm going to just probably be looking to get this in and address any issues later. @ppy/team-client any objections to that?

Sounds fine.

osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs

osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs

bdach · 2024-06-06T12:01:58Z

osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs

+                    updateAdditionBank(val.NewValue);
+                    updateAdditionBankPlaceholderText();
+                });
+                additionBank.OnCommit += (_, _) => updateAdditionBankText();


Not much has simplified here. Speaking from personal experience (and I've made the same mistake myself), having multiple update methods like updateAdditionBankText and updateAdditionBankVisual and updateAdditionBank (with a bool) is not good news. Ideally you'd want just one updateState that gets everything correct.

Can you explain what the issues are here before I try and go in with a hammer to simplify this?

…o edit-nodesample

bdach · 2024-06-10T10:17:42Z

I've changed it to dark pink for the sliderbody hitsound and normal pink for the node samples.

I felt that the contrast with this between the dark pink bg and the black text was pretty dreadful so I lightened it up a touch in 7d5dc75:

The only remaining concern to me with this is this review thread. If we can revert this change / localise it to the editor, or justify its correctness in all contexts somehow, then I think this is good to go.

OliBomby · 2024-06-10T10:58:36Z

I like the change to the contrast of the alternative colour.

The only remaining concern to me with this is #23443 (comment). If we can revert this change / localise it to the editor, or justify its correctness in all contexts somehow, then I think this is good to go.

I'll look into it to see what this change exactly means for all the usages of this method.

…while not having an editable addition bank

smoogipoo · 2024-06-17T03:38:14Z

osu.Game.Rulesets.Catch/Objects/JuiceStream.cs

@@ -76,6 +76,8 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
        {
            base.CreateNestedHitObjects(cancellationToken);

+            this.PopulateNodeSamples();


Is every single implementation of CreateNestedHitObjects going to have to call this?

Every IHasRepeats would need to I suppose. Not entirely sure how to fix that without introducing eldritch abstract entities.

@smoogipoo do you have strong opinions on this?

not at the moment, no

bdach · 2024-06-17T06:28:57Z

One other thing I didn't really consider (which was pointed out to me in a reddit discussion) is that this UI does not work with buzz sliders:

@OliBomby thoughts on how we can remedy this? I wouldn't block this PR over this but yeah.

OliBomby · 2024-06-17T09:25:58Z

I can think of a few workarounds but the UX leaves a lot to be desired. As a workaround you can zoom in really far in the timeline to create more space so the sample points dont overlap anymore.
You could also multiselect another slider and edit the body hitsounds of the other to hit the buzz slider body at the same time.

The information density of buzz sliders is pretty hard to handle on the timeline. Maybe a fix could be to merge the sample point pieces into a single expanding box whenever they overlap too much. Clicking it would then expend it down vertically and show every node to be edited. This still would work for super long buzz sliders though.

Other idea could be to cycle between overlapped sample point pieces by clicking in the same spot multiple times. It would need an extra indicator to make it clear which samples are currently open.

bdach · 2024-06-17T09:31:12Z

The more these sorts of issues pop up, the more I think we need a new dedicated screen for hitsounding. A few of the ideas you proposed popped up in my head too but I'm not super sure they are going to work well in practice.

OliBomby · 2024-06-17T10:56:49Z

The more these sorts of issues pop up, the more I think we need a new dedicated screen for hitsounding. A few of the ideas you proposed popped up in my head too but I'm not super sure they are going to work well in practice.

I want to add a dedicated screen for hitsounding too, alongside being able to do basic hitsounds in the compose screen. This is because some people prefer hitsounding while mapping.

A dedicated screen for hitsounding will definitely make hitsounding a lot better of an experience for anything that isn't just adding simple whistle, finish, claps.

OliBomby added 11 commits May 8, 2023 11:55

Remove dependency on SamplesBindable

244ca4e

remove editor beatmap

b447018

create NodeSamplePointPiece

cb7b747

fix updating wrong samples

32f945d

Add node sample pieces to timeline blueprint

e945846

Abbreviate common bank names on timeline

3b5bae7

fix assigned hitsounds dont have bank or volume

88d840a

allow editing additions in sample point piece

4c36530

cleanup code duplication

bb8285e

fix crash on multiselect and node sample piece popup

7260dca

add addition bank

dd0fcea

pull-request-size bot added the size/L label May 8, 2023

OliBomby mentioned this pull request May 8, 2023

Redesign timeline sample pieces #23445

Open

1 task

peppy self-requested a review May 30, 2023 04:47

peppy added 2 commits May 30, 2023 14:03

Merge branch 'master' into edit-nodesample

4a81d06

Adjust CreateHitSampleInfo to handle additions correctly, rather th…

114f12a

…an implementing locally

peppy requested changes May 30, 2023

View reviewed changes

Invert colors

7a46b7b

OliBomby added 2 commits May 31, 2023 16:28

Fix regressed bank inheriting behaviour on node samples

b7bc49b

Make relevantObject and relevantSamples instance variables

fede432

OliBomby mentioned this pull request Jan 24, 2024

Undo changes to sliders in editor will also change and lock their sampleset to normal set #25488

Closed

Merge remote-tracking branch 'upstream/master' into edit-nodesample

c2d57dc

bdach added 2 commits June 6, 2024 13:42

Merge branch 'master' into edit-nodesample

86b1307

Use established constants

7a8a37d

Fix obsoletion warning

dd9c77d

bdach reviewed Jun 6, 2024

View reviewed changes

OliBomby and others added 6 commits June 6, 2024 14:48

Fix wasteful recreating of container

71ce400

Merge branch 'edit-nodesample' of https://github.com/OliBomby/osu int…

ba78b87

…o edit-nodesample

undo useless change

fcc8671

Add some in-depth xmldoc to GetSamples

e873698

Restructure popover updates to be more centralised

0efa028

Use slightly lighter shade of pink for alternative colour

7d5dc75

OliBomby added 2 commits June 11, 2024 21:20

remove code already covered by updatePrimaryBankState

12dd607

Fixed samples without additions contributing to common addition bank …

869cd40

…while not having an editable addition bank

bdach added the area:editor label Jun 13, 2024

bdach approved these changes Jun 13, 2024

View reviewed changes

smoogipoo reviewed Jun 17, 2024

View reviewed changes

peppy approved these changes Jun 18, 2024

View reviewed changes

peppy merged commit 316125d into ppy:master Jun 18, 2024
13 of 17 checks passed

OliBomby deleted the edit-nodesample branch June 18, 2024 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make NodeSamples editable #23443

Make NodeSamples editable #23443

OliBomby commented May 8, 2023 •

edited

Loading

Speykious commented May 29, 2023

Speykious commented May 29, 2023

peppy commented May 30, 2023

peppy commented May 30, 2023

peppy left a comment

OliBomby commented May 30, 2023

peppy commented May 30, 2023

OliBomby commented May 30, 2023

peppy commented May 30, 2023

OliBomby commented May 30, 2023

OliBomby commented May 31, 2023 •

edited

Loading

bdach commented Sep 7, 2023

peppy commented Jan 25, 2024

OliBomby commented Jan 25, 2024

bdach commented Jun 6, 2024

bdach Jun 6, 2024

OliBomby Jun 6, 2024

bdach Jun 6, 2024

OliBomby Jun 6, 2024

bdach Jun 10, 2024 •

edited

Loading

OliBomby Jun 11, 2024

bdach Jun 13, 2024 •

edited

Loading

peppy Jun 18, 2024

bdach Jun 6, 2024

bdach commented Jun 10, 2024

OliBomby commented Jun 10, 2024

smoogipoo Jun 17, 2024 •

edited

Loading

bdach Jun 17, 2024

peppy Jun 17, 2024

smoogipoo Jun 17, 2024

bdach commented Jun 17, 2024 •

edited

Loading

OliBomby commented Jun 17, 2024 •

edited

Loading

bdach commented Jun 17, 2024

OliBomby commented Jun 17, 2024

	var baseSample = hitObject.CreateHitSampleInfo(hitType == HitType.Rim ? HitSampleInfo.HIT_CLAP : HitSampleInfo.HIT_NORMAL);

	if (strong)
	{
	PlaySamples(new ISampleInfo[]
	{
	baseSample,
	hitObject.CreateHitSampleInfo(hitType == HitType.Rim ? HitSampleInfo.HIT_WHISTLE : HitSampleInfo.HIT_FINISH)
	});
	}

Make NodeSamples editable #23443

Make NodeSamples editable #23443

Conversation

OliBomby commented May 8, 2023 • edited Loading

Speykious commented May 29, 2023

Speykious commented May 29, 2023

peppy commented May 30, 2023

peppy commented May 30, 2023

peppy left a comment

Choose a reason for hiding this comment

OliBomby commented May 30, 2023

peppy commented May 30, 2023

OliBomby commented May 30, 2023

peppy commented May 30, 2023

OliBomby commented May 30, 2023

OliBomby commented May 31, 2023 • edited Loading

bdach commented Sep 7, 2023

peppy commented Jan 25, 2024

OliBomby commented Jan 25, 2024

bdach commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach commented Jun 10, 2024

OliBomby commented Jun 10, 2024

smoogipoo Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach commented Jun 17, 2024 • edited Loading

OliBomby commented Jun 17, 2024 • edited Loading

bdach commented Jun 17, 2024

OliBomby commented Jun 17, 2024

OliBomby commented May 8, 2023 •

edited

Loading

OliBomby commented May 31, 2023 •

edited

Loading

bdach Jun 10, 2024 •

edited

Loading

bdach Jun 13, 2024 •

edited

Loading

smoogipoo Jun 17, 2024 •

edited

Loading

bdach commented Jun 17, 2024 •

edited

Loading

OliBomby commented Jun 17, 2024 •

edited

Loading