This is not really a bugpost, but it feels like a bug to me

The surface Extractors take a pointer to a volume.
I'm assuming that the Extractors will only call const functions of the volume object (since the extractor should not change the content of the object, I'm aware of the internal mutables)

also pointers are so '99 and my c++faqs tell me I should use references where I can, pointers where i must.

Does anything speak against making that volume pointer argument
1. const
2. a reference

I think it should be const, but actually I just gave it a quick test and it doesn't compile. The problem is that the Volume::Sampler also needs to be fixed for const correctness (and then there may be further changes). This should be done, but means it's no longer a 5 minute job. I'll leave this bug open until this gets sorted.


Not sure about this one... but it probably should be changed. I'll need to think about it some more. One point of concern is that we now have the start of some image processing classes (VolumeResampler, LowPassFilter, etc). More will probably be added, and it is possible that it will be nice to chain them together and then execute the whole chain at once. Toolkits such as ITK use this approach. I don't know yet if it makes sense for PolyVox, but if it does then using pointers may make it easier to build such chains. We'll have to see how that develops.

Generally the pattern I try to follow is that if a function might make a change to an argument then it should be passed in as a pointer and if it will not change it then it should passed as a const reference. Some people recommend the use of references for values which are changed by the function but I feel that using pointers makes it clearer to the user of the API when done our way. This is covered in the Qt style guide (as well as a little in the KDE API policy).

In this particular case, the surface extractor should not be making any change to the volumes and so ideally it should be passed as a const reference but as David says, currently the Sampler is not const-safe so it can't be changed yet. A possible solution would be to have both a Sampler and a ConstSampler (like there are iterators and const_iterators in the STL). Also the considerations of the processing pipeline API must be considered as David says.

I'm keen to have this sort of thing sorted out as I feel const correctness really helps make APIs clearer and easier to use. I'd like to take a few other cues from Qt in future as well such as avoidance of boolean parameters. Personally I consider inconsistent/unclear API as a bug so please don't hesitate to bring things like this up.

_________________
Matt Williams
Linux/CMake guy

Yes, except I think the term 'Sampler' already implies it is read only (and they are read only, they are just missing some const keywords). But I have been experimented with allowing them to write as well (e.g. for copying data between volumes) and in this case it starts to make more sense to call the Iterators than Samplers. Maybe we then need ConstIterators or maybe we don't... I'm not sure yet.



Yes, and I should also point out that these kind of improvements are under active consideration at the moment, it's just that actual commits are slow due to work on Voxeliens. But changes will be coming.