New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Oculus Example #39

Merged
merged 3 commits into from Apr 28, 2018

Conversation

Projects
None yet
2 participants
@Squareys
Contributor

Squareys commented Apr 16, 2018

Hi @mosra!

As promised, here is the pullrequest that simplifies the Oculus integration example. It still looks exactly the same. I removed all use of scenegraph and heavily cleaned up the rest of the code.

Looking forward to the review!
Cheers, Jonathan.

@mosra

Wow, 300 lines removed. Great job! 👍

Some minor comments... :)

{1.0f, 1.0f, 0.0f},
{1.0f, 0.0f, 0.0f},
{0.0f, 0.0f, 1.0f},
{0.0f, 1.0f, 1.0f}};

This comment has been minimized.

@mosra

mosra Apr 25, 2018

Owner
enum: std::size_t { CubeCount = 4 };
Color3 _cubeColors[CubeCount] = {0xffff00_rgbf, 0xff0000_rgbf, 0x000000_rgbf, 0x00ffff_rgbf};

(... I'm trying to push for the color literals as much as possible so people know it exists ;))

_eyes[1].setParent(&_cameraObject);
/* Setup depth attachment */
TextureFormat format = TextureFormat::DepthComponent24;
if(Magnum::Context::current().isExtensionSupported<Extensions::GL::ARB::depth_buffer_float>())

This comment has been minimized.

@mosra

mosra Apr 25, 2018

Owner

Does just Context::current() work here? Also, the extension is part of GL 3.0, I seriously doubt someone would try VR on a card that does GL 2.1 at most ... so maybe just MAGNUM_ASSERT_EXTENSION_SUPPORTED() or ... screw it, not even that ;)

_cameraObject.rotateY(Deg(0.1f));
}
/* Provide some rotation, but only without real devices to avoid VR sickness ;) */
if(_session->isDebugHmd()) _cameraRotation += Deg(0.1f);

This comment has been minimized.

@mosra

mosra Apr 25, 2018

Owner

0.1_degf?

Matrix4::rotationY(Deg(45.0f))*Matrix4::translation({0.0f, 0.0f, -3.0f}),
Matrix4::rotationY(Deg(45.0f))*Matrix4::translation({5.0f, 0.0f, 0.0f}),
Matrix4::rotationY(Deg(45.0f))*Matrix4::translation({-10.0f, 0.0f, 0.0f}),
Matrix4::rotationY(Deg(45.0f))*Matrix4::translation({0.0f, 0.0f, 7.0f})};

This comment has been minimized.

@mosra

mosra Apr 25, 2018

Owner

45.0_degf? :)

#include <Corrade/Containers/Array.h>
#include <Corrade/Utility/utilities.h>

This comment has been minimized.

@mosra

mosra Apr 25, 2018

Owner

Those empty lines are unnecessary, I think. The different prefixes already separate it enough.

@@ -96,16 +97,28 @@ endif()
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(OVR DEFAULT_MSG
OVR_LIBRARY
OVR_LIBRARY_DEBUG
OVR_LIBRARY_RELEASE

This comment has been minimized.

@mosra

mosra Apr 25, 2018

Owner

🤔 doesn't this fail when you have an OVR SDK that has only the Release version present? Would SelectLibraryConfigurations help here?

(Yes, I know, this is just a copy of the file from the integration repo, but since that other PR is still stuck on me I thought I would comment here.)

This comment has been minimized.

@Squareys

Squareys Apr 27, 2018

Contributor

Thanks, works!

@mosra mosra self-assigned this Apr 25, 2018

@mosra mosra added this to the 2018.0b milestone Apr 25, 2018

@Squareys Squareys force-pushed the Squareys:simplify-ovr-example branch from 4c87e54 to f6e6091 Apr 27, 2018

@Squareys

This comment has been minimized.

Contributor

Squareys commented Apr 27, 2018

@mosra Thanks for the review! I made all the requested changes, this is once again ready for review/merge 👍

@Squareys Squareys force-pushed the Squareys:simplify-ovr-example branch from f6e6091 to a89efaa Apr 28, 2018

Squareys added some commits Apr 12, 2018

ovr: Condense example to a single file
 * Remove SceneGraph usage
 * Use NoCreate instead of unique_ptr where possible

Signed-off-by: Squareys <squareys@googlemail.com>
modules: Update FindOVR.cmake
Signed-off-by: Squareys <squareys@googlemail.com>
doc: Remove deleted files from ovr.dox
Signed-off-by: Squareys <squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:simplify-ovr-example branch from a89efaa to 153cccc Apr 28, 2018

@mosra

This comment has been minimized.

Owner

mosra commented Apr 28, 2018

Great, ready to merge this 👍

Is it okay if I put the updated FindOVR.cmake to the integration repo as well, while keeping the rest of mosra/magnum-integration#26 unmerged? I assume it won't cause any problems.

@Squareys

This comment has been minimized.

Contributor

Squareys commented Apr 28, 2018

Sure, please do!

@mosra mosra merged commit 153cccc into mosra:master Apr 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mosra

This comment has been minimized.

Owner

mosra commented Apr 28, 2018

💥 merged! :) Thanks a lot.

@Squareys

This comment has been minimized.

Contributor

Squareys commented Apr 29, 2018

Awesome, thanks! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment