Adds the digital drop pedal feature to exist within RSMods natively - #233
Adds the digital drop pedal feature to exist within RSMods natively#233Cheesewizard wants to merge 10 commits into
Conversation
a54c816 to
ad563c6
Compare
Cheesewizard
left a comment
There was a problem hiding this comment.
Adds a digital drop pedal feature for rocksmith 2014
|
Thank you! We'll take a look ASAP. |
There was a problem hiding this comment.
I'd like you to take care of a few smaller things to ensure this altogether works nicely.
Two sidenotes:
- as it stands, this mod is incompatible with the MIDI auto tuning mod and there is no guardrails to prevent an user from enabling both. It's unlikely to cause any problems, and proper fix for this would require changes to the whole infrastructure, so for now there's no need to change anything.
- this whole approach may have been a tad bit overkill, since the audio output is the important part, while this modified the input audio (i.e. requiring more steps)... nonentheless, it doesn't really matter in the end if it works.
| IInputProcessor* processor = activeProcessor.load(std::memory_order_relaxed); | ||
| if (!processor) return; | ||
|
|
||
| if (activeSampleType != ASIOSTInt32LSB) return; |
There was a problem hiding this comment.
Many interfaces output ASIOSTFloat32LSB or ASIOSTInt24LSB or ASIOSTInt16LSB.
Implementation accepts only ASIOSTInt32LSB, while documentation implies general ASIO-interface support. Add common Float32/Int24 conversions or document hard requirement.
There was a problem hiding this comment.
This is a great point. I've only got 1 audio interface available to test with, but will look at extending this.
There was a problem hiding this comment.
Added Float32, Int24 and Int16 support alongside Int32 (Untested, should be correct though). Each buffer is converted to float for processing and back to the driver's native format, with clamping on the way out.
| // Semitones the guitar has to move, in cents. Written by the game loop and read | ||
| // by SetParam on other threads: a torn read of a float is impossible on x86, and | ||
| // a briefly stale value is harmless. | ||
| volatile float targetCents = 0.0f; | ||
|
|
||
| // From [Drop Pedal] in RSMods.ini, read once at startup. Values follow the ini's | ||
| // lowercase convention (on / off / automatic). | ||
| bool isConfiguredEnabled = false; | ||
| std::string engineSetting = "automatic"; | ||
|
|
||
| // Enabled state lives in the session rather than in the settings map, because | ||
| // the settings reload during boot and would wipe it. Starts on, so a session | ||
| // never silently begins with the pedal off; the toggle key still turns it off | ||
| // for the session. Read by SetParam on other threads. | ||
| volatile bool isEnabledSession = true; |
There was a problem hiding this comment.
Not critical, but hotkey thread writes targetCents/isEnabledSession while Wwise and render threads read them. volatile provides no thread synchronization. Since the rest of code uses (what appears to be) proper synchronization, use atomics or publish one immutable state snapshot.
There was a problem hiding this comment.
Replaced the volatile state with std::atomic throughout DropPedalState.
| // Virtual member functions are __thiscall on x86, which passes this in ECX. | ||
| // __fastcall matches that once the unused EDX argument is declared explicitly. | ||
| typedef AKRESULT(__fastcall* tSetParamRaw)(void* self, void* unused, AkUInt32 paramId, const void* value, AkUInt32 size); | ||
| typedef void(__fastcall* tTermRaw)(void* self, void* unused, void* allocator); |
There was a problem hiding this comment.
If you take a look at the WWise docs, IAkPluginParam::Term actually returns AKRESULT, but tTermRaw and SpyTerm return void. Caller receives undefined EAX. Use AKRESULT and return originalTerm(...).
There was a problem hiding this comment.
Fixed. tTermRaw and SpyTerm now return AKRESULT and forward the original's result.
| cmp dword ptr [EBP + 0x8], -1200 // [EBP + 0x8] = CentOffset. If CentOffset == -1200, we need to use the true tuning or bass can break on some songs. | ||
| jne trueTuningForceA440 // If this song does not have a CentOffset of -1200, then continue with forcing our reference. | ||
|
|
||
| jmp Offsets::runtimeVersionStructValue | ||
| jmp forceTrueTuningAddress | ||
|
|
||
| trueTuningForceA440: | ||
| pop EAX // Restore EAX from the stack | ||
| fld ForcedTrueTuning // Set ST(0) to 440 | A440. This tells note detection that we want to use A440 as our true tuning. | ||
|
|
||
| pushad | ||
|
|
||
| lea ecx, Offsets::ptr_disableTrueTuning_jmpBck | ||
| call VersioningStruct<uintptr_t>::GetValue | ||
| mov Offsets::runtimeVersionStructValue, eax | ||
|
|
||
| popad | ||
|
|
||
| jmp Offsets::runtimeVersionStructValue | ||
| fld ForcedTrueTuning // Set ST(0) to our reference. This tells note detection which tuning we want it to expect. | ||
| jmp jumpBackAddress | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Disable true tuning by telling note detection that it should use A440 as the base-point. | ||
| /// Disable true tuning by telling note detection that it should use our reference as the base-point. | ||
| /// </summary> | ||
| void TrueTuning::DisableTrueTuning() | ||
| { | ||
| forceTrueTuningAddress = Offsets::ptr_disableTrueTuning_forceTT.GetValue(); | ||
| jumpBackAddress = Offsets::ptr_disableTrueTuning_jmpBck.GetValue(); | ||
|
|
||
| if (forceTrueTuningAddress == 0 || jumpBackAddress == 0) | ||
| { | ||
| LOG_ERROR("True tuning not hooked because this Rocksmith version has no branch targets: forceTT 0x" | ||
| << std::hex << forceTrueTuningAddress << ", jmpBck 0x" << jumpBackAddress << std::dec << std::endl); | ||
| return; | ||
| } | ||
|
|
||
| MemUtil::PatchAdr(Offsets::ptr_disableTrueTuningGate, "\xEB", 1); // Force a jump into our code, JMP. | ||
| MemUtil::PlaceHook(Offsets::ptr_disableTrueTuning, disableTrueTuning, 6); |
There was a problem hiding this comment.
LP target 0x004DD93F lies inside NoteDetection @ 0x004DD7F0. [EBP+8] is sample count; skipped instructions perform unsigned-integer conversion for RMS math, not cents/tuning. Enabling drop pedal corrupts onset calculation. Remove the hook if necessary and find the actual tuning-reference producer before replacement.
Ensure this doesn't break songs like Voodo Child or Don't Look Back in Anger, which don't use A440 reference pitch.
There was a problem hiding this comment.
Removed the old hook and its offsets. Following your pointer, I found the actual reference producer: the cents-to-Hz builder at 0x4DCCB0 (Sept 2022 exe). It computes 440 * 2^(cents/1200) and stamps the result at [detection + 0x135C], which is also where the ptr_trueTuning pointer chain lands. It is now detoured with the cent offset adjusted on the way in, so the game computes the shifted reference through its own math. Non-A440 arrangements and the -1200 emulated bass case compose naturally, and the pre-song tuner's load-time snapshot picks up the shift. Verified in the tuner and in-song, including Voodoo Child. The LP address is unresolved: the old mask match at 0x004DD93F was the RMS site you identified, not this function. The byte pattern for the real builder is in the Offsets.cpp comment if you can derive the LP address from that binary. Without it, LP falls back to the live writes, so in-song detection stays correct but the tuner does not follow the shift.
Regarding the MIDI auto tuning mod. I don't have the hardware to test this. I could flag a console warning if it detects both enabled at the same time? Its not something I use personally. For your final point. I initially started with non asio users which in order to achieve the pitch down uses the multipitch effect loaded in the chain. The limitation been the loaded tone needs this pedal. Asio is 100% a better implementation, though it adds a requirement on a 3rd party mod. So I made it optional by supporting both methods. I then realized like a real drop cable in order to have low latency i need to change the pitch of the signal before it goes into the game. Changing the pitch of the final output audio introduced latency in my experimentation. This is the issue I'm tackling with speaker mode which is not released yet (tune the game audio to a target pitch rather than the guitar) Thanks for taking the time to peer review this, I will look at all the points you have raised. |
| ptr_disableTrueTuning_jmpBck = { {0x004DCCF8, baseHandle + 0x00DD978 } }; // Code | Bytes 33 c0 after the mask below (roughly 0x37 bytes away) | ||
| ptr_disableTrueTuning_forceTT = { {0x004DCCC1, baseHandle + 0x00DD941 } }; // Code | 83 7d 08 00 53 57 74 ? db 45 08 (db is the byte we want) | ||
| ptr_disableTrueTuningGate = { {0x004DCCBF, baseHandle + 0x00DD93F } }; // Code | 83 7d 08 00 53 57 74 ? db 45 08 (74 is the byte we want) | ||
| func_tuningReferenceBuilder = { {0x004DCCB0, 0x0} }; // Code | 55 8b ec 83 e4 f8 83 ec 10 83 7d 08 00 53 57 74 ? db 45 08 (we want the start of the function). Converts an arrangement cent offset to the note-detection reference, 440 * 2^(cents / 1200), and stamps it at [detection + 0x135C]. LP address is unresolved: the same mask on LP matched NoteDetection RMS code at 0x004DD93F, which is the wrong function. |
There was a problem hiding this comment.
Added, thank you. With the L&P builder address in place the pre-song tuner follows the shift on both supported versions, and I've removed the L&P caveats from the docs.
Lovrom8
left a comment
There was a problem hiding this comment.
It's all coming together nicely. A few more things to ensure it's altogether ready...
There was a problem hiding this comment.
The applied replacement sounds correct, but please do not completely drop the old TrueTuning mod. What I'd meant was that you undo the proposed changes in the mod. I didn’t express myself correctly in the original comment, sorry.
There was a problem hiding this comment.
Restored file from latest upstream
| const int targetSemitones = shouldTransposeDetection | ||
| ? DropPedalState::GetTargetSemitones() | ||
| : 0; | ||
| const float targetTrueTuning = authoredTrueTuning |
There was a problem hiding this comment.
Edge case: In Cable mode ApplyTrueTuningLocked patches the shared true-tuning float to a shifted value, it’s the same address SongTuning::GetTrueTuning() reads back live. That's intended for note detection, but GetTrueTuning() is also the A220 song-identity signal for the bass/ER fixes (== 220/<= 260) and the MIDI auto-tuner.
So a Cable octave drop on a bass-A220 song moves 220 → 440, those checks go false, and the octave correction silently drops out. That is difficult to find out with pedal-only testing.
Summarily - one float is carrying two meanings. Could we decouple them? E.g. have GetTrueTuning() return DropPedal's authored value (which it already tracks) whenever the pedal is holding a shifted reference, falling back to the live read otherwise.
There was a problem hiding this comment.
Decoupled as you suggested. SongTuning::GetTrueTuning() now asks the Drop Pedal for the authored value first (TryGetAuthoredTrueTuning), falling back to the live read when the pedal isn't holding a shifted reference. The authored value comes from the captured baseline when one exists, or is reconstructed from the cents the builder hook was given, so a Cable octave drop on a bass-A220 song still reads 220 and the octave correction, ≤260 checks, and MIDI auto-tuner are unaffected.
| if (ring.empty()) return; | ||
|
|
||
| const float pitchRatio = ratio.load(std::memory_order_relaxed); | ||
| const double drift = 1.0 - (double)pitchRatio; |
There was a problem hiding this comment.
When the pedal's off or target is 0, ratio is exactly 1.0, so drift = 0 and no splice fires. In the same time, Process() still runs DetectPeriod() every 256 samples and pipes every sample through ReadTap(130). Net effect with ASIO on and nothing to shift: a ~130-sample delay plus the full detector cost, for no pitch change.
One solution would be to early-out at the top of Process() when pitchRatio == 1.0f, skip detection and splicing, and leave samples untouched?
There was a problem hiding this comment.
Good catch.
Done. Process() early-outs at a 1.0 ratio: samples pass through untouched, with no detection or splicing cost, and GetLatencyFrames() reports 0. The ring and decimation buffers keep filling during bypass so re-engagement doesn't start from a cold detector.
| void* paramObject = deliveredParamObjects[i]; | ||
| if (paramObject == nullptr || MemUtil::IsBadReadPtr(paramObject)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| uintptr_t* vtable = *(uintptr_t**)paramObject; | ||
| if (MemUtil::IsBadReadPtr(vtable)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| const float cents = deliveredAuthoredCents[i] + shiftCents; | ||
|
|
||
| originalSetParam(paramObject, nullptr, PITCH_PARAM_ID, ¢s, sizeof(float)); |
There was a problem hiding this comment.
Lower priority, since this can crash, though it’s hard to trigger - needs a hotkey press to coincide with a tone teardown, but it might show up as an unexplained crash on song-change/exit.
PushPitchToLiveShiftersWithShift runs on the hotkey/game thread but calls originalSetParam directly on deliveredParamObjects[i], while SpyTerm frees those objects on Wwise's audio thread, due to the hook setup.
The guard at DropPedalHooks.cpp:352 is a check-then-use race: the object can be Term'd between the pointer load and the originalSetParam, and IsBadReadPtr passes on freed-but-still-mapped memory.
This looks feasible to fix via the RegisterGlobalCallback already wrapped in SoundEngine.cpp:939. Its callback fires once per render frame on Wwise's audio thread, so I propose:
- Hotkey thread only publishes intent (atomic pending-shift + dirty flag).
- The global callback reads that and runs the existing push loop there, so an object can't be freed mid-push, and the cross-thread dereference goes away.
There was a problem hiding this comment.
Implemented as you proposed. The hotkey thread now only publishes intent (atomic pending shift + flag), and a global callback consumes it and runs the push loop on Wwise's render thread, so Term can't free an object mid-push and the IsBadReadPtr checks are gone. The callback is registered when the MultiPitch plug-in registers successfully, and the final callback (in_bLastCall) drops any pending work so teardown never touches a param object.
| if (active) | ||
| { | ||
| // Remove the Cable shift before ASIO takes ownership, so the two engines | ||
| // cannot apply the same target at once during automatic promotion. | ||
| PushPitchToLiveShiftersWithShift(0.0f); | ||
| inputShifterActive.store(true, std::memory_order_relaxed); | ||
| ApplyCapturedTrueTuning(); |
There was a problem hiding this comment.
The promotion path resets the live shifters to authored pitch while inputShifterActive still reads Cable, so a concurrent SpySetParam re-applies the shift onto the just-reset shifter, and ASIO retunes on top, so both engines shift. Reordering the two lines just moves the gap to the other side.
"Cable stops applying" and "ASIO is processing" have to happen at different times, which one bool can't express.
Could we replace the bool with an atomic enum like? Something that would contain a 3 states - Cable/Transitioning/Asio?
- Gates treat "Cable owns" as load(acquire) == Cable, so
Transitioningalready suppresses the shift. - Ordered promotion: store Transitioning (release) → restore authored + UpdateReferenceCentsAdjustment() → store Asio (release) last.
The demotion branch is already double-apply-safe, so it can stay as-is.
There was a problem hiding this comment.
Implemented as proposed: an atomic PitchOwner with the gates treating "Cable owns" as an acquire load == Cable, so Transitioning already suppresses the shift. Promotion stores Transitioning, the audio-thread callback restores every live shifter's authored pitch, and Asio is published last — only if every restore succeeded. On failure the main loop disables ASIO processing and Cable retains ownership. One extra hardening: during Transitioning the callback forces the restore shift, so a hotkey push racing the promotion can't overwrite it in the pending slot. The demotion branch stays as it was.
| isPitchPushPending = true; | ||
| lastTargetChangeTime = GetTickCount64(); |
There was a problem hiding this comment.
isPitchPushPending and lastTargetChangeTime are written on the hotkey thread and read on the main thread without synchronization. On 32-bit x86, reading/writing a 64-bit timestamp non-atomically can tear.
Could we consolidate these into a single std::atomic<ULONGLONG> pushDeadlineTick{ 0 }?
- Hotkey thread:
pushDeadlineTick.store(GetTickCount64() + DELAY, std::memory_order_release); - Main thread: Load with
acquireand reset withcompare_exchange_strong(expected, 0)before pushing pitch.
There was a problem hiding this comment.
Consolidated exactly as suggested: a single std::atomic pushDeadlineTick, release store on the hotkey thread, acquire load plus compare_exchange_strong consume on the main thread.
| IInputProcessor* processor = activeProcessor.load(std::memory_order_relaxed); | ||
| if (!processor) return; | ||
|
|
||
| if (activeBufferFrames <= 0 || activeBufferFrames > MAX_BUFFER_FRAMES) return; |
There was a problem hiding this comment.
If the ASIO driver negotiates a buffer size > 4096, ProcessInputBuffer() returns early on line 328, but Hook_CreateBuffers() still publishes valid format details. This causes automatic mode to report ASIO as ready even though no pitch shifting happens.
I’d suggest we explicitly mark format.sampleFormat as SampleFormat::Unsupported if bufferSize > MAX_BUFFER_FRAMES inside Hook_CreateBuffers(). RSMods caps at 4096 for the INI settings anyway.
| } | ||
| } | ||
|
|
||
| float ClampSample(float value) |
There was a problem hiding this comment.
Done. ClampSample now uses std::clamp, keeping the std::isfinite guard in front since std::clamp on NaN is undefined.
| return value; | ||
| } | ||
|
|
||
| int16_t FloatToInt16(float value) |
There was a problem hiding this comment.
Really all these functions are just templates of a lerp with a type-cast at the end. I'd say use std::lerp but that's C++20 :( , so you might just be able to use a template instead of writing the same function 3 times.
There was a problem hiding this comment.
Consolidated into one FloatToSignedInteger<SampleType, BIT_DEPTH> template with the asymmetric signed endpoints explicit and static_asserts on the type/bit-depth combination.
| std::string ReadDriverNameFromRsAsioIni() | ||
| { | ||
| CSimpleIniA reader; | ||
| if (reader.LoadFile("RS_ASIO.ini") < 0) return {}; |
There was a problem hiding this comment.
I have an overall concern that you are reading this ini file multiple times in multiple places when you could just read it once, get all the values you need out of it, then never need to touch it again.
(I know we do not follow this elsewhere, but in my eyes its a bit different because in those cases its OUR ini that we are reading. In this case, it is NOT OUR ini that is being read.)
There was a problem hiding this comment.
Agreed, especially since it's RS_ASIO's file, not ours. It's now read exactly once at install. ReadRsAsioConfiguration() captures both input drivers/channels plus the output-driver fallback into one snapshot struct, and the file is never touched again.
| @@ -0,0 +1,632 @@ | |||
| #include "stdafx.h" | |||
There was a problem hiding this comment.
Overall I am a bit concerned about this entire scheme. In my mind, we should not care if the user is ASIO or WASAPI, (or even WDM or DirectXSink...).
I know its not in the spirit of this mod, but did you investigate if this asio piece could be done within RS_ASIO itself, as it deals with audio more than these mods do.
There was a problem hiding this comment.
I considered it. RS_ASIO has no public processing/extension API, so it would mean a fork or upstream change, a cross-DLL control channel back to RSMods for pedal state and hotkeys, and coordinated releases and it still wouldn't help non-ASIO users.
That's why there are two engines: the Cable engine works on the game side regardless of audio backend, and the ASIO engine shifts the physical input where RS_ASIO is present. The hook ordering (we process before RS_ASIO copies samples into the game) is documented in docs/asio-drop-pedal.md. If RS_ASIO ever grows an extension point, migrating the ASIO engine there is a natural follow-up.
| // [Asio.Input.0] channel, so a second player plays unshifted. | ||
| if (DropPedalState::IsConfiguredEnabled() && HasSecondInputConfigured()) | ||
| { | ||
| LOG_WARNING("RS_ASIO.ini configures a second input under [Asio.Input.1]. The drop " |
There was a problem hiding this comment.
Not a blocker, just curious if there is anything preventing you from setting up another buffer for input 1 and doing this so multiplayer could work
There was a problem hiding this comment.
There was nothing stopping me. It's implemented, for both engines. ASIO: each configured input gets its own persistent shifter, and ownership only transfers once every configured route is ready. Cable: each pitch-shifter effect is attributed to its owning player (docs/drop-pedal-multiplayer-design.md has the findings). Player 2 uses Ctrl + the Player 1 keys. Validated live with two inputs on songs with different arrangement tunings for example Creedence fortunate son
D standard lead
E standard for rhythm
E standard for bass
| /// </summary> | ||
| unsigned WINAPI DropPedalHotkeyThread() { | ||
| while (!GameState::GameClosing) { | ||
| ModManager::PollDropPedalHotkeys(); |
There was a problem hiding this comment.
Why are we spinning up a new thread here. You should be able to hook into our Keybindings::InitalizeCommands system and hit your hotkeys on the same frame the game gets them (we hook the game's WndProc, so those key pressed events go to us not the game).
You'd also get a somewhat snappier response I think. Not really noticeable since you're sleeping for 15ms but it would be event driven instead of polled.
There was a problem hiding this comment.
You're right, the thread is gone. The drop pedal keys are registered through Keybindings::InitializeCommands and dispatch from the WndProc hook, event-driven on release. ASIO retargets synchronously in the handler; the Cable path's debounced push is polled from the existing main loop.
While wiring this I noticed DispatchCommand never evaluates ModCommand.condition for any registered command, so e.g. ToggleLoft's "is the mod enabled" check is dead code, the drop pedal handlers guard themselves internally. I've put a fix on a separate branch (dev_keybind_condition_and_twortc_fixes) so it doesn't bloat this PR.
| { | ||
| if (!hasLoggedSongTuning) | ||
| { | ||
| const uintptr_t addrTuning = MemUtil::FindDMAAddy( |
There was a problem hiding this comment.
Nit: You could call SongTuning::GetCurrentTuning to handle this for you.
There was a problem hiding this comment.
That diagnostic has been removed entirely in the cleanup (its output was garbage in multiplayer anyway). For reference, the direct resolve existed because GetCurrentTuning returns an all-zero array both on a failed pointer resolve and for genuine E standard, and the log gate needed to distinguish them. If we ever want the shared helper for a case like this, a TryGetCurrentTuning returning success separately would do it.
|
Cable multiplayer attributes each tone's pitch shifter to its player through a mixer node ID that's stable across sessions but specific to the game version. Remastered Sept 2022 is resolved (Player 1 = 0x5e22c1ab, Player 2 = 0x5e22c1a8); Learn & Play Dec 2024 needs its two values before this can ship, and the engine is built to hand them over, same as the song-timer offsets you helped with before: Build this branch (debug, logging enabled) with EnableDropPedal = on and Engine = cable in RSMods.ini. Player 2's ID in multiplayer with a MultiPitch tone loaded on Player 2's side (keys 5–8 load their tone slots, and tones are per-profile, so Player 2's profile needs its own), a second distinct ID is logged, that's Player 2's. Add both to the versionedPlayerOnePipelineNodeId / versionedPlayerTwoPipelineNodeId initializers in DropPedalHooks.cpp (second slot = LPDecember2024, currently 0), then re-run once to confirm the unrecognized lines are gone and Player 2's Ctrl+, / Ctrl+. adjust their own tuning independently. |
There was a problem hiding this comment.
Note the deleted TrueTuning class had no callers at develop (DisableTrueTuning/EnableTrueTuning were never invoked); the MIDI pedal's true-tuning support uses SongTuning::GetTrueTuning, which is unchanged.
| ptr_disableTrueTuning_jmpBck = { {0x004DCCF8, baseHandle + 0x00DD978 } }; // Code | Bytes 33 c0 after the mask below (roughly 0x37 bytes away) | ||
| ptr_disableTrueTuning_forceTT = { {0x004DCCC1, baseHandle + 0x00DD941 } }; // Code | 83 7d 08 00 53 57 74 ? db 45 08 (db is the byte we want) | ||
| ptr_disableTrueTuningGate = { {0x004DCCBF, baseHandle + 0x00DD93F } }; // Code | 83 7d 08 00 53 57 74 ? db 45 08 (74 is the byte we want) | ||
| func_tuningReferenceBuilder = { {0x004DCCB0, 0x0} }; // Code | 55 8b ec 83 e4 f8 83 ec 10 83 7d 08 00 53 57 74 ? db 45 08 (we want the start of the function). Converts an arrangement cent offset to the note-detection reference, 440 * 2^(cents / 1200), and stamps it at [detection + 0x135C]. LP address is unresolved: the same mask on LP matched NoteDetection RMS code at 0x004DD93F, which is the wrong function. |
There was a problem hiding this comment.
Added, thank you. With the L&P builder address in place the pre-song tuner follows the shift on both supported versions, and I've removed the L&P caveats from the docs.
There was a problem hiding this comment.
Restored file from latest upstream
| const int targetSemitones = shouldTransposeDetection | ||
| ? DropPedalState::GetTargetSemitones() | ||
| : 0; | ||
| const float targetTrueTuning = authoredTrueTuning |
There was a problem hiding this comment.
Decoupled as you suggested. SongTuning::GetTrueTuning() now asks the Drop Pedal for the authored value first (TryGetAuthoredTrueTuning), falling back to the live read when the pedal isn't holding a shifted reference. The authored value comes from the captured baseline when one exists, or is reconstructed from the cents the builder hook was given, so a Cable octave drop on a bass-A220 song still reads 220 and the octave correction, ≤260 checks, and MIDI auto-tuner are unaffected.
| if (ring.empty()) return; | ||
|
|
||
| const float pitchRatio = ratio.load(std::memory_order_relaxed); | ||
| const double drift = 1.0 - (double)pitchRatio; |
There was a problem hiding this comment.
Good catch.
Done. Process() early-outs at a 1.0 ratio: samples pass through untouched, with no detection or splicing cost, and GetLatencyFrames() reports 0. The ring and decimation buffers keep filling during bypass so re-engagement doesn't start from a cold detector.
| } | ||
| } | ||
|
|
||
| float ClampSample(float value) |
There was a problem hiding this comment.
Done. ClampSample now uses std::clamp, keeping the std::isfinite guard in front since std::clamp on NaN is undefined.
| std::string ReadDriverNameFromRsAsioIni() | ||
| { | ||
| CSimpleIniA reader; | ||
| if (reader.LoadFile("RS_ASIO.ini") < 0) return {}; |
There was a problem hiding this comment.
Agreed, especially since it's RS_ASIO's file, not ours. It's now read exactly once at install. ReadRsAsioConfiguration() captures both input drivers/channels plus the output-driver fallback into one snapshot struct, and the file is never touched again.
| // [Asio.Input.0] channel, so a second player plays unshifted. | ||
| if (DropPedalState::IsConfiguredEnabled() && HasSecondInputConfigured()) | ||
| { | ||
| LOG_WARNING("RS_ASIO.ini configures a second input under [Asio.Input.1]. The drop " |
There was a problem hiding this comment.
There was nothing stopping me. It's implemented, for both engines. ASIO: each configured input gets its own persistent shifter, and ownership only transfers once every configured route is ready. Cable: each pitch-shifter effect is attributed to its owning player (docs/drop-pedal-multiplayer-design.md has the findings). Player 2 uses Ctrl + the Player 1 keys. Validated live with two inputs on songs with different arrangement tunings for example Creedence fortunate son
D standard lead
E standard for rhythm
E standard for bass
| /// </summary> | ||
| unsigned WINAPI DropPedalHotkeyThread() { | ||
| while (!GameState::GameClosing) { | ||
| ModManager::PollDropPedalHotkeys(); |
There was a problem hiding this comment.
You're right, the thread is gone. The drop pedal keys are registered through Keybindings::InitializeCommands and dispatch from the WndProc hook, event-driven on release. ASIO retargets synchronously in the handler; the Cable path's debounced push is polled from the existing main loop.
While wiring this I noticed DispatchCommand never evaluates ModCommand.condition for any registered command, so e.g. ToggleLoft's "is the mod enabled" check is dead code, the drop pedal handlers guard themselves internally. I've put a fix on a separate branch (dev_keybind_condition_and_twortc_fixes) so it doesn't bloat this PR.
| { | ||
| if (!hasLoggedSongTuning) | ||
| { | ||
| const uintptr_t addrTuning = MemUtil::FindDMAAddy( |
There was a problem hiding this comment.
That diagnostic has been removed entirely in the cleanup (its output was garbage in multiplayer anyway). For reference, the direct resolve existed because GetCurrentTuning returns an all-zero array both on a failed pointer resolve and for genuine E standard, and the log gate needed to distinguish them. If we ever want the shared helper for a case like this, a TryGetCurrentTuning returning success separately would do it.
2b219f5 to
b5b4c5a
Compare
|
The bus ids you shared are the same on L&P Dec 2024. Sorry for the delay. Verified in Wwiser, dumping the cache3/init.bnk |
Thanks for confirming that, will update, and no rush at all. I appreciate its a big pr. |
- Support Float32, Int24 and Int16 ASIO sample formats alongside Int32, converting to float for processing and back per buffer - Replace volatile session state with std::atomic in DropPedalState - Restore the authored pitch before ASIO takes ownership on automatic cable-to-ASIO promotion, and reapply the target on the reverse transition, so the shift is never applied by both engines at once - Return AKRESULT from the Term hook and forward the original's result, matching the IAkPluginParam contract - Remove the TrueTuning code hook, whose LP patch site landed inside NoteDetection RMS math. Detection transposition instead detours the game's tuning reference builder (cents -> Hz, stamped at song load) and adjusts the cent offset on the way in, so every consumer, including the pre-song tuner's load-time snapshot, sees the shifted reference; live target changes are written to the stamped value. Builder address is currently known for Remastered September 2022; without it, in-song detection still works via the live writes. Verified in the tuner and in-song, including non-A440 (Voodoo Child) - Remove F8 from the selectable hotkeys, since it does not always register properly
Address second round of drop pedal review feedback Restore the original TrueTuning mod byte-identical to develop, with its four DisableTrueTuning offsets; the Drop Pedal no longer touches it and uses only its own reference-builder hook. Add the Learn & Play builder address (0x002AD930), so the pre-song tuner follows the shift on both supported game versions. Decouple the two meanings of the true-tuning float: GetTrueTuning() now returns the Drop Pedal's authored value while the pedal holds a shifted reference, so the A220/Extended Range song-identity checks and the MIDI auto-tuner never see the shifted detection value. Move live pitch pushes onto the Wwise audio thread: the hotkey thread publishes an atomic pending shift, and a global callback registered with the MultiPitch plug-in runs the push loop on the render thread, where Term cannot free a param object mid-push. Replace the Cable/ASIO bool with an atomic Cable/Transitioning/Asio owner: promotion restores every live shifter's authored pitch on the audio thread and publishes Asio only on success; on failure ASIO processing is disabled and Cable keeps ownership. During Transitioning the callback forces the restore shift so a concurrent hotkey push cannot re-apply the target. The final Wwise callback drops pending work so teardown never touches param objects. Consolidate the hotkey debounce into one atomic push deadline. Mark ASIO buffer sizes outside 1-4096 frames as unsupported at CreateBuffers and clear the processing-enabled flag, so renegotiation cannot leave a stale ready state. Early-out the delay-line shifter at a 1.0 ratio: samples pass through untouched with no detection cost and zero added latency, while the ring keeps filling so re-engagement starts warm.
Remove the per-frame background plate, full Direct3D state capture, and text-width measurement. Render the Drop Pedal and engine readouts with a lightweight resolution-scaled shadow and brighter disabled text for consistent readability. This is better for performance and cleans up the code.
Update the ASIO pitch ratio only when the semitone target changes. Cache Drop Pedal overlay strings and glyph preloads until their state or font changes, and correct the documentation for neutral processing and detector workload. Moved drop pedal overlay code into its own class
Input and key handling: - Route Drop Pedal controls through the shared WndProc keybinding system and remove the dedicated polling thread. - Add Control-modified Player 2 pitch and base-tuning controls under either engine. - Read RS_ASIO.ini exactly once into a configuration snapshot; consolidate the sample-format conversions into one bit-depth-aware template. ASIO engine: - Process both configured RS_ASIO inputs with independent persistent pitch shifters, transferring processing ownership only once every configured route is ready. - Support guitar, emulated-bass and physical-bass arrangements independently per input. Cable engine: - Attribute each pitch-shifter effect to its owning player through the effect Init context's mixer pipeline node, using per-game-version node IDs; param objects are re-paired per delivery because the engine reuses them across both players' tone loads. - Identify each player's note-detection object from the tuning reference builder and keep both detection references in step with their player's target, including stamp-time adjustment of Player 2 loads and guarded live writes between them. - Apply per-player targets in the SetParam overrides and audio-thread pushes. Overlay and logging: - Rework the overlay into per-player rows showing only that pedal's own state, in the compact "E -> Eb (-1)" form, with a "No pedal in tone" indicator driven by live tone tracking; update the documentation screenshots to match. - Guard song-timer reads during pause and difficulty transitions, logging resolution failures once per episode instead of per frame. - Log pitch overrides only when the applied shift changes.
Follow configured RS_ASIO input channels and fix Learn & Play multiplayer routing. Reduce down-shift splice latency, simplify the base-tuning control, and sync the ASIO/Cable guides without Speaker Mode or harness files.
b5b4c5a to
a7a7788
Compare
Adds the digital drop pedal feature to exist within RSMods natively