LibOVRIntegration

[Squareys]

Hi @mosra, Hi everybody,

Since this definitely will be a thing now, let's move discussion of LibOVRIntegration to here. Previous discussion can be found in https://github.com/mosra/magnum-examples/pull/10.

If you are okay with it, only the most important compositing layer (ovrLayerEyeFov) will be wrapped for now and I will create an issue which lists the missing wrappers and implement them when needed.

Current TODOs:

• [x] Finish wrapping compositor.
• [x] Cleanup code.
  • [x] Cleanup code formatting.
  • [x] Change // comments to /* comments */.
  • [x] Change MagnumLibOvrIntegration_EXPORTS to be all uppercase.
  • [x] Remove #endif description comments.
  • [x] Add copyrights properly.
  • [x] Fix posef test.
  • [x] Use #include<memory>.
  • [x] Add enumset operators.
  • [x] Fix ovrSwapTextureSet* getOvrSwapTextureSet() being a raw pointer.
  • [x] Change case of MAGNUM_LibOvrIntegration_EXPORT.
  • [x] Refactor context.
  • [x] Use absolute includes.
• [x] Rebase to cleanup commit history.
  • [x] Squash fixups.
• [x] Finish doc++.
• [x] Finish FindOVR.cmake.

[mosra]

Thanks a lot for this effort. Great stuff, as I said already before.

Just a few things before you proceed further so you don't have to fix too much stuff later:

• To match naming policy of the rest of the project, I'd like to have LibOvrIntegration instead of LibOVRIntegration (you already did use Hmd instead of HMD which is great). But keep the FindOVR.cmake as is so it matches name of the DLL. Changing filename and directory case is rather tricky with Git on Windows, I'm able to help with that if you need.
• There's no need to have void in parameter-less function signatures (like Layer::layerType(void)). IIRC, this is some leftover from C, where the distinction actually had some meaning (but it doesn't in C++ and shorter is better).
• The inline keyword is implicit and thus redundant for functions defined inside class body -- little known feature and I don't blame you for not knowing it (but again, shorter is better).
• Please use either (const) references or std::unique_ptr instead of raw pointers and manual deletion, particularly when returning heap-allocated instances from functions to avoid accidental memory/resource leaks. The array of Texture2D pointers in SwapTextureSet is a tricky one, I have something in mind, but that needs to have some library support from my side.

That's for now, hopefully I didn't overwhelm you with the requests (sorry) :)

[Squareys]

To match naming policy [...]

I agree, I will just do a soft reset instead of a rebase then an recommit everything with the renamed directories, shouldn't be a problem.

The inline keyword is implicit and thus redundant for functions defined inside class body

Good to know, thank you.

Please use either (const) references or std::unique_ptr instead of raw pointers

In the cases where order of deletion matters, is it "clean" to use unique_ptr.reset() to delete?

That's for now, hopefully I didn't overwhelm you with the requests (sorry) :)

No problem, I learn alot from this kind of feedback :)

One thing regarding includes: I stated having problems with QTCreator showing me installed includes of LibOvrIntegration instead of the local ones so I am using relative imports now. Any idea what I could do about this?

[mosra]

I agree, I will just do a soft reset instead of a rebase then an recommit everything with the renamed directories, shouldn't be a problem.

The workaround that did the job for me was to rename in two steps (two commits that would be squashed later), first renaming the files/directories to completely different names and then back with proper case. Manual recommiting is not needed.

In the cases where order of deletion matters, is it "clean" to use unique_ptr.reset() to delete?

Can you point me to the code where this is the case? Could reordering the members help? (they are destructed in reverse order of declaration, last declared one is destructed first)

[Squareys]

Can you point me to the code where this is the case?

https://github.com/Squareys/magnum-examples/blob/ovr-example/src/ovr/OVRExample.cpp#L190-L193 Was the code snippet I had in mind, can be solved through reordering, though, as you pointed out.

[mosra]

Added the infrastructure for converting quaternions (mosra/magnum@94c4225165f72b970a67311cbcc40720c6595a27), dual quaternions (mosra/magnum@52434fb55c81df78ea3c57eb3a829cbc8cdc7a9d) and ranges (mosra/magnum@562d71dbe677c3dbd6427982895a62ce67dc9758) so you can finish the conversion utilities. The usage should be similar to what you have already done (see the test cases for hints). Funnily enough the new code revealed a GCC bug, so it took me much longer than I would like.

If I understand correctly, the ovrPosef class is just an orientation + position, so using dual quaternion instead of a matrix might be better idea in terms of memory efficiency and performance (and also way simpler conversion).

[Squareys]

If I understand correctly, the ovrPosef class is just an orientation + position, so using dual quaternion instead of a matrix might be better idea [...]

Yes, that's right. I never used dual quaternions before, but I'll figure it out.

Added the infrastructure for [...]

Thanks! That'll make things alot cleaner.

I need to concentrate on my studies for a bit, so sorry in advance that there will be a little pause in progress, I hope to get back to this next weekend, though, maybe I'll even find some time in between aswell.

[mosra]

Sure, you did amazing amount of work already :)

You don't need to understand the internal structure of dual quaternions to use them, they can be multiplied just like matrices and created from quaternion/translation pair like this:

Quaternion rotation;
Vector3 translation
DualQuaternion a = DualQuaternion::translation(translation)*DualQuaternion{rotation};

Conversion to matrices is then done via the obvious toMatrix(). Also, for the example, scene graph can (but doesn't need to be) be templated on dual quaternion type instead of matrices, just use SceneGraph::DualQuaternionTransformation instead of SceneGraph::MatrixTransformation3D.

[Squareys]

Alright, I think I the code should now pretty much respect your requests. The test for converting the DualQuaternions still fails, though. (The Texture2D** _textures; is still there, also.)

What is your opinion on the includes? I had had problems with QtCreator opening the installed headers instead of the local ones, so includes of LibOvrIntegration headers are currently all relative.

Also, I should probably add some more tests for the other code aswell. I saw there is a AbstractGLTester or similiar in Magnum. That might come in handy.

[mosra]

Okay, sorry again about annoying coding style comments :)

I'll look at the includes, it works for me without a problem in other projects, so maybe there's something wrong just in this repo. How's the test failing? (Maybe I was wrong about how the dual quaternion should be initialized.)

The AbstractOpenGLTester currently doesn't work on Windows (winapi requires me to pass some data through the constructor and I need to work around that somehow), will try to fix that soon.

I have one last note about the LibOvrContext class. It apparently needs to be some sort of a singleton, but I think it could be done without the initialize()/finalize() methods. Looking at the example, it's now like this:

`` cpp // LibOvrContext is created on heap statically before entering main() { LibOvrContext::get()->initialize();

// code that accesses the context through LibOvrContext::get() ... 

LibOvrContext::get()->finalize();

}

I propose changing that to the following:

``` cpp
{
    LibOvrContext _context; // as a private member of the application class

    // (unchanged) code that accesses the context through LibOvrContext::get() ... 

    // _context gets automatically destroyed when application is destructed
}

The static _instance variable will be initialized to nullptr, the constructor then will point it to itself (and assert that there was no other instance before), destructor sets it back to nullptr (and assert that it was equal to this) and get() method would return a reference instead of pointer (and assert that static instance is not nullptr). Should be a bit easier for the end user.

[mosra]

Just checked the absolute includes and they appear to work in QtCreator on Windows (e.g. ctrl-clicking on #include "Magnum/LibOvrIntegration/Conversion.h" inside Conversion.cpp file was working), if you install them to the same location as the rest of Magnum then they should work also when installed.

Huh... it looks like the first two commits still have the LibOVRIntegration directory name, case insensitive filesystems are painful :)

[Squareys]

Okay, sorry again about annoying coding style comments :)

I think I got used to it already ;)

How's the test failing?

.translation() on the instance does not give me the values I set in the first place with DualQuaternion::translation.

I have one last note about the LibOvrContext class. [...]

I had remembered that ovr_Initialize() needed to be called before any OpenGL call. That was removed in the current sdk version, though. And in this case your suggestion is alot cleaner, of course.

In your suggestion though, even accessing _context directly from Application would work, right?

Huh... it looks like the first two commits still have the LibOVRIntegration directory name, case insensitive filesystems are painful :)

Oh, well. I am on Linux currently, I might just cleanup the directories here later then.

Just checked the absolute includes [...]

Ctrl-clicking did work, yes, but it kept sending me to the installed files, causing a bit of confusion when I edited them and the local files did not change. I will try changing back the includes to be absolute and see if that problem persists.

[Squareys]

Folder renaming complete, I guess the squashing should happen under linux aswell.

[mosra]

.translation() on the instance does not give me the values I set in the first place with DualQuaternion::translation.

I think this is related to the fact that the rotation quaternion you are using in the test case is not normalized. Try this instead:

DualQuaternion{Quaternion{{0.1, 0.2, 0.3}, 1.0}.normalized()}

I need to look into the math again, apparently I completely forgot everything about dual quaternions :) I should also put some assertion somewhere to catch this case.

I had remembered that ovr_Initialize() needed to be called before any OpenGL call.

Even before creating GL context? Huh... Also, doesn't the SDK mess somehow with GL state so it needs to be accounted for on our side using Context::resetState()? Or are they reverting back all the state that they touch (slow...)?

In your suggestion though, even accessing _context directly from Application would work, right?

Yup. Best case of course would be if there is no need for the global instance.

it kept sending me to the installed files

Well, yeah :/ I have this exact problem with KDevelop, somehow system-wide includes are more important than the local ones. I'll try to investigate, maybe it is somehow related to order of include directories in CMake.

[Squareys]

I think this is related to the fact that the rotation quaternion you are using in the test case is not normalized.

Yeah, pretty sure that's it. As far as I know, non-normalized quaternions cannot be used for rotations.

Even before creating GL context?

You might have missed the next sentence in which I explained that this not the case in the current SDK version anymore and won't be so in the future.

Yup. Best case of course would be if there is no need for the global instance.

Well, it doesn't really make sense to have more than one context, also that would probably mess up the ovr_Initialize() and ovr_Shutdown() calls. So, I would keep it as a singleton.

[Squareys]

Some comments got lost, since they where attatched to diffs:

@mosra wrote:

Please use /* .. */ comments rather than // ... (except in Doxygen blocks, because these comments cannot be nested).

(Explanation: I have such policy that I'm using // ... exclusively for quick-and-dirty coding (commenting-out some part of code, short-lived todo/fixme comments etc.) so when I see it in a commit, it subconsciously triggers a warning that there is something that shouldn't be here.)

I think, this probably needs an update: http://mosra.cz/blog/corrade-doc/corrade-coding-style.html#corrade-coding-style-documentation

@mosra wrote:

Well, thinking about these, [the ovr structure comparison operators] I think it might be better to drop the operators and do the comparisons directly in the test cases, as it would do proper fuzzy comparison for floats, will verbosely print what's wrong when the test fails and won't break if next release of Oculus SDK adds the comparison operators itself. So in the test case below:

ovrQuatf c{a};
CORRADE_COMPARE(c.x, b.x);
CORRADE_COMPARE(c.y, b.y);
CORRADE_COMPARE(c.z, b.z);
CORRADE_COMPARE(c.w, b.w);

You can call the CORRADE_COMPARE macro in a loop, so that would work fine also in the matrix case.

Alright.

[mosra]

I think, this probably needs an update: http://mosra.cz/blog/corrade-doc/corrade-coding-style.html#corrade-coding-style-documentation

I know, yeah, it's confusing a lot, really, and it's bugging me all the time, but I was unable to persuade Doxygen to accept anything else -- writing /** /* */ */ completely breaks syntax highlighting in any editor (even though Doxygen somehow accepts that). That would be somehow bearable, though (abandon all editor sanity and autocompletion benefits), but writing /** */ inside Doxygen @code block is even more impossible because Doxygen then tries to parse it as an actual documentation block and that part then magically gets lost from the docs completely :D

I'll try to do something about it, though.

[Squareys]

I'll try to do something about it, though.

You could use /// /* */ as an exception for documenting documentation.

[Squareys]

So, should int s; //< short line. be int s; /*< short line. */ ?

[mosra]

Yup. I'm on it :)

[Squareys]

I'm on the finish stretch here.

I will probably finish the magnum-ovr example before this can be merged. I will take the working examples for the documentation from there, when that is done.

[Squareys]

Last TODOs left: Finish doc++ and then squash commits. Will finish https://github.com/mosra/magnum-examples/pull/10 first, though.

[mosra]

Added support for GL tests on Windows in mosra/magnum@88f04152905fb4dc3aefe2ed7fa81681e24f15e1, in case you would want to do them. And now off for the weekend :)

[Squareys]

Big squash incoming! I will add the doc in separate commits instead of fixups, if you are okay with that.

[mosra]

Sure, adding documentation to existing commits is no fun :)

Code looks great, I never imagined it would get so large. One last thing, could you rename the Conversion{.h,cpp} back to Integration.{h,cpp}? I know the name is not exactly clear, but let's stay consistent with Bullet integration lib.

[Squareys]

@mosra Alright, will do.

I'm currently wrapping another compositor layer for debugging (LayerDirect), and it turns out, that all of the methods needed are already implemented in LayerEyeFov. I could let LayerEyeFov inherit from LayerDirect, but then the method chaining would get messed up (calls to setViewport() for example would return a reference to LayerDirect instead of LayerEyeFov when called from an instance of the later.) What would you recommend?

• Is it okay to have a LayerEyeFov method return a reference to LayerDirect "for method chaining", or
• is that so bad that you would recommend code duplication (or using macros to define those methods 2-5 times in different subclasses of Layer)
• Or templates? Could it work defining the six method templates and then creating the required instances of those templates in the Layer subclasses?
• Or maybe just remove method chaining for Layer?

The inheritance doesn't really serve any more purpose than not having to rewrite the methods.

[Squareys]

While I was at it, I implemented all the layer types after all. I went with the code duplication solution, since inheritance is not actually desirable with the layers and there weren't that many shared methods overall.

And back to the example.

[Squareys]

@mosra I declare this ready for review aswell :)

You may want to cherry-pick the bullet doc fix (I could also create a new pull request for that, but it seemed a little overkill for one line change), to keep things clean. I can then rebase this onto master make it automatically mergable again.

[mosra]

No need to be so strict about pull requests, I'll merge (or fast-forward) everything in one go.

[Squareys]

@mosra Okay, ready for review/merge once again.