Move spinner sample point to the end time on the timeline

[OliBomby]

closes #29338

I had to add an interface to prevent this timeline change also applying to taiko rolls, ctb banana showers, and mania hold notes.

[OliBomby]

While spinnerspin doesn't have bank, what it does have is volume. So it feels misleading that changing a sample piece at the end of the spinner will change the volume of spin.

Yeah, I think having the sample piece for spinner spin volume at the end or even the start is kinda misleading. Ideally you want a separate sample piece for the spinner spin sample, similar to how sliders currently work, but that's out of scope for this PR. Hitsounders mostly care about the hitsound at the end for spinners, so I'd say this is overal an improvement.

Secondly, the entire API design is a bit weird. The structure is such that Samples generally play at the end time of the object (because that's when it usually becomes hit). So I'd expect the sample point to be at the end by default for objects that have duration, and not the start. The fact that it's not the case for taiko/catch/mania is basically a bunch of coincidences at once (one being that the taiko drum/mania keys make the noises rather than the individual objects, and that catch juice streams/banana showers aren't objects on their own so much as collections of ones).

I hadn't thought about this until you mentioned it, but the Samples generally playing at the end time of the object is such a weird behaviour. Generally in instruments and rhythm games you have no sounds on the release of a keypress. Spinners feel like the exception to me.

[smoogipoo]

I'm quite confused by this...

In my mind samples are assumed to be at end time by default. This is because in general hitobjects aren't hit until their nested hitobjects have all been judged, and those are assumed to span the entire duration of the hitobject.

All current implementations of hitobjects with duration have some form of:

void CheckForResult()
{
    if (Time < EndTime)
        return;
}

https://github.com/ppy/osu/blob/dd675fef45db1693db4087f088f99a62764021b7/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs#L295-L296 https://github.com/ppy/osu/blob/dd675fef45db1693db4087f088f99a62764021b7/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs#L254-L255

Same thing, different way of writing:

https://github.com/ppy/osu/blob/dd675fef45db1693db4087f088f99a62764021b7/osu.Game.Rulesets.Taiko/Objects/Drawables/DrawableDrumRoll.cs#L143-L144 https://github.com/ppy/osu/blob/dd675fef45db1693db4087f088f99a62764021b7/osu.Game.Rulesets.Catch/Objects/Drawables/DrawableCatchHitObject.cs#L65-L66

Mania differs because it doesn't have a sample on the hold note itself.

It's not a hard-line guarantee, but I'd say it's a common implementation. Reading more above, it seems I'm not the only one that independently came to this conclusion... I don't foresee this being used outside of the editor, if it even needs to exist in the first place, so I would much prefer it being specialised even to the point of specifying it's editor-specific in the interface name. In that case, I would also prefer [0;1] range rather than absolute time.

[OliBomby]

It's not a hard-line guarantee, but I'd say it's a common implementation. Reading more above, it seems I'm not the only one that independently came to this conclusion... I don't foresee this being used outside of the editor, if it even needs to exist in the first place, so I would much prefer it being specialised even to the point of specifying it's editor-specific in the interface name. In that case, I would also prefer [0;1] range rather than absolute time.

So I see two possibilities: Either I revert to the [0;1] range, or I remove the interface entirely and accept that taiko rolls, ctb banana showers, and mania hold notes all should have their Samples at the end time.

[bdach]

I don't foresee this being used outside of the editor, if it even needs to exist in the first place, so I would much prefer it being specialised even to the point of specifying it's editor-specific in the interface name. In that case, I would also prefer [0;1] range rather than absolute time.

This is a bit of a non-sequitur to me (why does editor usage impact whether this needs to be [0;1] range or not?) but ok, I guess I could live with it. I just feel like absolute time is a nicer thing to use in this context given everything else on hitobjects uses absolute time.

I dunno, I have no strong opinions, but if I were to lean in a specific direction then I feel like this PR could just be closed to no harm in general because as stated above even for osu! spinners the sample point being at the start of the object happens to make sense anyway because of spinnerspin.

[peppy]

I think this might be best left until we have the sample editor setup, but even so we'll likely need a similar interface.

But is there any valid case for putting a sample anywhere but the start or end (I can't think of one)? If not, I'd prefer this is just a bool ShowSampleAtEndTime.

As mentioned by @bdach, the sample settings also affects the spinner sound, so I dunno how valid this is. I don't think many people are going to care that it shows at the start so maybe we should leave it for now (until we have a sample editor where we want to show samples precisely when they happen so it matches visually with what you hear when using the editor).

[bdach]

But is there any valid case for putting a sample anywhere but the start or end (I can't think of one)? If not, I'd prefer this is just a bool ShowSampleAtEndTime.

I really don't like that.

Let me just emphasise one thing: we're adding a thing to a hitobject class. A thing that more or less amounts to controlling visual layout of editor, no less. If this were an actual absolute time instant of sample playback, I'd accept it, but doesn't this living in a hitobject class feel like a blatant break of layering? That stuff should be nowhere near Spinner.

[peppy]

Yeah sure, I agree with that. It should be something in the ruleset API which allows customising the display.

I'd be fine with closing this for now and revisiting as part of a sample editor implementation. It's such a minor thing that I don't want to spend too much time over it.

[smoogipoo]

It wasn't obvious, but my suggestion was basically saying "this is a hack". And if we need it at all, then I want to simplify it for the editor's specific purpose. I was also going to followup to your response suggesting the same bool value, so I agree with that.

I'm not sure what "layering" this breaks. If there was another place to put the interface, then I'd agree with it (same as the other bool added to Ruleset just before). But we don't currently have a Ruleset.CreateTimeline() or whatever.

So again this is just a hack, and I'd hope either the interface doesn't exist at all (accept current display location), or it's a once-off.

[smoogipoo]

I'm going to go ahead and close this as we seemingly all disagree with it.

[bdach]

But we don't currently have a Ruleset.CreateTimeline() or whatever.

I think we'll need it eventually, so make of that what you will.