Refactoring surface extractor interface

Create issue
Issue #49 closed
David Williams created an issue

The MarchingCubesSurfaceExtractor is currently used something like this:

SurfaceMesh<PositionMaterialNormal> mesh;
CubicSurfaceExtractorWithNormals< SimpleVolume<uint8_t> > surfaceExtractor(&volData, volData.getEnclosingRegion(), &mesh);
surfaceExtractor.execute();

Note how you have to decalre a (fairly complex) mesh type first. I'd actually like to templatise the 'PositionMaterialNormal' on the material type as this is proving useful in Cubqiuity, but this would complicate the above sample even further.

Instead of being passed as a parameter, the mesh could be the return type of the execute() function. This should mean the user doesn't need to see the type but can instead handle it with the 'auto' keyword. Something like:

CubicSurfaceExtractorWithNormals< SimpleVolume<uint8_t> > surfaceExtractor(&volData, volData.getEnclosingRegion());
auto mesh = surfaceExtractor.execute();

In practice the returned mesh would probably be some kind of C++11 smart pointer to simplify resource handling.

There's also the question of whether this surface extractor should be 'unclassed' into a simple function, but I think that is orthogonal to the problem at hand.

Comments (9)

  1. David Williams reporter

    @milliams My main concern is what effect the above would have on the Python bindings. If we return the mesh by some kind of smart pointer then can Python handle this? Do we want smart pointers used in the interface to PolyVox or will they cause trouble?

  2. Matt Williams

    I agree this is a good idea. I did a similar thing in dualContouringSurfaceExtractor() since it simplifies things a lot.

    In that case I also wrote it as a simple function rather than a class but I agree that the two are somewhat orthogonal. However, it should be a simple task to un-class the surface extractor as almost all the logic in in the execute() method. Un-classing will obviously also simplify the interface as you won't need to explicitly give the template arguments any more.

    As far as the bindings go, it looks like this will require some changes. SWIG can be made to work with smart pointers but it might take some fiddling. Either way, I wouldn't let breaking the bindings be a barrier to implementing a better API which I think this is.

    The alternative is to simply return the mesh by value and let move-semantics and return-value-optimisation do its magic. I'm not sure at this stage which return types of the two would be the most idiomatic.

  3. David Williams reporter

    I'm not that familiar with move semantics (though I know you've mentioned them before), but after a quick bit of research it does look like they might be the right solution. I guess the move semantics would need to be defined for the SurfaceMesh class, which should be straight-forward. But for now I can use the same approach as your DC code, and the move semantics will then be a later optimization.

    I might unclass at the same time - I'll just see how it goes. Did you wrap your DC code with Python bindings at all?

  4. Matt Williams

    Yes, adding a move constructor and move assignment operator to SurfaceMesh (or perhaps defining no constructors or copy operators and letting the compiler do them all) should be sufficient. In the meantime it's probably not a big inefficiency and we can implement it later as an optimisation like you say.

    I haven't done anything for Python with the DC but wrapping a function call which returns by value should be quite easy. The only problem comes from having to be explicit with templates. Feel free to go ahead with all the changes and if you can't/don't get the Python bindings working I have time this weekend to fix them if necessary.

  5. David Williams reporter

    I've started moving forward on this, by just wrapping the existing extractor classes with functions (see extractor-refactor feature branch). I think this lets us get the benefits of unclassing without actually breaking existing code, including the Python bindings. We can break the existing code eventually (and we might have to as we proceed) but it's nice if we verify the new approach works as far as possible before breaking the old.

    That said, I'm quite aware that the OpenGLExample is quite a mess and is likely to be some work to refactor. I think it's going to impede moving to a more flexible vertex type. I'm tempted to ditch it, as it doesn't show good practices (OpenGL immediate mode and other things) and we have Cubiquity now as a test of more advanced functionality. I'm not sure yet, I'll see how it goes.

    Also, did you make use of the PositionMaterial or PositionNormalMaterial vertex types for your Dual Contouring stuff? If I templatise these on material type it might have a knock-on effect on you?

  6. Matt Williams

    As of now I have only one dual-contouring implementation which uses PositionMaterialNormal since it is only a first draft and I didn't get round to making it flexible yet. If you change the vertex type to be templated, it might require a tweak or two in my code but it wouldn't be anything significant.

    I've also added the branch to the Buildbot.

    As for OpenGLExample, you might be right. What does it add over what's shown in BasicExample? I based the Python example on BasicExample except I changed it to use VBOs etc. rather than immediate mode. Regardless of OpenGLExample, it might be worth updating BasicExample to use OpenGL 3 stuff (it's pretty much supported everywhere now).

  7. David Williams reporter

    I just went to check what the OpenGLExample is really doing, and actually it's fairly useful. It demonstrates breaking the volume down into multiple pieces, and also uses multiple materials rendered as different colours. In fact this was broken, and I can see that it was a result of a commit I made only a couple of weeks ago. So I guess it's still serving a useful regression testing purpose :-)

    Perhaps I'll try to leave it in, but it does need reworking. There's a flag you can set for whether it should use immediate mode or vertex buffers and this makes the code messy. As you say, all the examples should be updated to OpenGL3. I've made a separate issue for that just to keep things tidy. See #50

    Also, is there an issue related to your Buildbot work? I didn't see one, and wanted to ask about email notifications.

  8. Matt Williams

    OK, then I agree it makes sense to keep the OpenGLExample. I guess removing the immediate mode stuff and just using OGL3 and VBOs would simplify it a bit which is also important to make it useful as a code showcase. We can continue this in the other issue if necessary.

    As for Buildbot, I haven't made an issue about it since it is a more general Volumes of Fun issue than a PolyVox one but feel free to make an issue here or a post on the forum about it. Specifically about email notifications though, it's quite possible for me to add a MailNotifier so that it can email us on failures. I'll see if I can add that in in the next week or two.

  9. David Williams reporter

    The main issue here has been fixed - that is the extractors return a mesh directly which can then be used with auto:

    auto mesh = extractCubicMesh(&volData, volData.getEnclosingRegion());
    

    or:

    auto mesh = extractMarchingCubesMesh(&volData, volData.getEnclosingRegion());
    

    As also discussed, the extractors are now free functions rather than classes (mostly, there is still a bit of refactoring to do here. Also, the OpenGL Example has indeed been refactored.

    Other discussion points such as the (benefits of move constructors) are still valid but don't need a issue open for them.

  10. Log in to comment