feat: Graph refactor chapter II#991
Conversation
…StereoPannerNode constraints
maciejmakowski2003
force-pushed
the
feat/graph_refactor_2
branch
2 times, most recently
from
62ae703 to
71f1fd0
Compare
There was a problem hiding this comment.
just checking in
is it possible to call start and stop concurently (eg at the same time)?
poneciak57
left a comment
poneciak57
left a comment
There was a problem hiding this comment.
Overally it looks really great. One global issue is lack of high level documentation. It would be great to write/generate some high level design doc on top of that or keep existing ones in sync with the code
But as i said overally good job 👍
| delay_ring::bufferOperation( | ||
| delayBuffer, audioBuffer_, framesToProcess, writeIndex, delay_ring::BufferAction::WRITE); |
There was a problem hiding this comment.
why u have Action type instead of having two methods write/read?
There was a problem hiding this comment.
because it shares a lot of code under the hood, but it would be hard to split it into two separate functions and put common code elsewhere, this way we can easily control what is shared and what is not
There was a problem hiding this comment.
@copilot address this architecture choice discussion
There was a problem hiding this comment.
hope he will not empty out my wallet for this question ....
There was a problem hiding this comment.
eh...
I would challenge the assumption that minimizing code duplication should be the primary goal.
Read and write are distinct operations with different semantics. The fact that they share some implementation details doesn't necessarily mean they should be exposed through a single function with an Action enum.
I'd generally prefer separate read() and write() APIs and keep any shared logic internal. A slightly larger implementation is often a reasonable trade-off for a cleaner and more explicit interface.
There was a problem hiding this comment.
make sure this file is compiled with -O3 -ffast-math
|
🎉 |
3 participants
Closes RNAA-433
Introduced changes
Checklist