Updated from SignalSmith to SoundSmith#1196
Merged
Arctis-Fireblight merged 1 commit intoRedot-Engine:masterfrom Feb 26, 2026
Merged
Updated from SignalSmith to SoundSmith#1196Arctis-Fireblight merged 1 commit intoRedot-Engine:masterfrom
Arctis-Fireblight merged 1 commit intoRedot-Engine:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new SoundSmith module that wraps the Soundsmith time-stretching library: build integration, module registration, a RefCounted C++ SoundSmith class with tempo/pitch APIs, sample processing, MP3->WAV tempo-change flow, and updated documentation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Script as Script/GDScript
participant SoundSmith as SoundSmith Instance
participant MP3Loader as AudioStreamMP3/Decoder
participant DSP as Signalsmith Stretch
participant WAVBuilder as WAV Constructor
User->>Script: call change_tempo(path, tempo, pitch)
Script->>SoundSmith: change_tempo(path, tempo, pitch)
SoundSmith->>MP3Loader: load and decode MP3 frames
MP3Loader->>SoundSmith: provide PCM frames
SoundSmith->>DSP: deinterleave and feed per-channel PCM
DSP->>DSP: apply tempo-stretch and pitch-shift
DSP->>SoundSmith: return processed PCM per channel
SoundSmith->>SoundSmith: re-interleave and convert to PCM16
SoundSmith->>WAVBuilder: build AudioStreamWAV
SoundSmith->>Script: return Ref<AudioStreamWAV>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
modules/signalsmith/signalsmith_module.cpp (2)
207-208: Local variables shadow member fieldschannelsandsample_rate.These locals shadow the class members, which is why the SCsub suppresses MSVC C4456/C4458 warnings. This makes the code harder to reason about (e.g.,
set_sample_rate(sample_rate)on line 240 passes the local, then the member is also updated as a side effect). Rename the locals to avoid confusion and remove the need for warning suppression.♻️ Suggested rename
- const int channels = mp3->is_monophonic() ? 1 : 2; - const int sample_rate = AudioServer::get_singleton()->get_mix_rate(); + const int src_channels = mp3->is_monophonic() ? 1 : 2; + const int src_sample_rate = AudioServer::get_singleton()->get_mix_rate();Then update all references in the method accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/signalsmith/signalsmith_module.cpp` around lines 207 - 208, The method declares local variables named channels and sample_rate which shadow the class member fields and cause confusing behavior; rename the locals (for example to local_channels and local_sample_rate) wherever they are declared and referenced (the lines using mp3->is_monophonic() and AudioServer::get_singleton()->get_mix_rate(), and any subsequent calls that currently use those locals such as set_sample_rate(...)), update all uses in the method to the new local names, and then remove the MSVC C4456/C4458 warning suppression since shadowing will be resolved.
64-96: Silent rejection of invalid inputs — preferERR_FAIL_COND_MSG.
set_sample_rate,set_channels,set_pitch, andset_temposilently return on invalid input. In Godot, the convention is to useERR_FAIL_COND_MSGso that callers get feedback when something goes wrong.♻️ Example for set_sample_rate
void SignalSmith::set_sample_rate(int p_rate) { - if (p_rate < 1) { - return; - } + ERR_FAIL_COND_MSG(p_rate < 1, "Sample rate must be >= 1."); sample_rate = p_rate; stretch.presetDefault(channels, sample_rate); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/signalsmith/signalsmith_module.cpp` around lines 64 - 96, Replace the silent early returns in set_sample_rate, set_channels, set_pitch, and set_tempo with Godot error macros (ERR_FAIL_COND_MSG) so invalid inputs produce clear error messages; e.g. in set_sample_rate check p_rate < 1 and call ERR_FAIL_COND_MSG(p_rate >= 1, "set_sample_rate: sample rate must be >= 1") before assigning sample_rate and calling stretch.presetDefault(channels, sample_rate), do the analogous change in set_channels using p_channels and "channels must be >= 1" before setting channels and calling stretch.presetDefault, use ERR_FAIL_COND_MSG(p_pitch > 0.0f, "set_pitch: pitch must be > 0") before calling stretch.setTransposeFactor(p_pitch), and ERR_FAIL_COND_MSG(p_tempo > 0.0f, "set_tempo: tempo must be > 0") before assigning tempo.modules/godot_physics_3d/godot_physics_server_3d.cpp (1)
352-356: Inconsistent pattern with sibling functions.The optimization to eliminate the double lookup (
owns+get_or_null→ justget_or_null) is correct. However,area_get_param(Line 369),area_attach_object_instance_id(Line 332), andarea_get_object_instance_id(Line 342) still use the old two-stepif (space_owner.owns(p_area))pattern. Consider applying the same optimization there for consistency.Also, the comment "caching prevents multiple lookups" is slightly misleading — this is consolidating two lookups into one, not caching.
Suggested comment fix
- // caching prevents multiple lookups. + // Single lookup instead of owns() + get_or_null().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/godot_physics_3d/godot_physics_server_3d.cpp` around lines 352 - 356, Replace the two-step pattern using space_owner.owns(p_area) followed by space_owner.get_or_null(p_area) in area_get_param, area_attach_object_instance_id, and area_get_object_instance_id with the single call GodotSpace3D *space = space_owner.get_or_null(p_area) and the subsequent null check (same pattern used already above), and update the comment "caching prevents multiple lookups" to something like "consolidate lookup to a single get_or_null call" to accurately reflect the change; remove references to space_owner.owns in those functions so they match the optimized pattern used elsewhere.modules/signalsmith/SCsub (1)
15-31:is_clangis detected but no clang-specific flags are applied.
is_clangis computed on Line 16 but never used — clang may emit the same-Wshadowand-Wclass-memaccesswarnings as GCC. If the shadow warnings are relevant for clang builds too, add the same suppression flags.That said, if the variable shadowing in
change_tempois addressed (as suggested insignalsmith_module.cpp), some of these suppressions may become unnecessary.♻️ Potential fix
if is_gcc: signalsmith_env.Append(CCFLAGS=["-Wno-class-memaccess"]) signalsmith_env.Append(CCFLAGS=["-Wno-shadow"]) +if is_clang: + signalsmith_env.Append(CCFLAGS=["-Wno-shadow"]) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/signalsmith/SCsub` around lines 15 - 31, The code computes is_clang but never applies clang-specific warning suppressions; update the conditional logic so clang builds get the same CCFLAGS as GCC where appropriate: when is_clang is true, call signalsmith_env.Append(CCFLAGS=[...]) to add "-Wno-class-memaccess" and "-Wno-shadow" (or only "-Wno-shadow" if class-memaccess is GCC-only) similar to the existing is_gcc block; reference the existing is_clang/is_gcc variables and the signalsmith_env.Append(CCFLAGS=...) usage, and if you also fixed the variable shadowing in change_tempo (signalsmith_module.cpp) consider removing the "-Wno-shadow" addition.modules/signalsmith/signalsmith_module.h (1)
41-69:change_tempomutates object state as a side effect — consider documenting or refactoring.
change_tempointernally callsset_sample_rate,set_channels,set_tempo,set_pitch, andreset(), silently overwriting the object's configuration. A caller who has pre-configured theSignalSmithinstance for streamingprocess()calls would lose their state after callingchange_tempo. Consider either:
- Documenting this clearly, or
- Making
change_tempoa static/standalone method that doesn't modifythis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/signalsmith/signalsmith_module.h` around lines 41 - 69, SignalSmith::change_tempo currently mutates the instance by calling set_sample_rate, set_channels, set_tempo, set_pitch and reset, which silently overwrites state used by process(); either document this side-effect clearly on change_tempo's declaration and docblock, or refactor change_tempo into a non-mutating operation (make it static or a free function) that does not modify this: e.g., copy necessary configuration from the instance or read file metadata, perform operations on a local temporary SignalSmith/stretch instance, and return the Ref<AudioStreamWAV> without calling the instance setters or reset; if you prefer to keep the method instance-bound, capture the original sample_rate/channels/tempo/pitch, run the work, and restore them before returning to avoid side effects.doc/classes/SignalSmith.xml (1)
17-21: Clarifypitchparameter edge-case behavior.The docs explain that
2.0raises by one octave and0.5lowers by one octave, but don't mention what happens with zero or negative values. If these are invalid, documenting that (or noting that values must be positive) would help users avoid silent misbehavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/classes/SignalSmith.xml` around lines 17 - 21, The documentation currently omits the pitch parameter's behavior for zero or negative values; update the SignalSmith class/method docs (the description in SignalSmith.xml that explains `tempo` and `pitch`) to state that `pitch` must be a positive, non-zero multiplier (e.g., 2.0 = +1 octave, 0.5 = -1 octave) and that zero or negative values are invalid and will be rejected (or will throw an error) by the Signalsmith engine; include a brief sentence telling users to validate inputs before calling if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/classes/SignalSmith.xml`:
- Around line 24-33: The XML docs for the SignalSmith class leave the
<description> tags empty for the methods get_last_channels and
get_last_sample_rate; update each method's <description> to clearly state what
value is returned (e.g., get_last_channels returns the number of audio channels
from the most recent processed/loaded signal, and get_last_sample_rate returns
the sample rate in Hz of that last signal) and mention any special cases (return
value when no signal exists or on error). Edit the <method
name="get_last_channels"> and <method name="get_last_sample_rate"> entries and
add concise, user-facing sentences describing the return semantics and units.
In `@modules/signalsmith/signalsmith_module.cpp`:
- Around line 192-196: The change_tempo function currently only loads MP3 via
AudioStreamMP3::load_from_file(path) which misleads callers; either rename the
function to change_tempo_mp3 or update its API to accept a generic audio stream
and handle formats. To fix: (a) if you prefer the rename, change the method name
SignalSmith::change_tempo to change_tempo_mp3 and update all callers and docs to
reflect MP3-only behavior, or (b) to support all formats, change the signature
to accept Ref<AudioStream> (or Ref<AudioStreamPlayback/AudioStream) instead of a
file path, update callers to pass a loaded audio stream, and replace the
AudioStreamMP3::load_from_file(path) call with logic that accepts the provided
Ref<AudioStream> (or add factory logic to detect filetype and load the correct
AudioStream subclass). Ensure references to change_tempo and
AudioStreamMP3::load_from_file are updated accordingly.
- Around line 213-222: The loop can leave uninitialized frames when pb->mix
fills only a partial block: after calling pb->mix(frames.ptrw() + old, 1.0f,
block) you must treat a positive mixed < block as a terminal partial fill—resize
frames to old + mixed and break immediately (similar to the existing mixed <= 0
branch). Modify the loop around frames, old, block and pb->mix so that when
mixed > 0 but mixed < block you call frames.resize(old + mixed) and break to
avoid preserving garbage.
---
Nitpick comments:
In `@doc/classes/SignalSmith.xml`:
- Around line 17-21: The documentation currently omits the pitch parameter's
behavior for zero or negative values; update the SignalSmith class/method docs
(the description in SignalSmith.xml that explains `tempo` and `pitch`) to state
that `pitch` must be a positive, non-zero multiplier (e.g., 2.0 = +1 octave, 0.5
= -1 octave) and that zero or negative values are invalid and will be rejected
(or will throw an error) by the Signalsmith engine; include a brief sentence
telling users to validate inputs before calling if needed.
In `@modules/godot_physics_3d/godot_physics_server_3d.cpp`:
- Around line 352-356: Replace the two-step pattern using
space_owner.owns(p_area) followed by space_owner.get_or_null(p_area) in
area_get_param, area_attach_object_instance_id, and area_get_object_instance_id
with the single call GodotSpace3D *space = space_owner.get_or_null(p_area) and
the subsequent null check (same pattern used already above), and update the
comment "caching prevents multiple lookups" to something like "consolidate
lookup to a single get_or_null call" to accurately reflect the change; remove
references to space_owner.owns in those functions so they match the optimized
pattern used elsewhere.
In `@modules/signalsmith/SCsub`:
- Around line 15-31: The code computes is_clang but never applies clang-specific
warning suppressions; update the conditional logic so clang builds get the same
CCFLAGS as GCC where appropriate: when is_clang is true, call
signalsmith_env.Append(CCFLAGS=[...]) to add "-Wno-class-memaccess" and
"-Wno-shadow" (or only "-Wno-shadow" if class-memaccess is GCC-only) similar to
the existing is_gcc block; reference the existing is_clang/is_gcc variables and
the signalsmith_env.Append(CCFLAGS=...) usage, and if you also fixed the
variable shadowing in change_tempo (signalsmith_module.cpp) consider removing
the "-Wno-shadow" addition.
In `@modules/signalsmith/signalsmith_module.cpp`:
- Around line 207-208: The method declares local variables named channels and
sample_rate which shadow the class member fields and cause confusing behavior;
rename the locals (for example to local_channels and local_sample_rate) wherever
they are declared and referenced (the lines using mp3->is_monophonic() and
AudioServer::get_singleton()->get_mix_rate(), and any subsequent calls that
currently use those locals such as set_sample_rate(...)), update all uses in the
method to the new local names, and then remove the MSVC C4456/C4458 warning
suppression since shadowing will be resolved.
- Around line 64-96: Replace the silent early returns in set_sample_rate,
set_channels, set_pitch, and set_tempo with Godot error macros
(ERR_FAIL_COND_MSG) so invalid inputs produce clear error messages; e.g. in
set_sample_rate check p_rate < 1 and call ERR_FAIL_COND_MSG(p_rate >= 1,
"set_sample_rate: sample rate must be >= 1") before assigning sample_rate and
calling stretch.presetDefault(channels, sample_rate), do the analogous change in
set_channels using p_channels and "channels must be >= 1" before setting
channels and calling stretch.presetDefault, use ERR_FAIL_COND_MSG(p_pitch >
0.0f, "set_pitch: pitch must be > 0") before calling
stretch.setTransposeFactor(p_pitch), and ERR_FAIL_COND_MSG(p_tempo > 0.0f,
"set_tempo: tempo must be > 0") before assigning tempo.
In `@modules/signalsmith/signalsmith_module.h`:
- Around line 41-69: SignalSmith::change_tempo currently mutates the instance by
calling set_sample_rate, set_channels, set_tempo, set_pitch and reset, which
silently overwrites state used by process(); either document this side-effect
clearly on change_tempo's declaration and docblock, or refactor change_tempo
into a non-mutating operation (make it static or a free function) that does not
modify this: e.g., copy necessary configuration from the instance or read file
metadata, perform operations on a local temporary SignalSmith/stretch instance,
and return the Ref<AudioStreamWAV> without calling the instance setters or
reset; if you prefer to keep the method instance-bound, capture the original
sample_rate/channels/tempo/pitch, run the work, and restore them before
returning to avoid side effects.
Comment on lines
+24
to
+33
| <method name="get_last_channels" qualifiers="const"> | ||
| <return type="int" /> | ||
| <description> | ||
| </description> | ||
| </method> | ||
| <method name="get_last_sample_rate" qualifiers="const"> | ||
| <return type="int" /> | ||
| <description> | ||
| </description> | ||
| </method> |
There was a problem hiding this comment.
Missing descriptions for get_last_channels and get_last_sample_rate.
Both getter methods have empty <description> tags. Users won't know what these return without reading source code.
📝 Suggested descriptions
<method name="get_last_channels" qualifiers="const">
<return type="int" />
<description>
+ Returns the number of audio channels last used during processing.
</description>
</method>
<method name="get_last_sample_rate" qualifiers="const">
<return type="int" />
<description>
+ Returns the sample rate, in Hz, last used during processing.
</description>
</method>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <method name="get_last_channels" qualifiers="const"> | |
| <return type="int" /> | |
| <description> | |
| </description> | |
| </method> | |
| <method name="get_last_sample_rate" qualifiers="const"> | |
| <return type="int" /> | |
| <description> | |
| </description> | |
| </method> | |
| <method name="get_last_channels" qualifiers="const"> | |
| <return type="int" /> | |
| <description> | |
| Returns the number of audio channels last used during processing. | |
| </description> | |
| </method> | |
| <method name="get_last_sample_rate" qualifiers="const"> | |
| <return type="int" /> | |
| <description> | |
| Returns the sample rate, in Hz, last used during processing. | |
| </description> | |
| </method> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/classes/SignalSmith.xml` around lines 24 - 33, The XML docs for the
SignalSmith class leave the <description> tags empty for the methods
get_last_channels and get_last_sample_rate; update each method's <description>
to clearly state what value is returned (e.g., get_last_channels returns the
number of audio channels from the most recent processed/loaded signal, and
get_last_sample_rate returns the sample rate in Hz of that last signal) and
mention any special cases (return value when no signal exists or on error). Edit
the <method name="get_last_channels"> and <method name="get_last_sample_rate">
entries and add concise, user-facing sentences describing the return semantics
and units.
mcdubhghlas
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
modules/soundsmith/soundsmith_module.cpp (1)
240-248:change_tempomutates object state as a side effect.Calling
set_sample_rate,set_channels,set_tempo,set_pitch, andreset()here overwrites the object's internal configuration. A caller who had set up theSoundSmithinstance for streamingprocess()calls would have their state silently overwritten. Consider either documenting this prominently or makingchange_tempoa static/standalone method that creates its own internal stretch instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/soundsmith/soundsmith_module.cpp` around lines 240 - 248, The change_tempo implementation is mutating the SoundSmith instance by calling set_sample_rate, set_channels, set_tempo, set_pitch and reset before calling process, which silently overwrites any caller-managed streaming state; instead make change_tempo non-mutating by creating and configuring a temporary SoundSmith (or a dedicated internal stretch instance) inside change_tempo, call process on that temporary instance, and return its result so the original object's state (methods: set_sample_rate, set_channels, set_tempo, set_pitch, reset, process) is left unchanged—or alternatively convert change_tempo into a static/standalone function (e.g., change_tempo_static) that constructs and uses its own SoundSmith locally and does not touch the caller's instance state.doc/classes/SoundSmith.xml (1)
3-7: Naming inconsistency: "Soundsmith" vs the library name "signalsmith-stretch".The class is
SoundSmith, the docs say "Soundsmith", the library issignalsmith-stretch, and the PR title says "Signal Smith." Consider aligning the terminology — at minimum the docs should reference the actual upstream library name ("Signalsmith Stretch") so users can find the library documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/classes/SoundSmith.xml` around lines 3 - 7, The docs use "Soundsmith" inconsistently with the upstream library name; update the class documentation for SoundSmith to reference the correct upstream library "Signalsmith Stretch" (or "signalsmith-stretch") and make the terminology consistent across brief_description and description; specifically replace occurrences of "Soundsmith" with "Signalsmith Stretch" and add a short parenthetical pointing to the npm/package name "signalsmith-stretch" so users can find upstream docs while keeping the class name SoundSmith intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/classes/SoundSmith.xml`:
- Around line 24-33: The documentation for methods get_last_channels and
get_last_sample_rate is missing descriptions; update the <description> element
for get_last_channels to a short sentence like "Returns the most recently
configured channel count." and for get_last_sample_rate to "Returns the most
recently configured sample rate in Hz." so the XML API docs are complete and
informative.
In `@modules/soundsmith/SCsub`:
- Around line 15-31: The MSVC detection is too brittle: replace the string-match
logic that sets is_msvc (the `"cl" in cxx` check) with the environment-provided
MSVC flag (use env.msvc or the equivalent boolean used elsewhere) so
soundsmith_env only receives MSVC-specific CCFLAGS when the environment actually
indicates MSVC; update the SCsub block that sets is_msvc and the conditional
that appends "/wd4456" and "/wd4458" to gate on env.msvc instead of the current
string check, and then remove or narrow the module-wide shadow-warning
suppressions (keep the GCC -Wno-shadow handling only for true GCC/Clang via
is_gcc/is_clang) while addressing the underlying shadowing in change_tempo() /
soundsmith_module.h (rename or disambiguate the local variables `channels` and
`sample_rate`) rather than silencing the warnings broadly.
In `@modules/soundsmith/soundsmith_module.cpp`:
- Around line 213-222: The loop that fills frames using pb->mix incorrectly
handles partial and negative returns from mix(): when mixed < block but > 0 it
should trim the unused portion and stop, and when mixed <= 0 it must not shrink
existing valid data. Change the logic around mixed so that: if mixed <= 0 simply
break (don’t call frames.resize with a negative value), if mixed < block then
call frames.resize(old + mixed) and break, otherwise continue (mixed == block
keeps looping). Update the block that uses frames, old, mixed and pb->mix
accordingly.
- Around line 133-138: The computation of output_frames (using tf, input_frames,
tempo and std::lround) can overflow when input_frames / tf exceeds int/long
range and later when multiplying by channels; change the logic in the block that
computes output_frames so you (1) compute the desired double result safely, (2)
clamp that double to a safe integer range using std::numeric_limits<int>::max()
(or a configurable sensible_max_frames) before calling std::lround/cast, and (3)
before any multiplication by channels check/clamp against overflow (e.g., ensure
output_frames <= std::numeric_limits<int>::max() / std::max(1, channels) or
promote to size_t and check against SIZE_MAX), updating the variables around tf,
output_frames, input_frames and channels accordingly to guarantee no undefined
behavior or integer overflow.
- Around line 207-208: The local variables `channels` and `sample_rate` in the
method shadow the class members and call `AudioServer::get_singleton()` without
null-checking; change the locals to distinct names (e.g., `ch` and `rate`),
check that `AudioServer::get_singleton()` is not null before calling
`get_mix_rate()` (and handle the null case appropriately — return/fail or use a
safe default rate), and update all subsequent uses in this method (places that
currently use `channels` -> `ch` and `sample_rate` -> `rate`, including the
usages around the original lines 228, 232, 240-241, 267-268). Ensure
`mp3->is_monophonic()` is still used to set `ch` and avoid suppressing shadow
warnings globally.
In `@modules/soundsmith/soundsmith_module.h`:
- Line 38: The build fails because the include
"signalsmith-stretch/signalsmith-stretch.h" referenced in soundsmith_module.h is
not present in the repo; either add/vend the signalsmith-stretch library into
the project's thirdparty tree (creating thirdparty/signalsmith-stretch/ with its
headers and updating any build files to include that directory) or remove the
dependency by deleting the include and any uses of symbols from
signalsmith-stretch in soundsmith_module.h (or wrap them behind a compile-time
macro such as HAVE_SIGNALSMITH_STRETCH and provide a fallback implementation).
Ensure the chosen approach also updates build configuration so the include path
resolves.
---
Nitpick comments:
In `@doc/classes/SoundSmith.xml`:
- Around line 3-7: The docs use "Soundsmith" inconsistently with the upstream
library name; update the class documentation for SoundSmith to reference the
correct upstream library "Signalsmith Stretch" (or "signalsmith-stretch") and
make the terminology consistent across brief_description and description;
specifically replace occurrences of "Soundsmith" with "Signalsmith Stretch" and
add a short parenthetical pointing to the npm/package name "signalsmith-stretch"
so users can find upstream docs while keeping the class name SoundSmith intact.
In `@modules/soundsmith/soundsmith_module.cpp`:
- Around line 240-248: The change_tempo implementation is mutating the
SoundSmith instance by calling set_sample_rate, set_channels, set_tempo,
set_pitch and reset before calling process, which silently overwrites any
caller-managed streaming state; instead make change_tempo non-mutating by
creating and configuring a temporary SoundSmith (or a dedicated internal stretch
instance) inside change_tempo, call process on that temporary instance, and
return its result so the original object's state (methods: set_sample_rate,
set_channels, set_tempo, set_pitch, reset, process) is left unchanged—or
alternatively convert change_tempo into a static/standalone function (e.g.,
change_tempo_static) that constructs and uses its own SoundSmith locally and
does not touch the caller's instance state.
Arctis-Fireblight
approved these changes
Feb 26, 2026
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.
Requested.
Summary by CodeRabbit
New Features
Documentation