node method for Synth goes out of bounds of phase in next.rs

[SnoCold]

Describe the bug
The Handling of Node::Sine in the method node of Synth implementation goes out of bounds for the piano example
the osc count keeps on increasing with no check, leading to out-of-bounds panic

To Reproduce
Steps to reproduce the behavior:

run> cargo run --example piano

Expected behavior
piano.wav should get generated without errors

Tried FIx
I added an if block to return if the osc ever gets equal or higher to phase length

in next.rs
inside fn node

 Node::Sine { hz } => {
                if *osc >= self.phase.len() { //Added this block
                    return
                }
                let this = *osc;
                *osc += 1;
                self.node(hz, delta, osc); 
                for i in self.stack.last_mut().unwrap() {
                    let hertz = *i;
                    *i = libm::cosf(self.phase[this] * core::f32::consts::TAU);
                    self.phase[this] = (self.phase[this] + delta * hertz) % 1.0;
                }
            }

Desktop (please complete the following information):

• OS: Debian on Wsl2
• Version: 6.6.87.2-microsoft-standard-WSL2

Additional context
Backtrace:

thread 'main' panicked at src/next.rs:520:47:
index out of bounds: the len is 30 but the index is 30
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs:697:5
   1: core::panicking::panic_fmt
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panicking.rs:75:14
   2: core::panicking::panic_bounds_check
             at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panicking.rs:280:5
   3: <usize as core::slice::index::SliceIndex<[T]>>::index
             at /home/nishshuk/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:274:10
   4: core::slice::index::<impl core::ops::index::Index<I> for [T]>::index
             at /home/nishshuk/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:18:15
   5: <alloc::vec::Vec<T,A> as core::ops::index::Index<I>>::index
             at /home/nishshuk/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:3571:9
   6: twang::next::Synth::node
             at ./src/next.rs:520:47
   7: twang::next::Synth::node
             at ./src/next.rs:615:22
   8: twang::next::Synth::node
             at ./src/next.rs:505:26
   9: twang::next::Synth::node
             at ./src/next.rs:505:26
  10: twang::next::Synth::node
             at ./src/next.rs:615:22
  11: twang::next::Synth::synthesize
             at ./src/next.rs:490:14
  12: <twang::next::SynthIter as core::iter::traits::iterator::Iterator>::next
             at ./src/next.rs:635:20
  13: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::next
             at /home/nishshuk/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/adapters/map.rs:107:19
  14: <&mut fon::audio::AudioSink<Chan,_> as fon::sink::Sink<Chan,_>>::sink_with
             at /home/nishshuk/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/fon-0.6.0/src/audio.rs:192:48
  15: <fon::audio::AudioSink<Chan,_> as fon::sink::Sink<Chan,_>>::sink_with
             at /home/nishshuk/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/fon-0.6.0/src/audio.rs:172:9
  16: twang::next::Synth::stream
             at ./src/next.rs:477:14
  17: piano::main
             at ./examples/piano.rs:68:11
  18: core::ops::function::FnOnce::call_once
             at /home/nishshuk/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:253:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[SnoCold]

I would like to work on the PR for this.. Please let me know if my fix is incorrect and maybe there is a better way

[AldaronLau]

@SnoCold thanks for the issue! I've been trying to figure out the best way to go forward with twang's API, and the next module will likely be removed in the future. The most recent API design is in the tree module, although it is too incomplete for the piano example (currently missing the mixing and gain operators). Would you be interested in working on adding them?

[SnoCold]

@AldaronLau Thank you for the response!! I see, I will take a look and try to come up with something. I'd like to be proficient in Rust and I like working with stuff related to music so your repository is a great hangout for me..

Should we then close this issue?

[AldaronLau]

@SnoCold I think we should keep it open - it's still good for tracking that the piano example isn't working, and we can close it once fixed. I think the gain will likely be implemented similar to existing oscillators (in the twang::tree::osc::bezier module there are two parameters like how gain and mixing could work), and the mixing should be the same initially, too, to keep it simple (although eventually might have a more complicated implementation - thinking potentially inspired on the specializer API). Let me know if you have any questions

[SnoCold]

hi @AldaronLau sorry for the delay, I have been going over the code but I am just not that proficient in music production yet. I will wait till I become more confident but wouldn't want to hold up your development timeline. Apologies for being a let down, I hoped I could have done more at this stage.

however I will be around to test new changes and finding bugs and reporting them. I am definitely good at that

[AldaronLau]

@SnoCold no worries! I'm not developing this crate especially fast either, but thanks for being around to test changes. I'll try to get around to fixing the example in the near future