Firstly, this is intended to be a float division.
Secondly, dividing integers by 0 results in an exception, but dividing
non-zero floats by 0 results in +/- infinity which will be clamped to
the upper range.
In particular, this occurs when the beatmap has 1 hitobject (0 drain
length).
While `EndTime + tickSpacing` _was_ closer to what stable was using, it
could cause undesirable edge cases wherein a tick would be spawned
outside of the drum roll's duration
(see https://github.com/ppy/osu/pull/23768/files#r1222073027).
For this reason, stick with the old code for now, as it is not as
susceptible to that sort of breakage.
There were no usages of more than one column being provided per row, so
it seemed like unnecessarily complexity. I'm currently trying to reduce
complexity so we can improve the layout of the results screen, which
currently has up to three levels of nested `GridContainer`s.
Of note, I can't add backwards compatibility because the method
signature has not changed in `Ruleset` (only the return type). If we do
want to keep compatibility with other rulesets, we could designate a new
name for the updated method.
4a7b011a53 inadvertently unearthed that
nested strong hits could play samples of their own accord, rather than
delegating to `DrumSampleTriggerSource` as they were supposed to. This
was an unfortunate omission due to how the inheritance structure of
`TaikoHitObject` looks like (some irrelevant classes omitted for
brevity):
DrawableTaikoHitObject
DrawableTaikoHitObject<TObject> <-- `GetSamples()` was overridden to empty here
DrawableTaikoStrongableHitObject
DrawableHit
DrawableDrumRoll
DrawableDrumRollTick
DrawableSwell
DrawableSwellTick
DrawableStrongNestedHit <-- all strong nested hits are here => didn't receive `GetSamples()` override
DrawableHit.StrongNestedHit
DrawableDrumRoll.StrongNestedHit
DrawableDrumRollTick.StrongNestedHit
To fix, move the `GetSamples()` override one level higher, to the
non-generic `DrawableTaikoHitObject`, to suppress the spurious sample
playbacks.
The stale reference in the comment was also updated to match current
code.
The rationale is the same as in
6d325651dc. Due to the recursive nature of
`GameplaySampleTriggerSource.GetMostValidObject()`, in the case of
nested hits, drum rolls and drum roll ticks, the nested strong hits
would become the most valid object, and so without propagating
the samples down to that level too, nothing would play.
In d97daee96b, `DrumSampleTriggerSource`
was changed such that in order to play sounds for the user's inputs, the
bank of the normal sound would always be used.
The problem is that in the case of taiko objects which have nested
objects (swells and drum rolls), the samples were not propagated fully
(drum rolls, where only the finish sample was kept, for the purposes of
determining strongability), or not propagated at all (swells) to ticks.
As ticks of both objects are valid return values of
`GetMostValidHitObject()`, this would lead to the drum making no sounds
if the next object was a drum roll or swell, until that drum roll or
swell was completed. To fix, propagate the full set of samples, so that
`DrumSampleTriggerSource` can retrieve the normal sound to copy the bank
from.
Note that this may not necessarily reproduce prior behaviour. This is
because it is not guaranteed that all realised samples for a given
hitobject have the same bank - some may have been overriden locally on a
given hitobject. Previously, the bank would have been retrieved from the
sample control point, wherein there is only one possible bank to use;
however, when deciding the sound to play on the basis of a constructed
hitobject, it is possible that there are cases wherein the hitnormal
sample was overridden on that given hitobject, and in such cases, this
PR would make samples _play_, but not necessarily the _same_ samples
as prior to #23308.
If that turns out to be the case, this will have to be revisited.
Until now, these were haphazardly enforce inline in blueprint
implementations. The only thing stopping complete breakage is that
`EndPlacement` wasn't called (too much) from outside the blueprint,
leaving them responsible for their own placement.
By moving this conditional out of the provided paramters to
`EndPlacement`, it allows more flexible usage of that method externally.
Coming in a future PR.
