Page MenuHome

Audaspace 1.0 patch for Blender (C++11)
Needs ReviewPublic

Authored by Joerg Mueller (nexyon) on Jun 16 2015, 4:31 PM.

Details

Summary

Audaspace C++11 support for blender.

Diff Detail

Event Timeline

Joerg Mueller (nexyon) retitled this revision from to Audaspace 1.0 patch for Blender (C++11).
Joerg Mueller (nexyon) updated this object.
Joerg Mueller (nexyon) set the repository for this revision to rB Blender.
Joerg Mueller (nexyon) changed the visibility from "Public (No Login Required)" to "BF Blender (Project)".
Joerg Mueller (nexyon) changed the edit policy from "All Users" to "BF Blender (Project)".
Campbell Barton (campbellbarton) changed the visibility from "BF Blender (Project)" to "All Users".Jun 18 2015, 12:28 AM
Campbell Barton (campbellbarton) changed the visibility from "All Users" to "Public (No Login Required)".
Campbell Barton (campbellbarton) requested changes to this revision.Jun 19 2015, 7:49 AM
Campbell Barton (campbellbarton) edited edge metadata.
Campbell Barton (campbellbarton) added inline comments.
extern/audaspace/CMakeLists.txt
30–32

don't use globbing, its really bad

See: http://stackoverflow.com/a/18538444/432509

60

Can check CMAKE_COMPILER_IS_GNUCC here, instead of "${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU"

66

just do if(MSVC) (same for other similar checks)

211

Any reason to have separate source and headers? we normally have all in SRC

source/blender/editors/sound/sound_ops.c
69–71

Don't think this is acceptable to have copy-pasted all over the place.

we can have defines like freetype does.

`#include AUD_C_API_H`

Then keep the includes simple like they are now.

source/blender/makesrna/intern/CMakeLists.txt
232–245

Better just define CAUDASPACE_INCLUDE_DIRS to point into extern/audaspace/bindings/C or intern/audaspace/intern.

Reduce a lot of duplicate checking.

This revision now requires changes to proceed.Jun 19 2015, 7:49 AM
Sergey Sharybin (sergey) requested changes to this revision.Jun 19 2015, 10:12 AM
Sergey Sharybin (sergey) edited edge metadata.

First of all, please add SCons support. Otherwise our release environment is screwed up.

Second of all, what about having single WITH_AUDASPACE option and then we just detect whether compiler does support all the C++11 features Audaspace requires and if it does we do sue new audaspace, otherwise we stick to the old one. Just an idea, don't start implementing right away, opinion from @Campbell Barton (campbellbarton) is good to have here :)

Joerg Mueller (nexyon) edited edge metadata.
Joerg Mueller (nexyon) removed rB Blender as the repository for this revision.

I have fixed all of @Campbell Barton (campbellbarton)'s suggestions, except: "Any reason to have separate source and headers? we normally have all in SRC"

I need it separetly for building the plugins. This part is stripped from the CMakeLists.txt in Blender, but I want to keep that one and the original Audaspace one as similar as possible, so that changes are easier to synchronize.

@Sergey Sharybin (sergey): The idea is first to see how C++11 audaspace is working, we don't want it in release builds yet. Therefore autoselecting which one to build is also not a good idea I think. Also @Campbell Barton (campbellbarton) said that once buildbots can build with cmake, scons will eventually get removed soon. It would be really helpful to get rid of having to maintain a second build system.

Hi, discussed this patch with @Sergey Sharybin (sergey) at some length and we agree on basic approach.

Firstly getting C++11 working on OSX is seems to have binary compatibility issues, According to @Martijn Berger (juicyfruit) this bumps requirement to a much newer OSX version... though I don't have exact details - it needs some investigation (building vs running... ABI compat... etc).

However this can be added in a similar way to how we already have for WITH_SYSTEM_BULLET.

So:

  • Plan to eventually migrate to audaspace as a system library (in /lib for OSX and Windows, system dep for Linux).
  • Keep old audaspace in /intern for now, and keep using it by default.
  • Platform maintainers can prepare for C++11 and enable this to see how it works.
  • At a point when all platforms support C++11 without too many problems (OSX ABI ... etc), then we make the switch and default to new audaspace, removing old audaspace from git.

To me this is just audaspace growing up. if it is ready to live outside of blender we should just facilitate that by adding it to lib and get rid of it in intern old version asap.
It either is a part of blender that or a separate library in which case it should really not be in intern.

Then the whole discussion also becomes less about bumping blender but only about the API being c89 / c++99 compatible.

I actually changed the C++ audaspace code in the game engine to use the C API as well, so that Blender itself can still be compiled without C++11. So the API is definitely C89 compatible. I wonder though if it's still possible without problems to link to the library if a newer OS X version is needed for it? The library itself of course needs the C++11 standard library.

As I told @Martijn Berger (juicyfruit) on IRC, the problem with just having it as system library is, that all Linux distributions then need to create a package of audaspace. I would be really happy about that, but to do so I need to make an official release first, which I need on official OS X build for, which I can't build not having a Mac.

The plan of @Campbell Barton (campbellbarton) and @Sergey Sharybin (sergey) sounds like what I had in mind and this patch actually does: it defaults to the old audaspace and enables building with the C++11 version or having it as system library (the latter only on Linux so far). Do you think it's ready to merge this patch then?

@Joerg Mueller (nexyon), the patch also adds new audaspace to extern/, which does not correlate with mine and cambo's suggestion. So in this regard patch is not ready for merge.

Then the last two points of @Campbell Barton (campbellbarton)'s list don't make sense, as the interface would be C89 only. Why does blender need to be C++11 then?

While interface might be C89 only you'll still need to compile library on all platforms,and while doing this quite a few of underwater stones will be unleashed by platform maintainers.

P.S. Question which i'm keeping forgetting to ask for quite some time now -- is there a _really_ need to use C++11 for the new audaspace? I.e. if you'll look into Depsgraph and Cycles they've got condition to either use C++11 or Boost. Bet similar thing could be implemented in audaspace?

Joerg Mueller (nexyon) edited edge metadata.

I removed the audaspace version in extern/ and made WITH_SYSTEM_AUDASPACE work on windows. On Mac this needs to be done by someone else, as I don't have access to a Mac.

Sergey Sharybin (sergey) requested changes to this revision.Jul 21 2015, 2:54 PM
Sergey Sharybin (sergey) edited edge metadata.

Am i right this change switches blender code to use new API? As in, once the old audspace is deprecated and removed we don't need to port blender to yet another new C-API calls.

If so generally i like the idea of way of moving forward, but you need to fix SCosn compilation before the merge.

Also wouldn't mind feedback from @Campbell Barton (campbellbarton).

This revision now requires changes to proceed.Jul 21 2015, 2:54 PM
Joerg Mueller (nexyon) updated this object.
Joerg Mueller (nexyon) edited edge metadata.

Tested on linux build environment, 64bit only for now. Compiled fine, didn't test sound yet tho.

build_files/scons/tools/Blender.py
397 ↗(On Diff #4727)

Dead code is not welcome.

source/blender/blenkernel/intern/sound.c
847

Don't think it's really correct. System audaspace might not be compiled with jack support.

Hi, patch seems quite close

Noticed memory leaks P243
(ddd9a4d2bdacdb2f3891ba59231790046f1dfacf from https://github.com/neXyon/audaspace)

build_files/cmake/Modules/FindAudaspace.cmake
15–19

Should support our convention of having AUDASPACE_ROOT_DIR, see JACK_ROOT_DIR, OPENEXR_ROOT_DIR... etc.

This allows passing in a directory that will be searched.

67–75

picky. prefer AUDASPACE_C_LIBRARIES, AUDASPACE_PY_LIBRARIES,

Matches close to CMake's CMAKE_C_FLAGS and CMAKE_CXX_FLAGS, and means they're grouped in cmake options.

Joerg Mueller (nexyon) updated this object.
Joerg Mueller (nexyon) edited edge metadata.

@Campbell Barton (campbellbarton): as I said in IRC, I would need information on how to reproduce the memory leak. So far I coudln't. I also added the AUDASPACE_ROOT_DIR, but I won't rename AUDASPACE_C_LIBRARIES as the library is called caudaspace and not audaspace-c or audaspace_c, same for the python version.

@Sergey Sharybin (sergey): Dead code is removed. Regarding the second part: the devices shown in the configuration would be wrong anyway depending on whether SDL/OpenAL were enabled during build time in blender as well. I fixed this now by introducing a new method in audaspace that retrieves all available devices (even OpenAL subdevices).

@Joerg Mueller (nexyon), the purpose of these names isn't to match the file exactly, its so we can have a good way to reference multiple libraries for a package, See CUDA.

Also for OpenGL, its not called GL_LIBRARY, GLU_LIBRARY, (for libGL and libGLU) its OPENGL_gl_LIBRARY & OPENGL_glu_LIBRARY.

Ok, I renamed the libraries. Don't foget to rebuild/reinstall audaspace and reconfigure when trying.

Campbell Barton (campbellbarton) edited edge metadata.
Campbell Barton (campbellbarton) added inline comments.
build_files/cmake/Modules/FindAudaspace.cmake
95

Picky suggestion, keep each arg on own line...

mark_as_advanced(
  AUDASPACE_LIBRARY
  AUDASPACE_LIBRARIES
  ....