Details

Reviewers

Sergey Sharybin (sergey)
Bastien Montagne (mont29)
Ray Molenkamp (LazyDodo)
Campbell Barton (campbellbarton)

Maniphest Tasks

T71347: Virtual Reality - Milestone 1 - Scene Inspection
T71365: GSoC 2019: Core Support of VR Headsets through OpenXR -- Review

Commits

rB406bfd43040a: Ghost: Ghost-XR API to abstract away and access OpenXR functionality
rBa22573e243d7: Build System: Add OpenXR-SDK dependency and WITH_XR_OPENXR build option

Summary

Some points on the OpenXR-SDK dependency:

The repository is located at https://github.com/KhronosGroup/OpenXR-SDK (Apache 2).
We use the OpenXR loader lib from it, the headers, and some CMake utilities.
It contains a bunch of generated files, for which the sources are in a separate repository.
To use the injected OpenXR API-layers from the SDK (e.g. API validation layers), the SDK needs to be compiled from this other repository.
We could use that other repo by default, but I'd rather go with the simpler solution and allow people to opt in if they want advanced dev features.
WITH_OPENXR is disabled by default on macOS. No development or testing was done there, and there's no OpenXR compatible runtime for macOS in sight anyway.

Diff Detail

Repository

rB Blender

Branch

temp-openxr-buildstuff (branched from master)

Build Status

Buildable 6884
Build 6884: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Harbormaster completed remote builds in B5554: Diff 19398.Nov 5 2019, 11:50 AM

Julian Eisel (Severin) retitled this revision from XR: Add OpenXR-SDK dependency and WITH_XR build option to Core XR Support [part 1]: Add OpenXR-SDK dependency and WITH_XR build option.Nov 5 2019, 11:52 AM

@Bastien Montagne (mont29) added you for review of the install_deps part, @Ray Molenkamp (LazyDodo) for Windows stuff and @Sergey Sharybin (sergey) for general review.

Add back accidentally removed CMake files

No clue how those could end up being tagged as deleted...

Harbormaster completed remote builds in B5556: Diff 19400.Nov 5 2019, 12:37 PM

Julian Eisel (Severin) mentioned this in D6190: Core XR Support [part 2]: Ghost DirectX compatibility.Nov 5 2019, 1:43 PM

Julian Eisel (Severin) added a task: T71365: GSoC 2019: Core Support of VR Headsets through OpenXR -- Review.Nov 5 2019, 3:41 PM

Julian Eisel (Severin) mentioned this in D5537: GSoC 2019: Core Support of Virtual Reality Headsets through OpenXR.Nov 5 2019, 3:47 PM

Julian Eisel (Severin) added inline comments.Nov 5 2019, 4:04 PM

build_files/build_environment/CMakeLists.txt
101	Should this be wrapped by an `if(WITH_XR)`? Other optional dependencies don't do this here.

Once these patches land in master, XR will no longer be an optional dep, I'd perhaps wrap it with an if (WIN32) to single out platforms that support it, but it shouldn't require a WITH_XR option.

Julian Eisel (Severin) retitled this revision from Core XR Support [part 1]: Add OpenXR-SDK dependency and WITH_XR build option to Core XR Support [part 1]: Add OpenXR-SDK dependency and WITH_OPENXR build option.Nov 5 2019, 5:28 PM

Two clarifications:

I wrote WITH_XR in the title and previous comment, the option is actually called WITH_OPENXR.
What @Ray Molenkamp (LazyDodo) said about this not being an optional dependency only matters for the platform dev's perspective. For everybody else, the OpenXR SDK is still an optional dependency.

Correct: blender cmakelists.txt should have an option to build with or without openxr if people want, however the deps builder only should have options for things that have not yet landed in master (such as embree)

install_deps.sh part looks good to me.

Update OpenXR-SDK dependency to 1.0.3
Remove unrelated files (helpers for launching with Oculus runtime)
Hugely simplify system specific define and desktop environment setup

Harbormaster completed remote builds in B5582: Diff 19440.Nov 6 2019, 4:58 PM

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 6 2019, 4:59 PM

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Nov 6 2019, 5:08 PM

Demeter Dzadik (Mets) added a subscriber: Demeter Dzadik (Mets).Nov 7 2019, 4:57 PM

Dalai Felinto (dfelinto) added a task: T71347: Virtual Reality - Milestone 1 - Scene Inspection.Nov 13 2019, 3:33 PM

Sergey Sharybin (sergey) added inline comments.Nov 14 2019, 10:23 AM

CMakeLists.txt
1232–1240	Is it really expected to have those defines global for the entire Blender project? Our policy is to have defines and includes as local as possible.
build_files/build_environment/CMakeLists.txt
101	To me it seems that `make deps` should compile every library needed for the final release. I'd rather pay price of some "wasted" compile time over danger of missing library update. Worth hearing from Ray and Arto about it.
build_files/build_environment/patches/openxr_sdk.diff
8–24 ↗	(On Diff #19440)	With the recent changes from Ray about CRT flags in Blender, do we still need this?

Julian Eisel (Severin) added inline comments.Nov 14 2019, 11:27 AM

CMakeLists.txt
1232–1240	Indeed, I used to access them in two places, I think now they can be moved to the Ghost-XR API.
build_files/build_environment/CMakeLists.txt
101	We already checked (see Ray's comment), I just didn't catch that this is only for `make deps` when going over the patch again. So no `if(WITH_OPENXR)` here.

Remove now unnecessary OpenXR SDK patch for Win CRT linkage
Remove XR defines from top-level CMakeLists file, can be done locally
Disable XR features (and the OpenXR SDK dependency) by default on Apple

Harbormaster completed remote builds in B5668: Diff 19619.Nov 14 2019, 12:10 PM

Ray Molenkamp (LazyDodo) added inline comments.Nov 14 2019, 2:33 PM

build_files/build_environment/patches/openxr_sdk.diff
8–24 ↗	(On Diff #19440)	we should not, but i still have to apply and test this diff

Update OpenXR SDK dependency to version 1.0.6

Harbormaster completed remote builds in B6798: Diff 22106.Feb 26 2020, 9:34 PM

Here are step by step notes I made on updating the dependency:

Updating the OpenXR-SDK dependency

Repository: https://github.com/KhronosGroup/OpenXR-SDK
Note: Use the OpenXR-SDK repository, NOT the OpenXR-SDK-Source one!

* build_files/build_environment/cmake/openxr: may require version number changes
* build_files/build_environment/cmake/versions.cmake: update OPENXR_SDK_VERSION and OPENXR_SDK_HASH (MD5 of .tar.gz)
* build_files/build_environment/install_deps.sh: update OPENXR_VERSION and OPENXR_REPO_UID (commit hash)
* build_files/cmake/platform_win32.cmake: may require version number changes

Remember to always check the specification's change log for compatibility breaking changes.

Not sure where to put them but they were quite useful for myself on the last update.

Adding Campbell as reviewer, he's the buildsystem maintainer.

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Mar 2 2020, 10:26 AM

Sergey Sharybin (sergey) added inline comments.Mar 2 2020, 10:34 AM

CMakeLists.txt
186	Mark it as ADVANCED as well then perhaps?
build_files/cmake/Modules/FindOpenXR-SDK.cmake
32–33	These can be removed.
build_files/cmake/config/blender_full.cmake
41	If macOS does not have OpenXR this should not be attempting to force-enable on apple. Similar to, say, `WITH_GHOST_XDND`,
build_files/cmake/config/blender_release.cmake
42	Same as above.

Mark WITH_OPENXR as advanced on macOS
Remove unnecessary CMake search hints
Don't attempt to force enable WITH_OPENXR on macOS

Harbormaster completed remote builds in B6866: Diff 22245.Mar 2 2020, 11:04 AM

Sergey Sharybin (sergey) accepted this revision.Mar 2 2020, 2:11 PM

This revision is now accepted and ready to land.Mar 2 2020, 2:11 PM

Fix an oopsie in previous update

Harbormaster completed remote builds in B6877: Diff 22261.Mar 2 2020, 2:22 PM

install_deps.sh part actually needs some updates to reflect changes I did there last week, you need to add a `_update_deps_openxr()) function (probably empty since no other lib would depend on it currently I think?), just check the other builder funcs to see how it's handled.

Other issue I just realized: think we should find another name than WITH_OPENXR & co, OPENXR is horribly close (one typo away) from OPENEXR... Besides cursing people for their lack of imagination in lib names, maybe use VR_OPENXR ?

This revision now requires changes to proceed.Mar 2 2020, 3:35 PM

Fix Windows SDK lib name for new SDK version, patch by @Ray Molenkamp (LazyDodo)

Harbormaster completed remote builds in B6884: Diff 22272.Mar 2 2020, 3:53 PM

looks good on windows now, libs are in SVN as of a few minutes ago.

I have no opinion on the name change, it's a valid point, they are very close... on the other hand, if requiring having basic reading skills to build blender is somehow a too high bar... yikes

Update install_deps for recent changes
Rename WITH_OPENXR to WITH_XR_OPENXR
Temporarily disable WITH_XR_OPENXR by default

Harbormaster completed remote builds in B6886: Diff 22276.Mar 2 2020, 4:55 PM

Don't mind too much regarding WITH_OPENXR. But we should use "XR" not "VR" here, for compatibility: We plan to support AR/MR/whatever-R in future too, then the name would have to be changed. (To be clear, I changed it to WITH_XR_OPENXR.)

I also disabled the dependency by default for now, we can flick the switch at any point.

OPENXR -> XR_OPENXR should be done everywhere, in variable and function names, also in install_deps script, etc. The confusion can happen anywhere, better be safe, clear and consistent everywhere.

This revision now requires changes to proceed.Mar 3 2020, 3:23 PM

Only minor things, noted inline.

CMakeLists.txt
183–191	Defining the setting & description twice seems redundant. Why not do: # Disabled until there's more than just the build system stuff. Should be enabled soon. option(WITH_XR_OPENXR "Enable VR features through the OpenXR specification" OFF) if(APPLE) # There's no OpenXR runtime in sight for macOS, neither is code well # tested there -> disable it by default. mark_as_advanced(WITH_XR_OPENXR) endif()
build_files/cmake/Modules/FindOpenXR-SDK.cmake
2–13	Is there any reason to call this an 'SDK' ? Just wondering why this should be called an SDK compared to other libraries.
31	`/usr/local` isn't needed (was removed from package finders we maintain).

Brecht Van Lommel (brecht) added a subscriber: Brecht Van Lommel (brecht).Mar 4 2020, 9:12 AM

Brecht Van Lommel (brecht) added inline comments.

build_files/cmake/Modules/FindOpenXR-SDK.cmake
2–13	It's the name given by Khronos. There is a distinction between the OpenXR standard and SDK, I think it's good to use OpenXR-SDK here.

Campbell Barton (campbellbarton) added inline comments.Mar 4 2020, 11:10 AM

build_files/cmake/Modules/FindOpenXR-SDK.cmake
2–13	Good to know. Think this could be noted in here in the header.

Clarify OpenXR standard and OpenXR-SDK relation
Remove unnecessary CMake search path hint
Consistently use XR-prefix for OpenXR in build system
Fix wrong Windows library name (name changed in SDK repo)

Harbormaster completed remote builds in B6935: Diff 22367.Mar 4 2020, 12:55 PM

Julian Eisel (Severin) marked an inline comment as not done.Mar 4 2020, 12:56 PM

Julian Eisel (Severin) added inline comments.

CMakeLists.txt
183–191	Having the option disabled for non-apple platforms is just a temporary thing too ease the integration. Once the next VR patches land (as planned for 2.83) or at any other point, we can enable the option. For macOS it should remain disabled though. AFAIK we can't set an override-able default value without doing it through `option()` but we could also set a variable with the `ON` or `OFF` value depending on the platform. I.e. like we used to do it with `_init_FOO` until rBbbd5f30ad62c. Note that for `LLVM_STATIC` we also have the definition twice, which is why I did the same here.

Campbell Barton (campbellbarton) accepted this revision.Mar 4 2020, 1:03 PM

Bastien Montagne (mont29) accepted this revision.Mar 4 2020, 3:20 PM

This revision is now accepted and ready to land.Mar 4 2020, 3:20 PM

Minor fixes to info messages in install_deps.sh
Fix install_deps.sh failing to find unpacked OpenXR-SDK (mistake in unpacking sed command)

I did another clean run of install_deps.sh, everything seems fine now.

Also added notes for updating the dependency to the wiki: https://wiki.blender.org/wiki/Source/OpenXR_SDK_Dependency

Harbormaster completed remote builds in B6944: Diff 22377.Mar 4 2020, 4:21 PM

Closed by commit rBa22573e243d7: Build System: Add OpenXR-SDK dependency and WITH_XR_OPENXR build option (authored by Julian Eisel (Severin)). · Explain WhyMar 4 2020, 4:47 PM

This revision was automatically updated to reflect the committed changes.

Julian Eisel (Severin) added a commit: rBa22573e243d7: Build System: Add OpenXR-SDK dependency and WITH_XR_OPENXR build option.

Thank you all for the review!

Julian Eisel (Severin) added a commit: rB406bfd43040a: Ghost: Ghost-XR API to abstract away and access OpenXR functionality.Mar 17 2020, 9:43 PM

Core XR Support [part 1]: Add OpenXR-SDK dependency and WITH_OPENXR build option
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 22272

CMakeLists.txt

build_files/build_environment/CMakeLists.txt

build_files/build_environment/cmake/harvest.cmake

build_files/build_environment/cmake/openxr.cmake

build_files/build_environment/cmake/versions.cmake

build_files/build_environment/install_deps.sh

build_files/cmake/Modules/FindOpenXR-SDK.cmake

build_files/cmake/config/blender_full.cmake

build_files/cmake/config/blender_lite.cmake

build_files/cmake/config/blender_release.cmake

build_files/cmake/platform/platform_apple.cmake

build_files/cmake/platform/platform_unix.cmake

build_files/cmake/platform/platform_win32.cmake

source/blender/python/intern/CMakeLists.txt

source/blender/python/intern/bpy_app_build_options.c

Core XR Support [part 1]: Add OpenXR-SDK dependency and WITH_OPENXR build optionClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 22272

CMakeLists.txt

build_files/build_environment/CMakeLists.txt

build_files/build_environment/cmake/harvest.cmake

build_files/build_environment/cmake/openxr.cmake

build_files/build_environment/cmake/versions.cmake

build_files/build_environment/install_deps.sh

build_files/cmake/Modules/FindOpenXR-SDK.cmake

build_files/cmake/config/blender_full.cmake

build_files/cmake/config/blender_lite.cmake

build_files/cmake/config/blender_release.cmake

build_files/cmake/platform/platform_apple.cmake

build_files/cmake/platform/platform_unix.cmake

build_files/cmake/platform/platform_win32.cmake

source/blender/python/intern/CMakeLists.txt

source/blender/python/intern/bpy_app_build_options.c

Core XR Support [part 1]: Add OpenXR-SDK dependency and WITH_OPENXR build option
ClosedPublic
Actions

Revision Contents
Changeset List