Add OpenXR extension wrappers for fb_scene, fb_spatial_entity, fb_spatial_entity_query, fb_spatial_entity_container #66

maunvz · 2023-12-09T21:49:23Z

As title, following the pattern from the fb_scene_capture extension wrapper, I've added wrappers for the 4 extensions mentioned. They don't yet expose useful functionality to godot projects (beyond getting the singletons and checking if the extensions are supported), but provide most of the openxr related boilterplate so a later change can add an "XR Scene" abstraction that expresses a physical room's layout and surfaces to the a Godot scene.

m4gr3d

Looks good overall as a first step to add support for these extensions!

@BastiaanOlij @Malcolmnixon As a follow-up to this PR, we'll need to evaluate how we want to expose this functionality on the Godot side in a manner that's consistent.

m4gr3d · 2023-12-21T04:49:40Z

godotopenxrmeta/src/main/cpp/include/openxr_fb_scene_extension_wrapper.h

+/* Original contributed implementation:                                   */
+/*   Copyright (c) 2022-2023 MattaKis Consulting Kft. (Migeran)           */


You can remove this section and instead use the pattern in https://github.com/GodotVR/godot_openxr_vendors/blob/master/godotopenxrmeta/src/main/cpp/register_types.h

updated all headers to match that pattern (also switched to #pragma once)

m4gr3d · 2023-12-21T04:58:17Z

godotopenxrmeta/src/main/cpp/include/openxr_fb_spatial_entity_extension_wrapper.h

+	bool initialize_fb_spatial_entity_extension(const XrInstance instance);
+
+	std::map<godot::String, bool *> request_extensions;
+  std::map<XrAsyncRequestIdFB, SetSpaceComponentStatusCallback_t> set_status_callbacks;


nit: formatting issue?

m4gr3d · 2023-12-21T04:59:38Z

godotopenxrmeta/src/main/cpp/include/openxr_fb_spatial_entity_query_extension_wrapper.h

+
+	bool initialize_fb_spatial_entity_query_extension(const XrInstance instance);
+	void on_space_query_results(const XrEventDataSpaceQueryResultsAvailableFB* event);
+  void on_space_query_complete(const XrEventDataSpaceQueryCompleteFB* event);


nit: here and below, formatting issues?

m4gr3d · 2023-12-21T05:13:10Z

godotopenxrmeta/src/main/cpp/openxr_fb_spatial_entity_container_extension_wrapper.cpp

+    XrSpaceContainerFB spaceContainer = {XR_TYPE_SPACE_CONTAINER_FB};
+    xrGetSpaceContainerFB(SESSION, space, &spaceContainer);
+    std::vector<XrUuidEXT> uuids(spaceContainer.uuidCountOutput);
+    spaceContainer.uuidCapacityInput = uuids.size();
+    spaceContainer.uuids = uuids.data();
+    xrGetSpaceContainerFB(SESSION, space, &spaceContainer);
+		return uuids;
+}


nit: the indents used in this method are different from the ones used in the previous methods.

BastiaanOlij · 2023-12-21T07:41:25Z

@BastiaanOlij @Malcolmnixon As a follow-up to this PR, we'll need to evaluate how we want to expose this functionality on the Godot side in a manner that's consistent.

In Godot 3 we introduced Anchors based on how ARKit solved this, most of this logic is still in place but not all.
Whether detected by plane detection or otherwise detected, any location that was tracked in space was created as an XRPositionalTracker with the type set to XRServer::TRACKER_ANCHOR. While this location was mostly static, ARKit would update locations as it learned more about its surroundings. It would even merge or split anchors when needed.

So that part of the solution I think is sound and we should apply here too. I don't know the details of the flow Meta currently has but if any spatial location is detected, we should create a positional tracker and register it with the XRServer by calling XRServer::add_tracker. The XRServer already emits signals so the game can react to the creation of a new tracker and make any additions to the scene. This entails adding an XRAnchor3D node to the scene that is bound to the newly created tracker.

Using XRAnchor3D also ensures that the location is adjusted when we recenter the player.

What is missing is what to do after this, I wasn't at all happy with how we did things in Godot 3 so I left this out in the conversion to Godot 4. We need a mechanism to expose further meta data about the anchor and provide a collision shape and/or occlusion mesh.

These could become OpenXRCollisionShape and OpenXRMesh classes where you specify the tracker, the problem is that this would suggest this would also apply for other trackers such as controllers.
This is how we did it in Godot 3 where OpenVR also allowed us to provide render models in this fashion however with OpenXR this is no longer sufficient. With OpenXR the vendor render model APIs are all GLTF based and it looks like this is a trend. We could paint ourselves in a corner if this trend continues into a more generic AR solution.

maunvz · 2023-12-22T18:39:21Z

addressed the formatting nits (now using tabs throughout, turned on whitespace rendering in my editor to catch these sooner lol). Is there / should there be a linter for this project?

BastiaanOlij · 2023-12-23T01:31:35Z

addressed the formatting nits (now using tabs throughout, turned on whitespace rendering in my editor to catch these sooner lol). Is there / should there be a linter for this project?

We use clang-format in most of our projects but doesn't look like we set that up here. Probably worth doing.

maunvz · 2023-12-25T00:46:51Z

Added an export option to request the USE_SCENE permission as per https://developer.oculus.com/documentation/native/native-spatial-data-perm/

From a discussion in #68 the permission is already auto-requested by GodotActivity's existing logic

godotopenxrmeta/src/main/cpp/export/meta_export_plugin.h

m4gr3d

Looks good!

dsnopek · 2024-01-04T17:53:32Z

godotopenxrmeta/src/main/cpp/openxr_fb_scene_extension_wrapper.cpp

+}
+
+void OpenXRFbSceneExtensionWrapper::_bind_methods() {
+	ClassDB::bind_static_method("OpenXRFbSceneExtensionWrapper", D_METHOD("get_singleton"), &OpenXRFbSceneExtensionWrapper::get_singleton);


@m4gr3d @BastiaanOlij I know this is a pre-existing pattern from OpenXRFbSceneCaptureExtensionWrapper which is already merged, but I'm curious how this pattern of registering a static get_singleton() method came to be? Because this isn't how singletons are usually bound in Godot.

Usually, you'd register a singleton in register_types.cpp via:

Engine::get_singleton()->register_singleton("OpenXRFbSceneExtensionWrapper", OpenXRFbSceneExtensionWrapper::get_singleton());

The pattern used here certainly works, but it leads to an unusual API from GDScript. If there isn't a compelling reason for this, I think we should switch to the usual way. I can open a separate issue about that, though! I just happened to notice it when reviewing this PR.

I wasn't aware of the reigster_singleton(...) approach; let's use that instead and update OpenXRFbSceneCaptureExtensionWrapper accordingly!

@dsnopek What's the earliest that Engine::get_singleton()->register_singleton(...) can be used?

I tried invoking it during MODULE_INITIALIZATION_LEVEL_CORE and I kept getting crash errors because the Engine singleton was not available.

Also, doing Engine.get_singleton(...) does not retrieve the singleton instance.

Here's a draft PR for reference: #72

I'm getting segfaults when registering using this approach. Did a bit of digging, and I think it's due to Engine::get_singleton() not being available in a gdextension

ERROR: Failed to retrieve non-existent singleton 'Engine'. at: get_singleton_object (core/config/engine.cpp:270) ERROR: Parameter "singleton_obj" is null. at: get_singleton (thirdparty/godot-cpp/gen/src/classes/engine.cpp:49)

Digging into the code, godot-cpp auto-generates bindings that eventually call into Engine::get_singleton_object(const StringName &p_name) and passes the class name (in this case "Engine"), but godot never actually registers Engine itself as a singleton in this form, within the godot/ source tree it's just a raw pointer, and since it doesn't extend Object it's not trivial to register like all the other Singleton instances. From godot/core/config/engine.cpp

Engine *Engine::singleton = nullptr; Engine *Engine::get_singleton() { return singleton; }

I think we'd need to address that in the engine before we can use Engine::get_singleton() from an extension? In contrast, main.cpp registers Performance as follows: engine->add_singleton(Engine::Singleton("Performance", performance));

Not sure if you encountered something similar in #73 @dsnopek

@maunvz There was some logic missing in register_types.cpp that @dsnopek fixed in https://github.com/GodotVR/godot_openxr_vendors/pull/73/files#diff-aaccbe3504fdb5822961fde8149e985f5539985f9e60a4a31d804f3bf5feb5e4.

In the updated register_types.cpp, the singleton registration is initialized at the MODULE_INITIALIZATION_LEVEL_SCENE level and the minimum initialization level was also updated to MODULE_INITIALIZATION_LEVEL_SCENE.
With those changes in place, registration via the engine works as expected.

BastiaanOlij · 2024-01-12T01:49:12Z

godotopenxrmeta/src/main/cpp/include/openxr_fb_scene_extension_wrapper.h

+struct XrSceneObjectInternal {
+	XrUuidEXT uuid;
+	XrSpace space;
+	std::optional<std::string> label;


Please use Godots implementations whenever possible to prevent needless conversions of data between internal data and Godot.

This collection of OpenXR extensions use XrSpace and XrUuidEXT across multiple API calls to request different metadata and poses (for example, once you've queried for all available anchors, you can locate that anchor or ask about it's semantic labels / bounding volumes, all with uuid / space). The intent here is to store in the original OpenXR formats to facilitate later calls associated with the same anchor, and translate to a Godot type in some higher level "XrSceneManager" that gets used by gdscript (rather than using these wrappers directly)

@maunvz The comment was referring to the use of the std::string type, you can use godot::String instead here and throughout this PR.

maunvz · 2024-01-12T08:03:13Z

updated to remove unused constants, request the scene permission manually, based on #71, and use reigster_singleton(...) in the new additions.

m4gr3d · 2024-01-12T14:56:27Z

godotopenxrmeta/src/main/cpp/include/openxr_fb_scene_extension_wrapper.h

+
+	// Vertices and lines on a plane, probably use this for a floor / ceiling as they are irregularly shaped
+	// We store a std::vector instead of XrBoundary2DFB to own the vertex memory
+	std::optional<std::vector<XrVector2f>> boundary2D;


Similarly, you can replace std::vector with godot::Vector throughout this PR

related: If I have "using namespace godot;" at the top, should I omit the godot:: prefix throughout the rest of the file (including in headers and variable types)?

Yeah, you can omit the godot:: prefix.

m4gr3d · 2024-01-12T15:06:45Z

godotopenxrmeta/src/main/cpp/openxr_fb_scene_extension_wrapper.cpp

+			"TABLE,COUCH,FLOOR,CEILING,WALL_FACE,WINDOW_FRAME,DOOR_FRAME,STORAGE,BED,SCREEN,LAMP,PLANT,WALL_ART,INVISIBLE_WALL_FACE,OTHER";
+	const XrSemanticLabelsSupportInfoFB semanticLabelsSupportInfo = {


Should this list also include GLOBAL_MESH as mentioned in the documentation?

@maunvz ping on this ^^

m4gr3d · 2024-01-12T15:20:44Z

godotopenxrmeta/src/main/cpp/include/openxr_fb_scene_extension_wrapper.h

+	std::optional<std::string> get_semantic_labels(const XrSpace space);
+	void get_shapes(const XrSpace space, XrSceneObjectInternal& object);


Here and throughout the PR, we should use a const XrSpace &space argument instead.

hm does that actually make a difference? the XrSpace, like many other XR handles, is just a pointer / uint64_t, not a full object. See openxr.h:

#if !defined(XR_DEFINE_HANDLE) #if (XR_PTR_SIZE == 8) #define XR_DEFINE_HANDLE(object) typedef struct object##_T* object; #else #define XR_DEFINE_HANDLE(object) typedef uint64_t object; #endif #endif ... XR_DEFINE_HANDLE(XrSpace)

Will update to match the style of the rest of the codebase

maunvz · 2024-01-18T04:39:57Z

updated XrSpace usage, removed unnecessary godot:: prefixes, switched to Godot types throughout, and switched to new singleton registration

m4gr3d · 2024-01-18T04:58:35Z

godotopenxrmeta/src/main/cpp/openxr_fb_scene_extension_wrapper.cpp

+		return std::nullopt;
+	}
+
+	static const std::string recognizedLabels =


Another usage of std:string

m4gr3d · 2024-01-18T04:59:19Z

godotopenxrmeta/src/main/cpp/openxr_fb_scene_extension_wrapper.cpp

+	// First call.
+	xrGetSpaceSemanticLabelsFB(SESSION, space, &labels);
+	// Second call
+	std::vector<char> labelData(labels.bufferCountOutput);


Another usage of std::vector

m4gr3d · 2024-01-18T05:02:00Z

godotopenxrmeta/src/main/cpp/openxr_fb_spatial_entity_extension_wrapper.cpp

+bool OpenXRFbSpatialEntityExtensionWrapper::is_component_supported(const XrSpace& space, XrSpaceComponentTypeFB type) {
+	uint32_t numComponents = 0;
+	xrEnumerateSpaceSupportedComponentsFB(space, 0, &numComponents, nullptr);
+	std::vector<XrSpaceComponentTypeFB> components(numComponents);


Another usage of std::vector

m4gr3d · 2024-01-18T05:16:20Z

godotopenxrmeta/src/main/cpp/include/openxr_fb_spatial_entity_query_extension_wrapper.h

+
+	std::map<XrAsyncRequestIdFB, Vector<XrSpaceQueryResultFB>> query_results;
+	std::map<String, bool *> request_extensions;
+	std::map<XrAsyncRequestIdFB, SpaceQueryCompleteCallback_t> query_complete_callbacks;


You should be able to use godot::HashMap here instead

m4gr3d

Looks good overall!

Just some minor nits regarding types to address before merging.

maunvz · 2024-01-18T07:50:53Z

addressed final type nits, added missing SCENE_MESH semantic label

m4gr3d · 2024-01-18T17:04:49Z

Thanks for the contribution!

maunvz force-pushed the scene_ext branch 3 times, most recently from c7cbe0a to 970d1aa Compare December 11, 2023 00:56

m4gr3d requested review from m4gr3d and BastiaanOlij December 21, 2023 00:51

m4gr3d added this to the 2.1.0 milestone Dec 21, 2023

m4gr3d requested changes Dec 21, 2023

View reviewed changes

maunvz force-pushed the scene_ext branch from 970d1aa to 43b5f1d Compare December 22, 2023 18:36

maunvz requested a review from m4gr3d December 22, 2023 20:43

m4gr3d mentioned this pull request Dec 23, 2023

Add additional export options for Meta Quest features #68

Closed

m4gr3d reviewed Jan 3, 2024

View reviewed changes

godotopenxrmeta/src/main/cpp/export/meta_export_plugin.h Outdated Show resolved Hide resolved

m4gr3d approved these changes Jan 3, 2024

View reviewed changes

dsnopek reviewed Jan 4, 2024

View reviewed changes

BastiaanOlij reviewed Jan 12, 2024

View reviewed changes

maunvz force-pushed the scene_ext branch from 43fda9a to 729ccb5 Compare January 12, 2024 07:59

m4gr3d requested changes Jan 12, 2024

View reviewed changes

maunvz force-pushed the scene_ext branch from 729ccb5 to 9953436 Compare January 18, 2024 04:38

m4gr3d reviewed Jan 18, 2024

View reviewed changes

m4gr3d approved these changes Jan 18, 2024

View reviewed changes

maunvz added 2 commits January 17, 2024 23:48

Add OpenXR extension wrapper for fb_spatial_entity

9765dbe

Add OpenXR extension wrapper for fb_spatial_entity_query

b60616b

maunvz and others added 3 commits January 17, 2024 23:48

Add OpenXR extension wrapper for fb_spatial_entity_container

0a3c425

Add OpenXR extension wrapper for fb_scene

a4920ec

Add export option to support scene API and request if in manifest

baa01ff

maunvz force-pushed the scene_ext branch from 9953436 to baa01ff Compare January 18, 2024 07:49

m4gr3d merged commit 2b31718 into GodotVR:master Jan 18, 2024
5 checks passed

BastiaanOlij mentioned this pull request Jan 26, 2024

Preparing release 2.0.3 #78

Merged

BastiaanOlij modified the milestones: 2.1.0, 2.0.3 Jan 26, 2024

dsnopek mentioned this pull request Mar 11, 2024

Add a developer-facing API for interacting with scene anchors #107

Merged

		/* Original contributed implementation: */
		/* Copyright (c) 2022-2023 MattaKis Consulting Kft. (Migeran) */

		"TABLE,COUCH,FLOOR,CEILING,WALL_FACE,WINDOW_FRAME,DOOR_FRAME,STORAGE,BED,SCREEN,LAMP,PLANT,WALL_ART,INVISIBLE_WALL_FACE,OTHER";
		const XrSemanticLabelsSupportInfoFB semanticLabelsSupportInfo = {

		std::optional<std::string> get_semantic_labels(const XrSpace space);
		void get_shapes(const XrSpace space, XrSceneObjectInternal& object);

Add OpenXR extension wrappers for fb_scene, fb_spatial_entity, fb_spatial_entity_query, fb_spatial_entity_container #66

Add OpenXR extension wrappers for fb_scene, fb_spatial_entity, fb_spatial_entity_query, fb_spatial_entity_container #66

Conversation

maunvz commented Dec 9, 2023

m4gr3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BastiaanOlij commented Dec 21, 2023 • edited Loading

maunvz commented Dec 22, 2023

BastiaanOlij commented Dec 23, 2023

maunvz commented Dec 25, 2023

m4gr3d left a comment

Choose a reason for hiding this comment

dsnopek Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4gr3d Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maunvz commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maunvz commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4gr3d left a comment

Choose a reason for hiding this comment

maunvz commented Jan 18, 2024

m4gr3d commented Jan 18, 2024

BastiaanOlij commented Dec 21, 2023 •

edited

Loading

dsnopek Jan 4, 2024 •

edited

Loading

m4gr3d Jan 12, 2024 •

edited

Loading