bjornblissing / osgoculusviewer

An OsgViewer with support for the Oculus Rift
Other
106 stars 67 forks source link

Adding MSAA support. #67

Closed ChrisDenham closed 9 years ago

ChrisDenham commented 9 years ago

Adds support for MSAA which is enabled by passing a non zero samples parameter when creating OculusDevice

I think I have made the changes so that the original behaviour is the same as before when passing in samples = 0

bjornblissing commented 9 years ago

I will take a look at it on Monday, when I am back at work.

bjornblissing commented 9 years ago

Ok.. I have tried it. Some comments:

First of all I get the following error when compiling against osg 3.4.0: oculusdevice.cpp(164): error C2039: 'Extensions' : is not a member of 'osg::Texture'

This can be solved by including the following in oculusdevice.h:

#if(OSG_VERSION_GREATER_OR_EQUAL(3, 4, 0))
typedef osg::GLExtensions OSG_Texture_Extensions;
#else
typedef osg::Texture::Extensions OSG_Texture_Extensions;
#endif

And adding the following method in oculusdevice.cpp:

static const OSG_Texture_Extensions* getTextureExtensions(const osg::State& state)
{
#if(OSG_VERSION_GREATER_OR_EQUAL(3, 4, 0))
    return state.get<osg::GLExtensions>();
#else
    return osg::Texture::getExtensions(state.getContextID(), true);

#endif
}

Then the offending line (and the line above it) can be replaced with:

const OSG_Texture_Extensions* extensions = getTextureExtensions(state);

Then I have some minor issues about the general style...

I am not too happy with the hardcoded OpenGL commands. It will work for now, but in the future we should probably wrap the texture handling in its own subclass of osg::Texture improve the general style and drop the dependency on osg::Texture::TextureObject. This will probably help when adding support for asynchronous time warp as well.

Finally, I am not entirely sure that I like the function name SetupNormal(). IMHO, We should probably come up with something better. I mean using MSAA is not abnormal. So it should rather be something like: setup and setupMSAA. Also note that the functions in the rest of the codebase are using camelCase with the first letter in lower case. Only classes are using upper case on first letter.

So if you can update the pull request with these changes I am prepared to merge it.

ChrisDenham commented 9 years ago

Ok, thanks. I'll make the changes as per your suggestions, and I'll make sure it actually builds with OSG 3.4.0 this time (I was testing against OSG 3.2.1). I wasn't too happy about adding the raw OpenGL stuff either (though the precedent for this seemed to be already set). I did try quite hard to reduce the OpenGL calls and use the OSG encapsulation instead, but I kept hitting intractable problems and had to quit out.

ChrisDenham commented 9 years ago

@bjornblissing Changes done. Thanks.