napframework / nap

NAP Framework source code
https://nap-framework.tech
Mozilla Public License 2.0
410 stars 23 forks source link

Napportaudio module #18

Closed stijnvanbeek closed 9 months ago

stijnvanbeek commented 1 year ago

I order to be able to add optional functionality to a module, the file(GLOB_RECURSE) in cmake/nap_module.cmake is changed to a file(GLOB). Subdirectories now have to be added manually per module in module_extra.cmake . This allows for more customization. I wrote a function add_source_dir() for this in macros_and_functions.cmake. (a very large file which imho could be split up into different smaller files for maintainability)

Breaking changes:

cklosters commented 11 months ago

Looping in @cheywood for the build system changes

cklosters commented 11 months ago

Looking good, it's a welcome but (from what I understand) breaking change. This means we can't merge it for 0.6.X because it will break existing project structures, am I correct? People using napaudio would also have to include napportaudio. Next to the project include, is there something else in code people need to be aware of when porting in the future?

cklosters commented 11 months ago

Have the third party libraries (portaudio etc.) been recompiled or simply moved? The build passes so I assume all library paths are handlled correctly.

stijnvanbeek commented 11 months ago

Have the third party libraries (portaudio etc.) been recompiled or simply moved? The build passes so I assume all library paths are handlled correctly.

The portaudio libraries just have been moved from napaudio to napportaudio

stijnvanbeek commented 11 months ago

Looking good, it's a welcome but (from what I understand) breaking change. This means we can't merge it for 0.6.X because it will break existing project structures, am I correct? People using napaudio would also have to include napportaudio. Next to the project include, is there something else in code people need to be aware of when porting in the future?

Yes, if people want to include subdirectories of src/ into their sources for their modules they need to explicitly state these in module_extra.cmake using the new add_source_dir() function. The reason for this is to enable module developers to add platform specific or other optional sources. That's it though.

cklosters commented 11 months ago

Looking good, it's a welcome but (from what I understand) breaking change. This means we can't merge it for 0.6.X because it will break existing project structures, am I correct? People using napaudio would also have to include napportaudio. Next to the project include, is there something else in code people need to be aware of when porting in the future?

... if people want to include subdirectories of src/ into their sources for their modules they need to explicitly state these in module_extra.cmake using the new add_source_dir() function. The reason for this is to enable module developers to add platform specific or other optional sources. That's it though.

Isn't it better to keep using file(GLOB_RECURSE) in src and explicitly add another directory that sits besides it, which is platform specific? That way we don't change project include behaviour.

Also: some functionality has been moved from the audio service to nap::audio::PortAudioService, which means client apps that previously referenced the audio service must now reference the port audio service if they wish to access port audio related features. So that's another example of a project breaking change. It's important that after merging this we can explain what must change in code when people want to update / port their projects. I miss that from your description, hence my question.

Are there other major include / interface differences? (midi for example).

stijnvanbeek commented 11 months ago

Looking good, it's a welcome but (from what I understand) breaking change. This means we can't merge it for 0.6.X because it will break existing project structures, am I correct? People using napaudio would also have to include napportaudio. Next to the project include, is there something else in code people need to be aware of when porting in the future?

... if people want to include subdirectories of src/ into their sources for their modules they need to explicitly state these in module_extra.cmake using the new add_source_dir() function. The reason for this is to enable module developers to add platform specific or other optional sources. That's it though.

That's just an addition right? It doesn't break existing functionality because currently we don't include module src sub directories, correct?

Well I do that, but I guess I am the only one so that's correct. ;)

Also: some functionality has been moved from the audio service to nap::audio::PortAudioService, which means client apps that previously referenced the audio service must now reference the port audio service if they wish to access port audio related features. So that's another example of a project breaking change. It's important that when we merge this I can explain what people must change in order to port / update their projects. I miss that from your description, hence my question. Are there other major include / interface differences? (midi for example)

Ah yes I get you now, that's right. Everything audio device related has been moved from the AudioService to the PortAudioService. The AudioService just provides the audio::NodeManager now, but starting/stopping the processing has to be done by a client module like the PortAudioService. You could for example just include napaudio now (without napportaudio) and use it for non-realtime audio processing.

cheywood commented 11 months ago

Hey, my only real thoughts are similar to the ones you've already raised Coen; by replacing GLOB_RECURSE it modifies the module template that is provided to people. I imagine Stijn's right that nobody else is using it but it's also an assumption. So you would need to make it clear in the release notes that module sources in sub-directories will no longer be automatically pulled in and point towards updated documentation for how to have module source in subdirs.

FWIW I don't have a strong feeling about whether the previous or current approach is better. Obviously the older one was less work for the module creator but provided less control. Globbing is undesirable and recursive globbing is worse.

Are there use cases for add_source_dir separate of adding a source directory to a module? If not I personally like eg. add_source_dir_to_module but I tend to err on verbosity. Suffixing it would allow you to possibly extend the functionality later without impacting non-module uses?

a very large file which imho could be split up into different smaller files for maintainability

Agreed. Previously it was multiple files seemingly arbitrarily split.

stijnvanbeek commented 10 months ago

Are there use cases for add_source_dir separate of adding a source directory to a module? If not I personally like eg. add_source_dir_to_module but I tend to err on verbosity. Suffixing it would allow you to possibly extend the functionality later without impacting non-module uses?

Yes! I am as we speak using the same function for a non module target actually.

a very large file which imho could be split up into different smaller files for maintainability

Agreed. Previously it was multiple files seemingly arbitrarily split.

Haha! Ok

cklosters commented 10 months ago

Updated target to be 0.7 instead of main.

stijnvanbeek commented 10 months ago

This fails now on Mac OS but I can't see why as I don't have access to the teamcity server.

cklosters commented 10 months ago

It fails on macOS because macOS is not a supported target anymore. I will remove the macOS validation stamp when we merge 0.7.0.

[04:47:44]  [Step 1/1] CMake Error at cmake/macros_and_functions.cmake:51 (message):
[04:47:44]  [Step 1/1]   macOS is no longer supported as a target operating system.  Our development focus has shifted to ensure compatibility with other open platforms.
[04:47:44]  [Step 1/1] Call Stack (most recent call first):
[04:47:44]  [Step 1/1]   CMakeLists.txt:16 (bootstrap_environment)

Until then ignore macOS.