feat: build RakVoice into the core library#28
Conversation
|
Warning Review limit reached
More reviews will be available in 7 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
WalkthroughThis PR centralizes voice dependency fetching into a new CMake module, integrates RakVoice sources and header into the core MafiaNet library (linking opus/rnnoise privately), removes separate RakVoice targets from extensions/samples, and updates sample includes to ChangesRakVoice Core Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Kheartz
force-pushed
the
feature/rakvoice-in-core
branch
2 times, most recently
from
db1e36e to
be25fbe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/CMakeLists.txt (1)
317-326:⚠️ Potential issue | 🟠 MajorMafiaNetStatic install/export should propagate
opus/rnnoiselink requirements
- In
Source/CMakeLists.txt,opus/rnnoiseare added asPRIVATElinks using$<BUILD_INTERFACE:opus>and$<BUILD_INTERFACE:rnnoise>, so they won’t be part of the exported install-time usage requirements.cmake/MafiaNetConfig.cmake.inonly declaresfind_dependency(OpenSSL)andfind_dependency(Threads)(nofind_dependencyentries foropus/rnnoise), so installed static consumers won’t get the dependencies needed to resolveopus_*/rnnoise_*when using the RakVoice codepath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/CMakeLists.txt` around lines 317 - 326, The install/export omits opus and rnnoise usage requirements because in target_link_libraries(${target_name}) they are added as PRIVATE via $<BUILD_INTERFACE:opus> and $<BUILD_INTERFACE:rnnoise>, so change those entries to propagate to consumers (use PUBLIC or $<BUILD_INTERFACE:...,$<INSTALL_INTERFACE:...> style or add an INTERFACE link target) so installed MafiaNetStatic consumers see opus/rnnoise; then update cmake/MafiaNetConfig.cmake.in to add find_dependency(opus) and find_dependency(rnnoise) (or otherwise export/import targets for those libs) so the generated config pulls in the runtime build dependencies when an installed package consumes the RakVoice codepath.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Source/CMakeLists.txt`:
- Around line 317-326: The install/export omits opus and rnnoise usage
requirements because in target_link_libraries(${target_name}) they are added as
PRIVATE via $<BUILD_INTERFACE:opus> and $<BUILD_INTERFACE:rnnoise>, so change
those entries to propagate to consumers (use PUBLIC or
$<BUILD_INTERFACE:...,$<INSTALL_INTERFACE:...> style or add an INTERFACE link
target) so installed MafiaNetStatic consumers see opus/rnnoise; then update
cmake/MafiaNetConfig.cmake.in to add find_dependency(opus) and
find_dependency(rnnoise) (or otherwise export/import targets for those libs) so
the generated config pulls in the runtime build dependencies when an installed
package consumes the RakVoice codepath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d4952f9-31d1-4c3a-ac0e-7f21d1aa3459
📒 Files selected for processing (15)
CMakeLists.txtDependentExtensions/CMakeLists.txtSamples/RakVoice/CMakeLists.txtSamples/RakVoice/main.cppSamples/RakVoiceDSound/CMakeLists.txtSamples/RakVoiceDSound/DSoundVoiceAdapter.hSamples/RakVoiceDSound/main.cppSamples/RakVoiceFMOD/CMakeLists.txtSamples/RakVoiceFMOD/FMODVoiceAdapter.hSamples/RakVoiceFMOD/main.cppSource/CMakeLists.txtSource/include/mafianet/RakVoice.hSource/src/RakVoice.cppcmake/FetchDependencies.cmakecmake/FetchVoiceDependencies.cmake
💤 Files with no reviewable changes (3)
- Samples/RakVoiceDSound/CMakeLists.txt
- Samples/RakVoiceFMOD/CMakeLists.txt
- Samples/RakVoice/CMakeLists.txt
Kheartz
force-pushed
the
feature/rakvoice-in-core
branch
3 times, most recently
from
0b2e165 to
9e963c0
Compare
Move RakVoice from DependentExtensions into the core MafiaNet library so
voice support is always built, instead of being gated behind
MAFIANET_BUILD_SAMPLES.
- Move RakVoice.{h,cpp} to Source/include/mafianet and Source/src, and add
them to the core source/header lists.
- Add cmake/FetchVoiceDependencies.cmake to fetch Opus and RNNoise
unconditionally, included before the Source subdirectory. The core
targets link them via $<BUILD_INTERFACE:...> so the install/export set
is unaffected.
- Drop Opus/RNNoise from FetchDependencies.cmake (now sample-only:
bzip2, miniupnpc, jansson) and remove the standalone RakVoice target.
- Repoint the RakVoice samples at the MafiaNet target and the
mafianet/RakVoice.h header.
Fix the RNNoise target source list while here: it omitted
parse_lpcnet_weights.c and the model rnnoise_data.c, leaving
rnn_parse_weights/init_rnnoise/rnnoise_arrays unresolved when linking a
shared library or executable.
Kheartz
force-pushed
the
feature/rakvoice-in-core
branch
from
9e963c0 to
40e21a0
Compare
2 participants
Move RakVoice from DependentExtensions into the core MafiaNet library so voice support is always built, instead of being gated behind MAFIANET_BUILD_SAMPLES.
Fix the RNNoise target source list while here: it omitted parse_lpcnet_weights.c and the model rnnoise_data.c, leaving rnn_parse_weights/init_rnnoise/rnnoise_arrays unresolved when linking a shared library or executable.
Summary by CodeRabbit