DLS: fix the pitch scale conversions which saturated#217
Merged
Conversation
DlsDetector.normalizeEG2ToPitch clamped the raw 32-bit connection value to +-1200 and divided by 1200, but that value is a relative pitch stored as a fixed point number with 65536 representing one cent. Every non-zero value therefore saturated to full scale. * Pitch envelope depth: divided by 65536 first and normalized over the same range as every other format, so a one octave envelope is 1200 cent and not ten octaves. Measured on the Roland GS set that ships with macOS (gs_instruments.dls): 522 zones that were all written as +-12000 cent are now spread across their real -1200..1200 cent values. * Pitch tuning: converted to semi-tones (65536 per cent, 100 cent per semi-tone) instead of the saturated value. * Modulation envelope sustain: routed through the sustain level conversion like the first envelope instead of the pitch conversion. This branch is currently unreached because the envelope is only ever created for the first EG, but it removes the last use of the helper. The now unused normalizeEG2ToPitch is removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DlsDetector.normalizeEG2ToPitchclamps the raw 32-bit connection value to ±1200 and divides by 1200 — but that value is a relative pitch stored as a fixed point number with 65536 representing one cent, the same convention the file's ownabsoluteTimeToSecondsalready divides out. So every non-zero value saturates to full scale.The helper was used for three different quantities, each with a different correct unit, so all three were wrong:
MAX_ENVELOPE_DEPTHlike every other format, so a one-octave envelope is 1200 cent rather than ten octaves.The now-unused
normalizeEG2ToPitchis deleted. One file,DlsDetector.java.Validation
Measured against the Roland GS set that ships with macOS (
gs_instruments.dls), converted DLS → SFZ before and after:pitcheg_depth=±12000(the saturated maximum, ten octaves).1200,1000,200,-95,-63,-29, ... cent — matching the raw connection values decoded straight from the file.pitcheg_depth; loops, key ranges, amplitude envelopes and everything else are byte-identical, so there is no collateral change.The tuning and sustain paths are not exercised by this particular set (it has no region-level pitch tuning connection, and the sustain branch is unreached as noted), so their fix is by unit analysis rather than a visible diff here.
Note
Independent of #216 (the pitch-LFO PR): different concern, and #216 touches
DlsDetectoronly by adding a separate block, so the two can merge in either order.