From 5e74f910c66ca426400aa13f303abae0f944e897 Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Fri, 29 May 2020 23:30:53 +0200 Subject: [PATCH 1/5] Correct a bug with sfz v1 velcurve The velcurve logic now uses the same as sfz v2 curves --- src/sfizz/Curve.cpp | 29 ++++++++++++++++++++++++++ src/sfizz/Curve.h | 11 ++++++++++ src/sfizz/Region.cpp | 13 +++--------- src/sfizz/Region.h | 4 +++- src/sfizz/Synth.cpp | 24 +++------------------ tests/RegionT.cpp | 8 +++++-- tests/SynthT.cpp | 28 +++++++++++++++++++++++++ tests/TestFiles/velocity_endpoints.sfz | 2 ++ 8 files changed, 85 insertions(+), 34 deletions(-) create mode 100644 tests/TestFiles/velocity_endpoints.sfz diff --git a/src/sfizz/Curve.cpp b/src/sfizz/Curve.cpp index f349efad..eaa22cdf 100644 --- a/src/sfizz/Curve.cpp +++ b/src/sfizz/Curve.cpp @@ -57,6 +57,35 @@ Curve Curve::buildCurveFromHeader( return curve; } +Curve Curve::buildFromVelcurvePoints( + const std::vector>& points, + Interpolator itp, bool invert) +{ + Curve curve; + bool fillStatus[NumValues] = {}; + const Range fullRange { -HUGE_VALF, +HUGE_VALF }; + + auto setPoint = [&curve, &fillStatus](int i, float x) { + curve._points[i] = x; + fillStatus[i] = true; + }; + + if (invert) { + setPoint(0, 1.0); + setPoint(NumValues - 1, 0.0); + } else { + setPoint(0, 0.0); + setPoint(NumValues - 1, 1.0); + } + + for (const auto& point: points) { + setPoint(point.first, point.second); + } + + curve.fill(itp, fillStatus); + return curve; +} + Curve Curve::buildPredefinedCurve(int index) { Curve curve; diff --git a/src/sfizz/Curve.h b/src/sfizz/Curve.h index 829a2d8b..d75e1a67 100644 --- a/src/sfizz/Curve.h +++ b/src/sfizz/Curve.h @@ -67,6 +67,17 @@ public: absl::Span members, Interpolator itp = Interpolator::Linear, bool limit = false); + /** + * @brief Build a curve based on sfz v1 "amp_velcurve_&" points + * + * @param points the points vector + * @param itp kind of interpolator to fill between values + * @param invert whether to invert the curve + */ + static Curve buildFromVelcurvePoints( + const std::vector>& points, + Interpolator itp = Interpolator::Linear, bool invert = false); + /** * @brief Number of predefined curves */ diff --git a/src/sfizz/Region.cpp b/src/sfizz/Region.cpp index afd438bd..63fc5676 100644 --- a/src/sfizz/Region.cpp +++ b/src/sfizz/Region.cpp @@ -388,7 +388,7 @@ bool sfz::Region::parseOpcode(const Opcode& rawOpcode) return false; if (value) - velocityPoints.emplace_back(normalizeVelocity(opcode.parameters.back()), *value); + velocityPoints.emplace_back(opcode.parameters.back(), *value); } break; case hash("xfin_lokey"): @@ -1130,15 +1130,8 @@ float sfz::Region::velocityCurve(float velocity) const noexcept ASSERT(velocity >= 0.0f && velocity <= 1.0f); float gain { 1.0f }; - if (velocityPoints.size() > 0) { // Custom velocity curve - auto after = absl::c_find_if(velocityPoints, [velocity](const std::pair& val) { return val.first >= velocity; }); - auto before = after == velocityPoints.begin() ? velocityPoints.begin() : after - 1; - // Linear interpolation - float relativePositionInSegment { - (velocity - before->first) / (after->first - before->first) - }; - float segmentEndpoints { after->second - before->second }; - gain *= relativePositionInSegment * segmentEndpoints; + if (velCurve) { // Custom velocity curve + return velCurve->evalNormalized(velocity); } else { // Standard velocity curve // FIXME: Maybe there's a prettier way to check the boundaries? const float gaindB = [&]() { diff --git a/src/sfizz/Region.h b/src/sfizz/Region.h index b532f712..a3e8a096 100644 --- a/src/sfizz/Region.h +++ b/src/sfizz/Region.h @@ -6,6 +6,7 @@ #pragma once #include "CCMap.h" +#include "Curve.h" #include "LeakDetector.h" #include "Defaults.h" #include "EGDescription.h" @@ -304,7 +305,8 @@ struct Region { uint8_t ampKeycenter { Default::ampKeycenter }; // amp_keycenter float ampKeytrack { Default::ampKeytrack }; // amp_keytrack float ampVeltrack { Default::ampVeltrack }; // amp_keytrack - std::vector> velocityPoints; // amp_velcurve_N + std::vector> velocityPoints; // amp_velcurve_N + absl::optional velCurve {}; float ampRandom { Default::ampRandom }; // amp_random Range crossfadeKeyInRange { Default::crossfadeKeyInRange }; Range crossfadeKeyOutRange { Default::crossfadeKeyOutRange }; diff --git a/src/sfizz/Synth.cpp b/src/sfizz/Synth.cpp index 41f72086..4fd0743c 100644 --- a/src/sfizz/Synth.cpp +++ b/src/sfizz/Synth.cpp @@ -315,26 +315,6 @@ void sfz::Synth::handleEffectOpcodes(const std::vector& rawMembers) bus.addEffect(std::move(fx)); } -void addEndpointsToVelocityCurve(sfz::Region& region) -{ - if (region.velocityPoints.size() > 0) { - const auto velocityStart = sfz::Default::velocityRange.getStart(); - const auto velocityEnd = sfz::Default::velocityRange.getEnd(); - absl::c_sort(region.velocityPoints, [](const std::pair& lhs, const std::pair& rhs) { return lhs.first < rhs.first; }); - if (region.ampVeltrack > 0) { - if (region.velocityPoints.front().first != velocityStart) - region.velocityPoints.insert(region.velocityPoints.begin(), std::make_pair(velocityStart, velocityStart)); - if (region.velocityPoints.back().first != velocityEnd) - region.velocityPoints.push_back(std::make_pair(velocityEnd, velocityEnd)); - } else { - if (region.velocityPoints.front().first != velocityEnd) - region.velocityPoints.insert(region.velocityPoints.begin(), std::make_pair(velocityEnd, velocityStart)); - if (region.velocityPoints.back().first != velocityStart) - region.velocityPoints.push_back(std::make_pair(velocityStart, velocityEnd)); - } - } -} - bool sfz::Synth::loadSfzFile(const fs::path& file) { clear(); @@ -484,7 +464,9 @@ void sfz::Synth::finalizeSfzLoad() } } - addEndpointsToVelocityCurve(*region); + if (!region->velocityPoints.empty()) + region->velCurve = Curve::buildFromVelcurvePoints( + region->velocityPoints, Curve::Interpolator::Linear, region->ampVeltrack < 0.0f); region->registerPitchWheel(0); region->registerAftertouch(0); region->registerTempo(2.0f); diff --git a/tests/RegionT.cpp b/tests/RegionT.cpp index 4884274b..c18638cd 100644 --- a/tests/RegionT.cpp +++ b/tests/RegionT.cpp @@ -694,9 +694,13 @@ TEST_CASE("[Region] Parsing opcodes") SECTION("amp_velcurve") { region.parseOpcode({ "amp_velcurve_6", "0.4" }); - REQUIRE(region.velocityPoints.back() == std::make_pair(6_norm, 0.4f)); + REQUIRE(region.velocityPoints.back() == std::pair(6, 0.4f)); region.parseOpcode({ "amp_velcurve_127", "-1.0" }); - REQUIRE(region.velocityPoints.back() == std::make_pair(127_norm, 0.0f)); + REQUIRE(region.velocityPoints.back() == std::pair(127, 0.0f)); + region.parseOpcode({ "amp_velcurve_008", "0.3" }); + REQUIRE(region.velocityPoints.back() == std::pair(8, 0.3f)); + region.parseOpcode({ "amp_velcurve_064", "0.9" }); + REQUIRE(region.velocityPoints.back() == std::pair(64, 0.9f)); } SECTION("xfin_lokey, xfin_hikey") diff --git a/tests/SynthT.cpp b/tests/SynthT.cpp index e89ef65e..e59df54f 100644 --- a/tests/SynthT.cpp +++ b/tests/SynthT.cpp @@ -434,3 +434,31 @@ TEST_CASE("[Synth] Basic curves") // Default linear REQUIRE( curves.getCurve(16).evalCC7(63) == Approx(63_norm) ); } + +TEST_CASE("[Synth] Velocity points") +{ + sfz::Synth synth; + synth.loadSfzFile(fs::current_path() / "tests/TestFiles/velocity_endpoints.sfz"); + REQUIRE( !synth.getRegionView(0)->velocityPoints.empty()); + REQUIRE( synth.getRegionView(0)->velocityPoints[0].first == 64 ); + REQUIRE( synth.getRegionView(0)->velocityPoints[0].second == 1.0_a ); + REQUIRE( !synth.getRegionView(1)->velocityPoints.empty()); + REQUIRE( synth.getRegionView(1)->velocityPoints[0].first == 64 ); + REQUIRE( synth.getRegionView(1)->velocityPoints[0].second == 1.0_a ); +} + +TEST_CASE("[Synth] velcurve") +{ + sfz::Synth synth; + synth.loadSfzFile(fs::current_path() / "tests/TestFiles/velocity_endpoints.sfz"); + REQUIRE( synth.getRegionView(0)->velocityCurve(0_norm) == 0.0_a ); + REQUIRE( synth.getRegionView(0)->velocityCurve(32_norm) == Approx(0.5f).margin(1e-2) ); + REQUIRE( synth.getRegionView(0)->velocityCurve(64_norm) == 1.0_a ); + REQUIRE( synth.getRegionView(0)->velocityCurve(96_norm) == 1.0_a ); + REQUIRE( synth.getRegionView(0)->velocityCurve(127_norm) == 1.0_a ); + REQUIRE( synth.getRegionView(1)->velocityCurve(0_norm) == 1.0_a ); + REQUIRE( synth.getRegionView(1)->velocityCurve(32_norm) == 1.0_a ); + REQUIRE( synth.getRegionView(1)->velocityCurve(64_norm) == 1.0_a ); + REQUIRE( synth.getRegionView(1)->velocityCurve(96_norm) == Approx(0.5f).margin(1e-2) ); + REQUIRE( synth.getRegionView(1)->velocityCurve(127_norm) == 0.0_a ); +} diff --git a/tests/TestFiles/velocity_endpoints.sfz b/tests/TestFiles/velocity_endpoints.sfz new file mode 100644 index 00000000..dd3808d7 --- /dev/null +++ b/tests/TestFiles/velocity_endpoints.sfz @@ -0,0 +1,2 @@ + amp_velcurve_064=1 sample=*sine + amp_velcurve_064=1 amp_veltrack=-100 sample=*sine From 0898030e4c8f54914454279edf9a0ab9f9491fc8 Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Sat, 30 May 2020 00:15:53 +0200 Subject: [PATCH 2/5] Clamp at the first non-number character in readOpcode --- src/sfizz/Opcode.h | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/sfizz/Opcode.h b/src/sfizz/Opcode.h index 04cf67ac..52f67af7 100644 --- a/src/sfizz/Opcode.h +++ b/src/sfizz/Opcode.h @@ -115,20 +115,23 @@ private: template ::value, int> = 0> inline absl::optional readOpcode(absl::string_view value, const Range& validRange) { - int64_t returnedValue; - if (!absl::SimpleAtoi(value, &returnedValue)) { - float floatValue; - if (!absl::SimpleAtof(value, &floatValue)) - return {}; - returnedValue = static_cast(floatValue); - } + const auto numberEnd = value.find_first_not_of("-1234567890."); + value = value.substr(0, numberEnd); - if (returnedValue > std::numeric_limits::max()) - returnedValue = std::numeric_limits::max(); - if (returnedValue < std::numeric_limits::min()) - returnedValue = std::numeric_limits::min(); + int64_t returnedValue; + if (!absl::SimpleAtoi(value, &returnedValue)) { + float floatValue; + if (!absl::SimpleAtof(value, &floatValue)) + return {}; + returnedValue = static_cast(floatValue); + } - return validRange.clamp(static_cast(returnedValue)); + if (returnedValue > std::numeric_limits::max()) + returnedValue = std::numeric_limits::max(); + if (returnedValue < std::numeric_limits::min()) + returnedValue = std::numeric_limits::min(); + + return validRange.clamp(static_cast(returnedValue)); } /** @@ -144,8 +147,11 @@ inline absl::optional readOpcode(absl::string_view value, const Range template ::value, int> = 0> inline absl::optional readOpcode(absl::string_view value, const Range& validRange) { + const auto numberEnd = value.find_first_not_of("-1234567890."); + value = value.substr(0, numberEnd); + float returnedValue; - if (!absl::SimpleAtof(value, &returnedValue)) + if (!absl::SimpleAtof(value.substr(0, numberEnd), &returnedValue)) return absl::nullopt; return validRange.clamp(returnedValue); From eff5bd03624c23a942e4ba2637602013bc620416 Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Sat, 30 May 2020 09:54:02 +0200 Subject: [PATCH 3/5] Added tests and silence a warning --- src/sfizz/Opcode.h | 4 ++-- tests/OpcodeT.cpp | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/sfizz/Opcode.h b/src/sfizz/Opcode.h index 52f67af7..65436b2c 100644 --- a/src/sfizz/Opcode.h +++ b/src/sfizz/Opcode.h @@ -122,7 +122,7 @@ inline absl::optional readOpcode(absl::string_view value, const Range if (!absl::SimpleAtoi(value, &returnedValue)) { float floatValue; if (!absl::SimpleAtof(value, &floatValue)) - return {}; + return absl::nullopt; returnedValue = static_cast(floatValue); } @@ -151,7 +151,7 @@ inline absl::optional readOpcode(absl::string_view value, const Range value = value.substr(0, numberEnd); float returnedValue; - if (!absl::SimpleAtof(value.substr(0, numberEnd), &returnedValue)) + if (!absl::SimpleAtof(value, &returnedValue)) return absl::nullopt; return validRange.clamp(returnedValue); diff --git a/tests/OpcodeT.cpp b/tests/OpcodeT.cpp index 78ce2d87..53581dd6 100644 --- a/tests/OpcodeT.cpp +++ b/tests/OpcodeT.cpp @@ -246,3 +246,20 @@ TEST_CASE("[Opcode] Normalization") REQUIRE(sfz::Opcode("SaMpLe", "").cleanUp(sfz::kOpcodeScopeRegion).opcode == "sample"); } + +TEST_CASE("[Opcode] readOpcode") +{ + REQUIRE( sfz::readOpcode("16", sfz::Range(0, 100)).value() == 16 ); + REQUIRE( sfz::readOpcode("110", sfz::Range(0, 100)).value() == 100 ); + REQUIRE( sfz::readOpcode("-1", sfz::Range(0, 100)).value() == 0 ); + REQUIRE( sfz::readOpcode("12.5", sfz::Range(-100, 100)).value() == 12 ); + REQUIRE( sfz::readOpcode("-40", sfz::Range(-100, 100)).value() == -40 ); + REQUIRE( sfz::readOpcode("-140", sfz::Range(-100, 100)).value() == -100 ); + REQUIRE( sfz::readOpcode("12.5", sfz::Range(0.0f, 100.0f)).value() == 12.5_a ); + REQUIRE( sfz::readOpcode("-22.5", sfz::Range(-20.0f, 100.0f)).value() == -20.0_a ); + REQUIRE( sfz::readOpcode("150.5", sfz::Range(-20.0f, 100.0f)).value() == 100.0_a ); + REQUIRE( sfz::readOpcode("50.25garbage", sfz::Range(-20.0f, 100.0f)).value() == 50.25_a ); + REQUIRE( sfz::readOpcode("50.25garbage", sfz::Range(-20, 100)).value() == 50 ); + REQUIRE( !sfz::readOpcode("garbage50.25", sfz::Range(-20, 100)) ); + REQUIRE( !sfz::readOpcode("garbage", sfz::Range(-20, 100)) ); +} From 7ccdaa02c2fe6dbd2f9ea987c5a28211ddf7bf0c Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Sat, 30 May 2020 15:09:27 +0200 Subject: [PATCH 4/5] Change parameter to span --- src/sfizz/Curve.cpp | 2 +- src/sfizz/Curve.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sfizz/Curve.cpp b/src/sfizz/Curve.cpp index eaa22cdf..2e53bc4c 100644 --- a/src/sfizz/Curve.cpp +++ b/src/sfizz/Curve.cpp @@ -58,7 +58,7 @@ Curve Curve::buildCurveFromHeader( } Curve Curve::buildFromVelcurvePoints( - const std::vector>& points, + absl::Span> points, Interpolator itp, bool invert) { Curve curve; diff --git a/src/sfizz/Curve.h b/src/sfizz/Curve.h index d75e1a67..3cf4b2cb 100644 --- a/src/sfizz/Curve.h +++ b/src/sfizz/Curve.h @@ -75,7 +75,7 @@ public: * @param invert whether to invert the curve */ static Curve buildFromVelcurvePoints( - const std::vector>& points, + absl::Span> points, Interpolator itp = Interpolator::Linear, bool invert = false); /** From 952ce455a62d7bf267f6c5c6721f337d6cadf4dc Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Sat, 30 May 2020 15:18:09 +0200 Subject: [PATCH 5/5] Reactivated the test --- tests/FilesT.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/FilesT.cpp b/tests/FilesT.cpp index 5831210b..b64264db 100644 --- a/tests/FilesT.cpp +++ b/tests/FilesT.cpp @@ -346,14 +346,12 @@ TEST_CASE("[Files] wrong (overlapping) replacement for defines") REQUIRE( synth.getNumRegions() == 3 ); -#if 0 // Note: test checked to be wrong under Sforzando 1.961 // It is the shorter matching $-variable which matches among both. // The rest of the variable name creates some trailing junk text - // which Sforzando accepts without warning. (eg. `key=52Edge`) - REQUIRE( synth.getRegionView(0)->keyRange.getStart() == 52 ); - REQUIRE( synth.getRegionView(0)->keyRange.getEnd() == 52 ); -#endif + // which Sforzando accepts without warning. (eg. `key=57Edge`) + REQUIRE( synth.getRegionView(0)->keyRange.getStart() == 57 ); + REQUIRE( synth.getRegionView(0)->keyRange.getEnd() == 57 ); REQUIRE( synth.getRegionView(1)->keyRange.getStart() == 57 ); REQUIRE( synth.getRegionView(1)->keyRange.getEnd() == 57 );