This is interesting, and could potentially replace the compiler checks we currently have in Typedef.h and also ease the introduction of C++11 features. I don't mind including the relevant module in the PolyVox repo as long as it's not too big but of course it would be better to have native support in CMake. Even if they don't adopt this system I can imagine they would want something similar.

However, we should be careful about making PolyVox in some way dependant on CMake as not everybody uses it. For example, for the gameplay integration I've just copied the PolyVox source code into the appropriate folder and added the files directly to Visual Studio and to Ant (the build system use by Android). We should avoid a situation where only uses of CMake can access certain PolyVox functions, or where any prebuilt binaries we eventually provide can only be used by CMake users. I'm not exactly sure of the implications or if there's even a problem but just keep it in mind.

It may be possible to introduce a file 'Config.h' with our own #defines. These could be set by CMake if it's being used or could otherwise default to sensible values which the user can change.



I had a quick look at move constructors and I think they could be useful. Actually I think they will be most useful for the small classes (Vector, Region, etc) because these are frequently passed around as parameters, copied, assigned to, returned, etc. Although you would expect it to benefit something like the Volumes I think it might not help beause we always pass them by pointer anyway (and avoid operations which would create a temporaries).

Ok. If I integrate this into PolyVox I'll be sure to take this into account. It should be able to work as follows:

We have this feature detection in the CMake build system only. During the build process, it creates a config.h header file (or adds settings to TypeDef.h) with the appropriate values. When you run 'make install' it will copy the binaries and header files to a specific location (C:\PolyVox\{bin,include} or something). It is the contents of this install directory which we will distribute as the SDK and which people will use to build against.

On Windows we will have created this SDK with a specific compiler (e.g. VC++ 10.0) and so the SDK will have all the features available that that compiler supports. Therefore, anyone using PolyVox in their VC++ 10.0 project will be compatible.

Then, there is the case of if the user only have VC++ 8.0. In this case we have two options: either we make available a VC++ 8.0 SDK (this might have to be the option due to binary incompatibility) or they simply manually edit the config.h to disable any features their compiler doesn't understand.

In your case with gameplay3d (and anyone else doing the same) you can follow a similar workflow. Set the PolyVox install directory to be something sensible (either c:\PolyVox or even into a subdir of your project like C:\Mygameplayproject\polyvox), run 'make install' for PolyVox, point the solution file and Ant file to the right location (headers and (static) libraries) and it will just work.

This will work in an almost identical way on Linux and Mac and allows people to use wither the SDK or self-built binaries without having to change things around. We put some effort in recently to make the installed layout of PolyVox sensible so we should make use of that.

Does this make sense? Is there any thing I'm missing?


One of the big things people talk about with C++11 is that it should be possible to almost always just pass things as values. The reason this is avoided traditionally is because of all the unnecessary copies this entails but with move semantics and perfect forwarding this is obviated. I agree that passing round pointer to volumes works perfectly fine but it might be worth considering allowing people to be able to pass them by value without having to worry about them being copied (which would obviously be a very heavy operation). Either way, I'll look first into implementing them for the simpler classes.

It's worth noting that compilers are supposed to be able to generate implicit move constructors (like they do for copy constructors, destructors and copy assignment operators) under certain conditions. The conditions are listed at cppreference.com and are quite hard to understand . The main thing to take away from that is that what used to be the rule of three (if you declare any one of copy constructor, destructor or copy operator= you should think about all three) is now the rule of five (same as before but also including move constructor and move operator=). See also this post. However, I think that only very recent versions of GCC (and perhaps no versions of VC++) are actually generating these methods.

_________________
Matt Williams
Linux/CMake guy

It mostly sounds fine, but I still think we need to take care with this part:



For various reasons it's often much easier to just drop the the PolyVox files directly into a project rather than linking against the library. My gameplay version of PolyVox has numerous hacks and fixes applied to make it compile for Android (as well as some experimental features), and it would be a pain to have to edit, build and install a seperate project just to play around with these things. It would probably also interfere with the stable version of PolyVox which is installed on my system, and would make it more difficult to be using different versions for different projects (or a version for Android and a version for Windows?).

I'm also not clear on the details of building static libraries for the various mobile platforms. Android is all done through Cygwin and Ant which may or may not be compatable with the output from CMake. Blackberry and iPhone are completely unknown to me at this point.

Basically the setup you describe is fine for most people and is also what we should eventually aim for with the gameplay integration. But for people doing tests, hacks, or other experimental work there can be a lot of flexibility in just dropping the .cpp/.h into their build environment of choice. As long as this still works (even if it requires some manual tweaking of a header) there should be no problem.



In some ways I quite like this idea, but choosing to pass by reference/value has a sematic meaning to the user as well as being a performance optimisation. It makes it clear whether modifications to a copy will also affect the original object. But that's just my initial thought, and people have thought this through more carefully than I have. Plus we're talking about making use of them in user code rather than inside PolyVox, so in that case the user should understand the behaviour of their own application anyway. Basically I think it's fine to try adding these to some classes to see what effect it has.

Ok. There were a number of ways I could have set this up but I think this solves it. In the source code repo we'll have a number of #ifdefs like CXX_INITALIZER_LISTS etc for each of the features we care about. We'll wrap these around any optional features we have so that if those aren't defined when compiling, the feature will be omitted.

If building with CMake, the script will simply (once it's checked which features are available) create a config.h file which will #define those features which are available. This config.h file will be installed and made available as park of the SDK so that the average end user can simply include headers (which will implicitly include config.h) and link against libraries with all the appropriate features enabled (based on the version of VC++ we built the SDK with).

If you just want to include the raw source code in a subdir of your own project and not have to use CMake at all this will just work out the box. If you want to enable any of the C++11 features in your copy of PolyVox, you'll just have to set the appropriate #defines in your solution file or Ant script (or make your own config.h).

I think this covers things but I'll think on it a little more before implementing anything


Yes, I like the semantic meaning in the API where we have const& for most things and pointers for anything which we might change. It's what they do in Qt and it's always made sense. I agree that on the whole we should keep this in place for the PolyVox API. The things that could potentially change is that anywhere where we currently return a pointer to a new'd object (I don't think we have any of these) we could return a value instead and that our users would not have to worry about copying around large objects (like Volumes).

Also, it would enable the use of more heavyweight user-defined voxel types (if the user declares a move constructor for it) since at the moment, their values are copied around a lot (fine for ints but not great for bigger things) without having to resort to storing pointers.

Overall, I think we should keep our current API style (const& and pointers) but add move constructors to enable more efficiency both internally and for our users. I'll try to benchmark it to get a handle on the performance difference.

_________________
Matt Williams
Linux/CMake guy

Yep, this sounds reasonable. We'd essentially be saving in config.h the defines that CMake sets (possibly renamed), to make sure that these same defines can be used whe people build against the library using a different build system. And if config.h is empty then things still work as expected.


We have it a bit in the serialization code (loadVolume() creates a new volume) but I don't like it because it's not clear to the user if they need to call 'delete' on a volume when it wasn't them who called 'new' on it. But cleaning up the volume serialization will wait until later.


I agree, I do think that move constructors should be added. I don't know how much performance difference it will make but it can't hurt to try.

Exactly. If the config.h is empty then all C++11 features will be disabled (unless we want a few universally supported ones to be enabled by default) but everything will still work just fine. When we get to this stage, I'll try to remember to add a CDash build with all C++11 features disabled to make sure that it doesn't break.

I'm planning on putting together some benchmarks (essentially running the heavy tests repeatedly) to see how various things affect performance. Once I've got a few numbers (and maybe even graphs!) I'll put a blog post together about it.

_________________
Matt Williams
Linux/CMake guy

I've been investigating adapting Region to support move semantics. However, a Region is pretty much just two Vectors and so I first need to make Vector support moves. The first obstacle to this is that the Vector internal data structure is a C-style array. I don't know that it is possible to hand over control of an array to another instance. I figured the first step must be to adapt Vector to use a "Type* m_tElements" as its internal data rather than a "Type m_tElements[Size]". Making this change and adding the appropriate new[] commands to constructor and the delete[] to the destructor works just fine. However, this causes a massive performance hit. For example the Surface Extractor test takes about 3 times longer and the AO Calculator Test increases by about 50%. The exact patch I work with is attached to this post.
If I then implement move constructors on Region and Vector, the performance increases by around 5-10% but nowhere near to where it was in the first place.

Is this an expected slowdown using pointers? The only difference is the size of the Vector data structure and the addition of the new[]s but I wouldn't have expected such a hit. I'm compiling at full optimization (-O3).

_________________
Matt Williams
Linux/CMake guy

Yeah, I'm not that suprised. There are basically three reasons why this can cause a problem:

• The cost of calling 'new' for each Vector you construct. I'm not sure how slow this is but it's slower than allocating on the stack.
  
• There is some extra indirection to access the data. You have to follow a pointer to find the data rather than accessing it directly.
  
• Perhaps most importantly, consider an array of 50 Vectors. In the current system the data for all of those is consecutive in memory meaning that the cache can be used effectively. But if you hold pointers to the data then the pointers will be consecutive, but the locations they point to will be spread out all over memory. This is bad for the cache.

So, it seems that move constructors are most effective when classes have pointers to data. Maybe it doesn't really fit the small classes in PolyVox then, but perhaps it can still help the larger classes (Volumes, etc). Though as I mentioned this would only be in user code and you might find you have to construct a rather artificial scenario to see the benefits here.

I don't really see where you'd ever need a copy AND a move constructor.
Having both only makes your code unreadable or confuses people when writing their own code.
Either the object holds a unique resource, then you don't want it to be copied (and want the compiler to stop copying at compile time), or it holds an information like a Vector3d that you can easily have multiple instances of AND want multiple instances of.

Volumes could have move constructors, but no copy constructors.
The same could be true for any result objects (raycast result, extraction mesh, pathfinder result).
As you usually don't want multiple copies of those, but rather pass the whole result.

The real reason that you'd want to define both is that by default, you'd define neither and just let the compiler create them for you. However, as soon as you're managing some more complex resources, you're going to want to write your own copy constructor to make the copy work properly (i.e. don't just copy the value of the pointer but make a copy of the pointee). One you have a custom copy constructor, the compiler will no longer create an implicit move constructor and so you'll have to define that one too. This is the general reason one would want to define both.

However, as you say, there's times when it might make sense to disallow copy constructors. In PolyVox, we could define just a move constructor for Volume and not a copy constructor to prevent people from copying it by accident (since it would be slow for them) but for anywhere where a copy makes sense, a move would also make sense. Even then, I'd argue that a Volume isn't a unique resource and making a copy (clone) of a Volume could well be a valid use case.

In general I agree that most non-native types probably don't need to be (implicitly) copied and really the user will just want (conceptually) to move it -- usually when push_back()ing something into a vector, you don't use that variable name again (since conceptually 'it' is now in the vector). However, I'm still unsure about forcing this style (push_back(std::move(result))) on people since it's not yet a standard way of doing things and the compiler (as far as I know) offers no warning if you access a variable after it's been move()d away.

Being a new area of C++, the accepted style isn't yet established and it's interesting to be a part of it. I'm happy to experiment with things but we need to be careful about what we impose as API in stable releases.

_________________
Matt Williams
Linux/CMake guy