Page MenuHome

Audaspace 1.3 patch for Blender 2.8
AbandonedPublic

Authored by Joerg Mueller (nexyon) on Jun 18 2017, 8:37 PM.

Details

Summary

This patch removes the old audaspace from Blender and instead adds 1.3 to the extern library directory.

Changes from the old audaspace:

  • The whole library was refactored to use C++11.
  • This brings a lot of stability and speed improvements.
  • The Python API has major improvements, including features that were requested by Blender users for a while already.
  • In terms of the Blender UI the only change probably is if OpenAL reports multiple available devices, they can choose from those in the user settings.
  • Changelog since Audaspace 1.0 (the first C++11 version) can be found here: https://github.com/audaspace/audaspace/blob/master/CHANGES

What needs to be done:

  • check if it works for Windows
  • check if it works for MacOS
  • check if it works for release builds
  • check if everything is fine how I did it

Diff Detail

Repository
rB Blender

Event Timeline

Ray Molenkamp (LazyDodo) requested changes to this revision.Jun 18 2017, 10:59 PM

Currently doesn't build on windows, needs a little work in the parts where it looks for both python and fftw3 (there may be other things, but for now this is where it is most unhappy)

This revision now requires changes to proceed.Jun 18 2017, 10:59 PM

Also if an unmodified audaspace works, could we possibly take it out of extern and move it to install_deps.sh / svn like most other externals?

Thanks, I'll have a look at that. Can you show me the errors that you get with Python? For fftw3 it is not enough to enable it with cmake (WITH_FFTW3)?

Also if an unmodified audaspace works, could we possibly take it out of extern and move it to install_deps.sh / svn like most other externals?

I had a longer discussion with @Sergey Sharybin (sergey) already about that and the plan currently is to have it similar to cycles, developing it inside Blender, but synchronizing it to an outside repository in case anyone wants to use it outside of Blender.

Thanks, I'll have a look at that. Can you show me the errors that you get with Python? For fftw3 it is not enough to enable it with cmake (WITH_FFTW3)?

It's calling find_package to find both of them, the platform files in build_files/cmake/platform already do that and setup the correct variables, (for windows we actually bypass find_package and use hardcoded paths to the libs folder unless you explicitly request the use of find_package) so in the audaspace cmakefile you shouldn't have to do anything really to get the includepaths and libs vars (also we build everything static, so the globbing for dll's is also not needed)

Also if an unmodified audaspace works, could we possibly take it out of extern and move it to install_deps.sh / svn like most other externals?

I had a longer discussion with @Sergey Sharybin (sergey) already about that and the plan currently is to have it similar to cycles, developing it inside Blender, but synchronizing it to an outside repository in case anyone wants to use it outside of Blender.

Well if it's mature enough to stand on it's own, i'm happy to treat it as an external and build libs for it, if you want to keep it in the blender tree that's fine with me too

What are the bindings and demos to do in a version bundled to Blender? Blender's only bindings are RNA ones, you should not be adding extra bindings. Demos for some particular library used by Blender is also more like a waste of space in our repository.

You also should not be having own FindFoo scripts, especially if we've got similar ones already. This would be a straight way to cause conflicts and buggy build configurations if you'll mix CMake magic from Blender and library.

What are the bindings and demos to do in a version bundled to Blender? Blender's only bindings are RNA ones, you should not be adding extra bindings.

Those are the language bindings for C and Python, both which are important for Blender.

Demos for some particular library used by Blender is also more like a waste of space in our repository.

If audaspace is to live in side of Blender, the demos of course have to be there as well. Of course there is still the posibility to make it an actual external as @Ray Molenkamp (LazyDodo) suggested.

You also should not be having own FindFoo scripts, especially if we've got similar ones already. This would be a straight way to cause conflicts and buggy build configurations if you'll mix CMake magic from Blender and library.

I'll make sure they are not called when built with Blender, similar to what Cycles is doing.

Joerg Mueller (nexyon) updated this revision to Diff 9118.EditedAug 13 2017, 2:47 PM
Joerg Mueller (nexyon) edited edge metadata.

Disabled FFTW3 for blender's audaspace (it wasn't used anyway) and fixed cmake stuff (no more find_package calls, no cmake modules added, no dlls added, ...).

@Sergey Sharybin (sergey) I hope you can live with a few unneeded files (short demo code, unused cmake modules) in the repository. If not we have to make it an actual external library. Then linux folks will have to either build and install it themselves or hope that their distro will add it soon.

@Ray Molenkamp (LazyDodo) The issues you mentioned should be fixed now, can you please check if everything works for you?

Could we have a simple shell/py script that copies from the original git repo and removes demos or any other files that aren't needed?

We can just have an blender_update.py script that handles this and keep it in version control.

@Joerg Mueller (nexyon) - in patch description, could you summarize what changes users can expect from this patch? (if any) or changes from Py API.

If its just internal changes thats OK too but nice to know.

@Ray Molenkamp (LazyDodo) Can you check if it works this time please? (I guess not, maybe you can find the errors, I can't reproduce)

Joerg Mueller (nexyon) edited the summary of this revision. (Show Details)Aug 13 2017, 5:10 PM
Joerg Mueller (nexyon) edited the summary of this revision. (Show Details)
extern/CMakeLists.txt
118–141

These should be done in extern/audaspace/CMakeLists.txt since this changes the namespace in extern/

At the moment we get away with it because its the last library - but we can only do this once, so its not a good convention.

Possible solutions are to either have an intermediate CMakeLists.txt which includes these settings, then uses audaspace's.

It might be inconvenient to add an extra subdir though.

Another option could be to have audaspace run a configuration file, if a variable is set - then we can have AUDASPACE_CONFIG file contains these variables and runs when set. (call it blender_audaspace.cmake or so)

Fixed some more bugs, removed unnecessary files and added a cmake config files. Now everyone is hopefully happy?

extern/audaspace/CMakeLists.txt
316

This needs some extra guarding so we don't mess up the standard blender flags

if(MSVC)
	if(NOT BLENDER_AUDASPACE)
		list(APPEND CMAKE_C_FLAGS_DEBUG "/Zi /Od")
		list(APPEND CMAKE_CXX_FLAGS_DEBUG "/Zi /Od")
		list(APPEND CMAKE_SHARED_LINKER_FLAGS_DEBUG "/DEBUG")
		list(APPEND CMAKE_STATIC_LINKER_FLAGS_DEBUG "/DEBUG")
		list(APPEND CMAKE_EXE_LINKER_FLAGS_DEBUG "/DEBUG")
	endif()
        ...

ought to do it.

For a successful build this needs some of the numpy headers in the lib folder atleast for windows, pinging @Arto Kitula (akitula) to give this diff a whirl on mac ?

For a successful build this needs some of the numpy headers in the lib folder atleast for windows, pinging @Arto Kitula (akitula) to give this diff a whirl on mac ?

On (the cmake) dependency libraries build script we need to just add next line to NUMPY_INSTALL:

${CMAKE_COMMAND} -E copy_directory "${CMAKE_CURRENT_BINARY_DIR}/build/numpy/src/external_numpy/build/src.${PYTHON_ARCH2}-3.5/numpy/core" "${LIBDIR}/numpy-c"

and to harvest:

harvest(numpy-c numpy)

after that, audaspace can use LIBDIR/numpy (that then has include, src, lib directories)

For example this works. Directory naming (numpy-c) was quite freely put, we can decide other naming also =)

Some suggestions, from reading over the CMake files:

  • Rename (and flip) BLENDER_AUDASPACE to AUDASPACE_STANDALONE. Since any project using audaspace will probably want to use this, and means it can be written as less of a Blender spesific hack.
  • Rename BINDING to PYTHON, eg: WITH_BINDING_DOCS -> WITH_PYTHON_DOCS ... since binding is a general term that might apply to plugins or some other area. Even in the context of audaspace there are C bindings.

Noted some issues inline too.

extern/audaspace/CMakeLists.txt
310

Should use set(CMAKE_CXX_STANDARD 11) instead, then there is no need to be GCC spesific.

589

While not blender related, Python 3.2 is quite old, probably it should find the latest Python3?

After rBa8eaaa21e02b windows build might work already, but since there is problem with numpy cmake file (PYTHON_ARCH2 is undefined), macOS installation for the numpy is failing. @Brecht Van Lommel (brecht) might want to take look at that?

I've now committed the needed numpy changes for macOS, after that there is just one more issue to make it build.

extern/audaspace/CMakeLists.txt
726–733

These 3 install() commands need to be moved into if(NOT BLENDER_AUDASPACE).

Otherwise I get:

CMake Error at extern/audaspace/cmake_install.cmake:39 (file):
  file INSTALL cannot find "/Users/brecht/dev/blender/include".

I incorporated all the latest comments. As I understand it, Mac works now. So if everything is ok now, I'll push the patch as soon as I get the ok from @Ray Molenkamp (LazyDodo).

  • Rename (and flip) BLENDER_AUDASPACE to AUDASPACE_STANDALONE. Since any project using audaspace will probably want to use this, and means it can be written as less of a Blender spesific hack.

Done.

  • Rename BINDING to PYTHON, eg: WITH_BINDING_DOCS -> WITH_PYTHON_DOCS ... since binding is a general term that might apply to plugins or some other area. Even in the context of audaspace there are C bindings.

The bindings documentation is for both Python and C - they have the same API - so the name fits.

Should use set(CMAKE_CXX_STANDARD 11) instead, then there is no need to be GCC spesific.

Done, had to increase the minimum required cmake version to 3.1.

While not blender related, Python 3.2 is quite old, probably it should find the latest Python3?

That's a minimum required version as long as you don't pass EXACT as parameter.

I've now committed the needed numpy changes for macOS, after that there is just one more issue to make it build.

Thanks @Brecht Van Lommel (brecht)! I made sure that actually no install() command is called for audaspace being built as part of blender.

I incorporated all the latest comments. As I understand it, Mac works now. So if everything is ok now, I'll push the patch as soon as I get the ok from @Ray Molenkamp (LazyDodo).

I think i can temporary add the needed headers to the python include dir, that should sort things out short term, given i'm not entirely comfortable doing big lib changes while 2.79 is not quite out the door yet.

Since I got the OK from @Ray Molenkamp (LazyDodo) on IRC, I'm ready to push the commit. I will do so on Friday morning (CEST) so that I'll be around for a few hours afterwards and I can handle any problems people might have after the push. That is, if there is nothing else coming here.

The latest changes required for numpy paths to work on Linux and Mac! All build bots build this patch fine now, so I guess it's ready to push. Will do that tomorrow morning.

I'm a bit unsure about the cmake_minimum_required bump to 3.1. Blender itself only requires 2.8. I'd find it weird to have Blender require 2.8 and a module of it 3.1.
I also think most Linux distros only have something older than 3.1 in their default repositories (Debian Jessie repos only have 3.0.2, which is what I'm using here).

I think set(CMAKE_CXX_FLAGS "-std=c++11 ${CMAKE_CXX_FLAGS}") should be supported by all platforms with CMake 2.8, or maybe 2.8.12 (such a minor bump wouldn't be a big deal I guess).

In any case, I don't really see the need to go all the way to 3.1, neither for Blender nor for Audaspace.

@Julian Eisel (Severin), newer cmake is available in jessie-backports, also keep in mind Jessie is on it's way to EOL now, so you should consider dist-upgrading. Not saying i'm supporting such a rather silent and non-communicated bump of CMake tho.

What i'm not sure is: why do we need to specify std in audaspace? This is already done globally for the whole Blender [1]. WITH_CXX11 is default in 2.8 branch and it will fail to build if you disable this option. Eventually we'll need to clean this up and remove option all together, but that's another story.

[1] https://developer.blender.org/diffusion/B/browse/blender2.8/CMakeLists.txt;9a262ed47ecb1c9f43053b0653364c59d9595fdf$1468

Joerg Mueller (nexyon) abandoned this revision.EditedAug 19 2017, 11:54 AM

@Julian Eisel (Severin), newer cmake is available in jessie-backports, also keep in mind Jessie is on it's way to EOL now, so you should consider dist-upgrading. Not saying i'm supporting such a rather silent and non-communicated bump of CMake tho.

What i'm not sure is: why do we need to specify std in audaspace? This is already done globally for the whole Blender [1]. WITH_CXX11 is default in 2.8 branch and it will fail to build if you disable this option. Eventually we'll need to clean this up and remove option all together, but that's another story.

[1] https://developer.blender.org/diffusion/B/browse/blender2.8/CMakeLists.txt;9a262ed47ecb1c9f43053b0653364c59d9595fdf$1468

I just pushed a fix using WITH_CXX11. There I also lowered back to cmake 3.0 reverting the change that @Campbell Barton (campbellbarton) suggested. However 3.0 is the minimum cmake version that audaspace needs to build because of the string(CONCAT) command that I need, especially for building audaspace for blender. I talked to @Campbell Barton (campbellbarton) about this on IRC and he said, that we will likely bump the cmake version to 3.something for blender 2.8 anyway.

I also thought about adding

if(${CMAKE_MAJOR_VERSION} LESS 3)
	if(WITH_AUDASPACE AND NOT WITH_SYSTEM_AUDASPACE)
		message(FATAL_ERROR "WITH_AUDASPACE requires at least CMake 3.0")
	endif()
endif()

but that's pretty much what cmake_minimum_required(VERSION 3.0) does anyway.

Cheers