-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix sample banks not transferring when adjusting via editor #23564
Changes from all commits
83dcd78
d9ae822
31fff72
8528fca
ebce39c
a8bc337
dc51d5e
510ebe1
ee52225
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -226,12 +226,16 @@ internal class LegacySampleControlPoint : SampleControlPoint, IEquatable<LegacyS | |
|
||
public override HitSampleInfo ApplyTo(HitSampleInfo hitSampleInfo) | ||
{ | ||
var baseInfo = base.ApplyTo(hitSampleInfo); | ||
|
||
if (baseInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacy && legacy.CustomSampleBank == 0) | ||
return legacy.With(newCustomSampleBank: CustomSampleBank); | ||
if (hitSampleInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacy) | ||
{ | ||
return legacy.With( | ||
newCustomSampleBank: legacy.CustomSampleBank > 0 ? legacy.CustomSampleBank : CustomSampleBank, | ||
newVolume: hitSampleInfo.Volume > 0 ? hitSampleInfo.Volume : SampleVolume, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably good to add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, given what you found above I don't think this is necessary as it's a strictly legacy flow. |
||
newBank: legacy.BankSpecified ? legacy.Bank : SampleBank | ||
); | ||
} | ||
|
||
return baseInfo; | ||
return base.ApplyTo(hitSampleInfo); | ||
} | ||
|
||
public override bool IsRedundant(ControlPoint? existing) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the volume is 100 by default you also have to make sure that the legacy decoder properly overrides hit object volumes with volume from the control points. If the volume is 100 then it will not take the volume from the control points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the code for this? Sounds flaky as hell, and untested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay turns out my concerns were misplaced. The
ConvertHitObjectParser
which creates theLegacyHitSampleInfo
always assigns the volume of theSampleBankInfo
whose volume is still 0 by default, so it works out just fine. For a moment I thought there were occurances ofLegacyHitSampleInfo
being created while decoding without a volume argument given.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And even if that wasn't the case,
ConvertHitObjectParser
only constructsLegacyHitSampleInfo
s, andLegacyHitSampleInfo
ctor still has 0 volume as a default, which makes the change of the default value in the base class largely irrelevant.