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

WebVR example #33

Closed
wants to merge 6 commits into
base: master
from

Conversation

2 participants
@Squareys
Contributor

Squareys commented Aug 22, 2017

Hi @mosra !

As discussed via gitter, here is an example that shows how to use the emscripten vr API I am working on (to resolve kripken/emscripten#5028) together with Magnum.

Cheers, Jonathan.

PS: @mosra was so kind to upload a preview of this example to the website, if you want to see it in action.

TODO:

  • Screenshot
  • Possibly adapt to API changes

Note that this requires my branch of emscripten to compile, which builds on the latest branch of emscripten.

@Squareys Squareys changed the title from WebVR example to WIP: WebVR example Aug 22, 2017

@Squareys Squareys changed the title from WIP: WebVR example to [WIP] WebVR example Aug 22, 2017

@Squareys Squareys force-pushed the Squareys:webvr branch from 391408d to 3d3b24d Aug 22, 2017

@Squareys Squareys force-pushed the Squareys:webvr branch from 3d3b24d to 0f51459 Sep 28, 2017

@mosra

Initial review :)

if(CORRADE_TARGET_EMSCRIPTEN)
# TODO: give me INTERFACE_LINK_OPTIONS or something, please
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -s USE_SDL=2")
endif()

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

This is obsolete, right?

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

Ah, you mean because of the SDL2 changes we did? Or were there changes in the findmodule? I don't remember... is it?

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Because I reverted all the SDL2-on-Emscripten changes, we're still on SDL "1" there.

check_symbol_exists(EMSCRIPTEN_VR_API_VERSION "emscripten/vr.h" HAS_WEBVR_1_1)
if(NOT HAS_WEBVR_1_1)
message(ERROR "webvr-example requires WebVR 1.1 API in emscripten.")
endif()

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

As per the discussion on gitter, this will be removed (and moved to the source itself), right?

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

No, the example cannot be built without WebVR 1.1 (vs. 1.0) support in emscripten. But the code will allow two Emscripten API versions of the WebVR 1.1 support.

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

TL;DR this is still required

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Okay, great, thanks for the clarification.

# Don't treat imported targets with :: as files
if(POLICY CMP0028)
cmake_policy(SET CMP0028 NEW)
endif()

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

This needs to be updated with new policies from other examples.

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Oh, and moved before project() as the comment says (sorry).

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

Oh, I see, sorry!

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

(Done)

#
# 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 —
# Vladimír Vondruš <mosra@centrum.cz>
# 2017 — Jonathan Hale <squareys@googlemail.com>

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

The copyright years need to be updated 😂

DESTINATION ${CMAKE_INSTALL_PREFIX} OPTIONAL)
install(FILES webvr-emscripten.html EmscriptenApplication.js WebApplication.css DESTINATION ${CMAKE_INSTALL_PREFIX})
install(TARGETS magnum-webvr DESTINATION ${CMAKE_INSTALL_PREFIX})
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/magnum-webvr.js.mem DESTINATION ${CMAKE_INSTALL_PREFIX} OPTIONAL)

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Oh, right, so this won't work anywhere else than on Emscripten? Then I would wrap the WITH_WEBVR_EXAMPLE option in the root CMakeLists.txt in if(CORRADE_TARGET_EMSCRIPTEN) so people don't get confused.

The other examples now have a bit different installation procedure (installing the entry file as magnum-webvr/index.html instead of magnum-webvr.html, for example), check the other example files for details.

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

Oh, right, so this won't work anywhere else than on Emscripten?

Well, it requires WebVR 😂

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Which reminds me that a bunch of stuff in package/ci/travis.yml needs to be uncommented to have master building on Emscripten again :)

static void displayPresentCallback(void* app) {
static_cast<WebVrExample*>(app)->displayPresent();
}

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Anonymous namespace instead of static in these, please ;)

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

I could also make this static functions in WebVrApplication. What do you prefer?

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

if they can be free functions, let them be free (less typing for you) .. they could be also lambdas, though (maybe even less typing?)

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

I use vim 😈 (as in: not much more typing)
What, wow, you can pass references to lambdas? WTF, why don't I know this 😅

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

as long as they don't capture anything, they are the same as plain function pointers, yup

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

So:

    auto displayPresentCallback = [](void* app) {
        static_cast<WebVrExample*>(app)->displayPresent();
    };
    if (!emscripten_vr_request_present(_displayHandle, &init, 1, displayPresentCallback, this)) {
        Error() << "Error while requesting present permission for VR display.";
        exit();
    }

correct?

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

as long as they don't capture anything, they are the same as plain function pointers, yup

Ah, so I can even pass it in directly, without a variable? Would make for a pretty long if directive, though

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

I will go for the anonymous namespace free functions for the shared ones and with a lambda one for the newer-API emscripten_vr_init call.

}
}
}}

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Minor coding style clash (yours vs. mine): I'm omitting the {} for single-line blocks to compress vertically a bit. I don't strictly require you to change that, though :)

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

Omitting and joining the line?

if(foo) return;

right? (yes)

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

Well, it's not like I'm consistently adding them anyway, so I can consistently remove them

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

if not too long, joining the line, yes

thanks!

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

(Done)

<script async="async" src="magnum-webvr.js"></script>
</div>
</body>
</html>

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Ugh, I need to do an update of this markup in all examples, this is old :(

But keep it here as-is, for consistency.

const int halfWidth = 0.5f*_displayResolution.x();
Range2Di viewport[2] = {
{{}, inVR ? Vector2i{halfWidth, _displayResolution.y()} : _displayResolution},

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Code golf? :D

        {{}, _displayResolution*Vector2i::xScale(inVR ? 0.5f : 1.0f)},

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

Well... actually, the halfWidth is still used in the max part of the Range also. Still?

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

no, keep it as it is, it's simple enough ;)

if(!emscripten_vr_set_display_render_loop_arg(_displayHandle, displayRenderCallback, this)) {
Error() << "Failed to set display render loop for VR display!";
exit();

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Fail{} << instead of Error + exit maybe? (same above)

This comment has been minimized.

@Squareys

Squareys Jan 19, 2018

Contributor

I guess that's equivalent to Fail() << ? Should I use Debug{} << then also?

This comment has been minimized.

@mosra

mosra Jan 19, 2018

Owner

Fail{} is Fail(), yes. I'm trying to use {} instead of () in new code.

@Squareys Squareys force-pushed the Squareys:webvr branch 4 times, most recently from d9a8121 to 51adf53 Jan 19, 2018

@Squareys

This comment has been minimized.

Contributor

Squareys commented Jan 19, 2018

webvr_original

@mosra Finally... done! The above is the screenshot in original size as requested. CI is also enabled. Thanks for all the feedback, I once again learned a bunch 👍

@Squareys Squareys changed the title from [WIP] WebVR example to WebVR example Jan 19, 2018

@mosra

Thanks a lot for the fixes! There's some minor things left (I can fix that myself as well, if you don't have the time), other than that I'm ready to merge this 👍

For the full experience, this example requires a WebVR 1.1 capable browser and a VR headset.
@m_div{m-button m-primary} <a href="http://magnum.graphics/showcase/webvr/">@m_div{m-big} Live web demo @m_enddiv @m_div{m-small} uses WebAssembly & WebGL 2 @m_enddiv </a> @m_enddiv

This comment has been minimized.

@mosra

mosra Jan 21, 2018

Owner

The button says it's WebGL 2 ... is it or is it not?

This comment has been minimized.

@Squareys

Squareys Jan 22, 2018

Contributor

I don't think so. Shall I change it to "WebGL 1" or just leave it out entirely?

[magnum-examples GitHub repository](https://github.com/mosra/magnum-examples/tree/master/src/webvr).
- @ref webvr/WebVrExample.cpp "WebVrExample.cpp"
- @ref webvr/CMakeLists.txt "CMakeLists.txt"

This comment has been minimized.

@mosra

mosra Jan 21, 2018

Owner

The example file list here has to be alphabetically ordered, otherwise the prev/next buttons in the source listings (such as here on the bottom -- i see that I messed up there as well) don't match the order of the list. Doxygen limitation, sorry.

emscripten_vr_get_eye_parameters(_displayHandle, VREyeRight, &eyeRight);
_displayResolution = {2*int(Math::max(eyeLeft.renderWidth, eyeRight.renderWidth)), int(eyeLeft.renderHeight)};
emscripten_set_canvas_size(_displayResolution.x(), _displayResolution.y());

This comment has been minimized.

@mosra

mosra Jan 21, 2018

Owner

Emscripten is complaining about usage of a deprecated API here: https://travis-ci.org/mosra/magnum-examples/jobs/330965818#L2668 (it's possible that it changed since mid-2017, I have no idea).

This comment has been minimized.

@Squareys

Squareys Jan 22, 2018

Contributor

Yes for me locally aswell. I think I looked into this half a year ago, but for some reason left it there after all. Will check again.

This comment has been minimized.

@mosra

mosra Jan 22, 2018

Owner

Thank you!

@Squareys Squareys force-pushed the Squareys:webvr branch from a17b4e6 to 7fbc344 Jan 22, 2018

Squareys added some commits Aug 22, 2017

webvr: Add initial WebVR example
Signed-off-by: Squareys <squareys@googlemail.com>
webvr: Support emscripten WebVR API version 1.1-0 and 1.1-1
Signed-off-by: Squareys <squareys@googlemail.com>
webvr: Some changes requested in review
Signed-off-by: Squareys <squareys@googlemail.com>
Add EmscriptenApplication.js and WebApplication.css
Signed-off-by: Squareys <squareys@googlemail.com>
webvr: Create a example dox page and remove obsolete files
Signed-off-by: Squareys <squareys@googlemail.com>
ci: Enable emscripten and building WebVR example
Signed-off-by: Squareys <squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:webvr branch from 7fbc344 to 16bdc3b Jan 22, 2018

@Squareys

This comment has been minimized.

Contributor

Squareys commented Jan 22, 2018

@mosra Alright, done! Also reordered the commits and squashed a few together. (And rebased on master again)

@mosra

This comment has been minimized.

Owner

mosra commented Jan 22, 2018

Thank you! Will merge this afternoon together with putting the live example on the website :)

@mosra mosra added this to TODO in Project management via automation Jan 22, 2018

@mosra

This comment has been minimized.

Owner

mosra commented Jan 22, 2018

Merged as 25e5239, d1e71de and 6158b72. Not waiting for the CI, as it will take several eons again.

@mosra mosra closed this Jan 22, 2018

Project management automation moved this from TODO to Done Jan 22, 2018

@mosra mosra added this to the 2018.02 milestone Feb 16, 2018

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