Add OpenXRSpatialEntitiesUnified by dhoverb · Pull Request #483 · GodotVR/godot_openxr_vendors

dhoverb · 2026-04-02T18:35:18Z

Add a new singleton that can be used for customized spatial entities setup and update.

This singleton is intended to be an alternative to the existing builtin project settings (like enable_builtin_anchor_detection, enable_builtin_plane_detection, and enable_builtin_marker_tracking). The overall benefit is allowing more control over the spatial entity configuration without needing to reimplement context creation + query loop for every app.

Notable changes

Any number of "unified" spatial contexts can be created, at any time, and can be destroyed at any time
Unified spatial contexts are just a spatial context created with 1 or more capabilities (like anchors + planes).
Automatic updates can be toggled at any time, and can be set per-capability.
Almost all capability options (see OpenXRSpatialEntitiesUnified::CapabilityOptions) can be toggled at any time, and other initialization-only capability options can now be configured to use a value other than the project-setting value.
Previously un-customizable options are now customizable. One example is that apps can enable/disable polygon2d for planes (this wasn't possible before without reimplementing the context creation + query loop + other data structures).

This PR depends on godotengine/godot#118128

dhoverb · 2026-04-02T18:46:19Z

Documentation check may succeed once #480 is merged.

dsnopek

Thanks!

I'm not super familiar with EXT spatial entities in general, so I think @BastiaanOlij will ultimately need to review this

But I skimmed the code a bit and had a couple notes/questions :-)

dsnopek · 2026-05-05T21:34:04Z

+		        OpenXRSpatialEntitiesUnified.CAPABILITY_OPTIONS_ENABLE_PLANE_MESH_2D : false,
+		        OpenXRSpatialEntitiesUnified.CAPABILITY_OPTIONS_ENABLE_PLANE_POLYGON_2D : false,


Suggested change

OpenXRSpatialEntitiesUnified.CAPABILITY_OPTIONS_ENABLE_PLANE_MESH_2D : false,

OpenXRSpatialEntitiesUnified.CAPABILITY_OPTIONS_ENABLE_PLANE_POLYGON_2D : false,

OpenXRSpatialEntitiesUnified.CAPABILITY_OPTIONS_ENABLE_PLANE_MESH_2D: false,

OpenXRSpatialEntitiesUnified.CAPABILITY_OPTIONS_ENABLE_PLANE_POLYGON_2D: false,

dsnopek · 2026-05-05T21:36:18Z

+		An API to simplify code that requires spatial context(s) to be created with multiple spatial capabilities.
+	</brief_description>
+	<description>
+		The original motivation for this class was to create a single spatial context with multiple capabilities since otherwise some extensions would not work.


I don't think we should talk about "the original motivation", and instead just state what this class does.

Ex:

Suggested change

The original motivation for this class was to create a single spatial context with multiple capabilities since otherwise some extensions would not work.

This class allows creating a single spatial context with multiple capabilities, which is required by some OpenXR extensions.

dsnopek · 2026-05-05T21:37:40Z

+				[param callback] is called when the context is created, with the first parameter being the spatial context [RID].
+				[param capability_options] is a [Dictionary], where each key is [enum CapabilityOptions] and the value depends on the option.  See [enum CapabilityOptions].
+				Any capabilities set in [param enable_automatic_discovery_query] and [param enable_automatic_update_query] will automatically be updated after the context is created, assuming those capabilities are available by the XR runtime.
+				[b]NOTE:[/b] this does not return [OpenXRFutureResult] because sometimes it has async operations that must complete before the context is created.  A valid [param callback] must be provide for this function to be useful.


Suggested change

[b]NOTE:[/b] this does not return [OpenXRFutureResult] because sometimes it has async operations that must complete before the context is created. A valid [param callback] must be provide for this function to be useful.

[b]NOTE:[/b] this does not return [OpenXRFutureResult] because sometimes it has async operations that must complete before the context is created. A valid [param callback] must be provided for this function to be useful.

I ended up rephrasing this sentence slightly.

dsnopek · 2026-05-05T21:37:59Z

+			<param index="0" name="unified_spatial_context" type="RID" />
+			<description>
+				Destroy the spatial context created by [method create_unified_spatial_context].
+				[b]NOTE:[/b] Even though the spatial context can also be destroyed by [method OpenXRSpatialEntityExtension.free_spatial_context], be sure to call this method to ensure proper cleanup.


Suggested change

[b]NOTE:[/b] Even though the spatial context can also be destroyed by [method OpenXRSpatialEntityExtension.free_spatial_context], be sure to call this method to ensure proper cleanup.

[b]NOTE:[/b] Even though the spatial context can also be destroyed by [method OpenXRSpatialEntityExtension.free_spatial_context], be sure to call this method to ensure proper clean-up.

dsnopek · 2026-05-05T22:06:10Z

+				Index 0 is a [Dictionary] containing capability options for discovery, where the [Dictionary] maps [enum CapabilityFlags] to a [Dictionary], which maps [enum CapabilityOptions] to a [code]boolean[/code] indicating if it's enabled or not.
+				Index 1 is a [Dictionary] containing capability options for update, where the [Dictionary] maps [enum CapabilityFlags] to a [Dictionary], which maps [enum CapabilityOptions] to a [code]boolean[/code] indicating if it's enabled or not.


Why return an array with two dictionaries, rather than two different methods which each return a dictionary?

Done.

I made this function follow the pattern of the other functions, where one parameter is bool p_discovery. If it's true, it'll get the discovery data, and false it'll get the update data.

dsnopek · 2026-05-05T22:11:20Z

+public:
+	virtual uint64_t _get_configuration() override;
+
+	void init(bool p_enable_persistence);


This might be personal preference, but I would give this a longer name:

Suggested change

void init(bool p_enable_persistence);

void initialize(bool p_enable_persistence);

I don't have the data to back this up, but I feel we've got more initialize() than init() in Godot's public API

dsnopek · 2026-05-05T22:12:15Z

+public:
+	virtual uint64_t _get_configuration() override;
+
+	void init(bool p_supports_mesh_2d, bool p_supports_polygons, bool p_supports_labels);


Suggested change

void init(bool p_supports_mesh_2d, bool p_supports_polygons, bool p_supports_labels);

void initialize(bool p_supports_mesh_2d, bool p_supports_polygons, bool p_supports_labels);

dsnopek · 2026-05-05T22:14:09Z

+}
+
+bool OpenXRSpatialEntitiesUnified::UnifiedSpatialContextData::add_update_capability_option(BitField<OpenXRSpatialEntitiesUnified::CapabilityFlags> p_capability_flags, CapabilityOptions p_capability_option, const Dictionary &p_capability_options, bool p_adding_required) {
+	return update_datas[p_capability_flags].enable_capability_option(p_capability_option, !p_capability_options.has(p_capability_option) || !(bool)p_capability_options[p_capability_option], true, p_adding_required);


Is the second ! correct here? Or should this be:

Suggested change

return update_datas[p_capability_flags].enable_capability_option(p_capability_option, !p_capability_options.has(p_capability_option) || !(bool)p_capability_options[p_capability_option], true, p_adding_required);

return update_datas[p_capability_flags].enable_capability_option(p_capability_option, !p_capability_options.has(p_capability_option) || (bool)p_capability_options[p_capability_option], true, p_adding_required);

dsnopek · 2026-05-05T22:19:30Z

+
+void OpenXRSpatialEntitiesUnified::_on_create_unified_spatial_context_dependency_completed(RID p_completed_dependency, uint64_t p_unified_spatial_context_data, const Callable &p_user_callback, const Dictionary &p_capability_options, TypedArray<OpenXRSpatialCapabilityConfigurationBaseHeader> p_capability_configurations, Ref<OpenXRStructureBase> p_next, BitField<CapabilityFlags> p_capability_flags_completed, BitField<CapabilityFlags> p_capability_flags_just_completed) {
+	UnifiedSpatialContextData *unified_spatial_context_data = (UnifiedSpatialContextData *)p_unified_spatial_context_data;
+	if (CAPABILITY_FLAGS_ANCHOR_TRACKING != (p_capability_flags_completed & CAPABILITY_FLAGS_ANCHOR_TRACKING) && unified_spatial_context_data->capability_flags.has_flag(CAPABILITY_FLAGS_ANCHOR_TRACKING)) {


Is the CAPABILITY_FLAGS_ANCHOR_TRACKING != (p_capability_flags_completed & CAPABILITY_FLAGS_ANCHOR_TRACKING) check correct?

CAPABILITY_FLAGS_ANCHOR_TRACKING is a combination of flags, and this would check that none of them are enabled? Although, I'm very unsure I understand what's going on here, so maybe this is correct :-)

But if it is incorrect, this pattern is repeated a couple times below

I would've preferred to make this clearer but wasn't sure how :/

The code in this function is organized like:

if (/* need to process anchors */) { // collect capability option(s) related to anchors, might have a recursive call } if (/* need to process planes */) { // collect capability option(s) related to planes, might have a recursive call } if (/* need to process markers */) { // collect capability option(s) related to markers, might have a recursive call } // create spatial context with the collected capability options

One way to make this function clearer is to turn each of these massive blocks into their own functions, though that would only clarify what _on_create_unified_spatial_context_dependency_completed is doing, not so much their internal logic.
(the functions may have a bunch of out-vars since they modify some incoming state too, so it may be (even more) complex/difficult to read)

Anyway, at the end of each block, you see this:

// similar for planes and markers p_capability_flags_completed.set_flag(CAPABILITY_FLAGS_ANCHOR_TRACKING);

All this says is that we've completed anchors and they can be skipped in potential future recursive calls.

Which is why you see this check before process anchors (and similar for planes and markers):

// do we still need to process anchors? if (CAPABILITY_FLAGS_ANCHOR_TRACKING != (p_capability_flags_completed & CAPABILITY_FLAGS_ANCHOR_TRACKING /* maybe more checks*/ )) { // anchors are not done }

Currently only anchors has a recursive call (persistence has to wait for the persistence context to be created before continuing). So these checks for planes and markers is only for consistency and have no additional usefulness currently.

dsnopek · 2026-05-05T22:27:51Z

+	unified_spatial_context_data->capability_flags_automatic_update_query = unified_spatial_context_data->capability_flags & unified_spatial_context_data->capability_flags_automatic_update_query;
+
+	OpenXRSpatialEntityExtension *se_extension = OpenXRSpatialEntityExtension::get_singleton();
+	ERR_FAIL_NULL(se_extension);


If we return here, will the unified_spatial_context_data be freed anywhere?

BastiaanOlij · 2026-05-14T08:01:01Z

Couldn't get this to compile (yet), might be a step I missed with it complaining about a variant conversion.

Anyway, code wise there isn't much I can add that @dsnopek hasn't already said. I don't know if we had any discussion in the past yet for the use of auto and using Yoda notation. That's generally frowned upon in core but I haven't got a strong opinion either way for using it in vendors.

Functionally I think I follow how this all works. It reproduces what we do in core but trying to combine everything in a single unified class. I'm not sure which approach is better, it feels to me we're still in the same boat I was when implementing things in core. It's hard to see how this can extend once more types of capabilities and features are added. Especially with the issues we've had in the past when combining things that can't be combined.
Instead of writing our own classes to manage a new context for a new type of capability, this class will have to be enhanced to understand those capabilities.
The flip side of that argument is that we currently have limits enhancing the current capabilities with new features. We can't do this in core, this will potentially allow us to do so on the vendor side.

I don't have an answer here yet but I'm worried about us going in a direction that long term may be incorrect.

dhoverb · 2026-05-14T16:15:23Z

Especially with the issues we've had in the past when combining things that can't be combined.

If something cannot be combined with something else, this class would block or limit it, or make it clear to the user why two particular extensions cannot be used together.

Instead of writing our own classes to manage a new context for a new type of capability, this class will have to be enhanced to understand those capabilities.

That's correct. As new spatial entities are added, this class must be updated to support those new spatial entities.

The flip side of that argument is that we currently have limits enhancing the current capabilities with new features.

This is exactly why I wrote this, since it's otherwise impractical for anyone to use new future spatial entities that depends on existing spatial entities. It's definitely possible (you could write OpenXRSpatialEntitiesUnified in GDScript), but it would be comparatively inefficient performance-wise as well as programmer-time-wise since every app will have to implement some form of it.

The reason why I say "every app will have to implement some form of it" is because there is no automatic update/discovery query for custom spatial entity contexts. The moment you need to combine capabilities, you're on your own to correctly query updates and discoveries (and all of the bookkeeping that goes along with it). Since spatial entities can build on other spatial entities, this would mean every app would have to reimplement some form of OpenXRSpatialEntitiesUnified.

Some alternatives to this PR:

put it into a new GDExtension (in a new github repo). That way godot-xr-vendors does not commit to it, and, developers have an option for using new spatial entity extensions without needing to reimplement update/discovery logic.
update the godot-core implementation to allow automatic update/discovery queries for custom spatial entity contexts. It's been some time since I've looked at spatial entities, so it wouldn't surprise me if there is a massive caveat that would make this solution not work in the long term. If you're interested in this one, I can take a deeper look to determine if this option is practical.
Do both options 1 and 2

I'm open to other suggestions as well.

dsnopek reviewed Apr 2, 2026

View reviewed changes

Comment thread plugin/src/main/cpp/include/extensions/openxr_ext_spatial_entities_unified.h Outdated

dhoverb force-pushed the unified_spatial_entities branch 5 times, most recently from 74901c8 to 3c18f4f Compare April 3, 2026 15:01

dhoverb force-pushed the unified_spatial_entities branch 6 times, most recently from 6db241e to 53fac30 Compare April 14, 2026 21:10

dsnopek modified the milestones: 5.x, 6.x Apr 30, 2026

m4gr3d reviewed May 1, 2026

View reviewed changes

Comment thread thirdparty/godot_cpp_gdextension_api/extension_api.json

dhoverb force-pushed the unified_spatial_entities branch from 53fac30 to a329993 Compare May 5, 2026 18:14

dsnopek reviewed May 5, 2026

View reviewed changes

dsnopek marked this pull request as draft May 5, 2026 22:35

dhoverb force-pushed the unified_spatial_entities branch 4 times, most recently from 737e65b to f543a8d Compare May 8, 2026 16:03

Add OpenXRSpatialEntitiesUnified

0e5ff02

dhoverb force-pushed the unified_spatial_entities branch from f543a8d to 0e5ff02 Compare May 8, 2026 16:08

dsnopek added the enhancement New feature or request label May 13, 2026

		OpenXRSpatialEntitiesUnified.CAPABILITY_OPTIONS_ENABLE_PLANE_MESH_2D : false,
		OpenXRSpatialEntitiesUnified.CAPABILITY_OPTIONS_ENABLE_PLANE_POLYGON_2D : false,

	The original motivation for this class was to create a single spatial context with multiple capabilities since otherwise some extensions would not work.
	This class allows creating a single spatial context with multiple capabilities, which is required by some OpenXR extensions.

	[b]NOTE:[/b] this does not return [OpenXRFutureResult] because sometimes it has async operations that must complete before the context is created. A valid [param callback] must be provide for this function to be useful.
	[b]NOTE:[/b] this does not return [OpenXRFutureResult] because sometimes it has async operations that must complete before the context is created. A valid [param callback] must be provided for this function to be useful.

	[b]NOTE:[/b] Even though the spatial context can also be destroyed by [method OpenXRSpatialEntityExtension.free_spatial_context], be sure to call this method to ensure proper cleanup.
	[b]NOTE:[/b] Even though the spatial context can also be destroyed by [method OpenXRSpatialEntityExtension.free_spatial_context], be sure to call this method to ensure proper clean-up.

		Index 0 is a [Dictionary] containing capability options for discovery, where the [Dictionary] maps [enum CapabilityFlags] to a [Dictionary], which maps [enum CapabilityOptions] to a [code]boolean[/code] indicating if it's enabled or not.
		Index 1 is a [Dictionary] containing capability options for update, where the [Dictionary] maps [enum CapabilityFlags] to a [Dictionary], which maps [enum CapabilityOptions] to a [code]boolean[/code] indicating if it's enabled or not.

	void init(bool p_enable_persistence);
	void initialize(bool p_enable_persistence);

	void init(bool p_supports_mesh_2d, bool p_supports_polygons, bool p_supports_labels);
	void initialize(bool p_supports_mesh_2d, bool p_supports_polygons, bool p_supports_labels);

	return update_datas[p_capability_flags].enable_capability_option(p_capability_option, !p_capability_options.has(p_capability_option) \|\| !(bool)p_capability_options[p_capability_option], true, p_adding_required);
	return update_datas[p_capability_flags].enable_capability_option(p_capability_option, !p_capability_options.has(p_capability_option) \|\| (bool)p_capability_options[p_capability_option], true, p_adding_required);

Conversation

dhoverb commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhoverb commented Apr 2, 2026

Uh oh!

Uh oh!

Uh oh!

dsnopek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BastiaanOlij commented May 14, 2026

Uh oh!

dhoverb commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dhoverb commented Apr 2, 2026 •

edited

Loading