Skip to content
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.

Sign up for GitHub

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

Clarify xrEnumerateInstanceExtensionProperties #173

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Clarify xrEnumerateInstanceExtensionProperties #173

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion specification/sources/chapters/instance.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,15 @@ different results if a pname:layerName is available in one call but not in
another.
The extensions supported by a layer may also change between two calls, e.g.
if the layer implementation is replaced by a different version between those
calls.
calls, or if an `XrInstance` is created or destroyed between those calls.

This function may: be called before an instance has been created;
implementations must: not assume an instance exists.

When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment below, the spec does not really know anything about a potential loader's implementation. If we have something that we are going to require here, we need to require it of the runtime or of the API layer.

this function must: still be implemented by API layers and runtimes, as it may
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new unenforceable must: - it was already required that runtimes implemented this function, but it wasn't required for API layers. Is there some path to make this enforceable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is new for API layers - it's already a must: at https://github.com/KhronosGroup/OpenXR-SDK-Source/blob/main/specification/loader/api_layer.adoc#api-layer-conventions-and-rules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for making it enforceable:

  • we don't currently have any CTS that's really usable for API layers. Perhaps this should change?
  • we could make the loader refuse to load extensions that do not implement the functions and where the detail doesn't matter. This would have a minor performance cost on startup.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not new via the loader spec; but the loader spec is full of things like this. What's being proposed here is a change to the OpenXR spec where we are much more careful. You can see the difference in style / enforcement in the MRs / PRs where I moved the negotiate functions from the loader spec to the OpenXR spec.

Copy link
Contributor Author

@fredemmott fredemmott Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is new for API layers - it's already a must:

My assumption here is that the 'must:' for API layers in the loader specification are actually requirements for API layers; is this the actually the case, or is this just describing assumptions that Khronos's loader implementation is allowed to make?

If the latter, that implies there's potentially such a thing as a conformant API layer that the Khronos loader is unable to load - that seems undesirable.

be invoked by API layers.
Comment on lines +189 to +192
Copy link

@rblenkinsopp rblenkinsopp Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd also like to see:

Suggested change
When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;
this function must: still be implemented by API layers and runtimes, as it may
be invoked by API layers.
When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;
this function must: still be implemented by API layers and runtimes, as it may
be invoked by API layers, and must: return the same information as the manifest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that belong here, or in the loader docs over on https://github.com/KhronosGroup/OpenXR-SDK-Source/pull/490/files ?

Copy link

@rblenkinsopp rblenkinsopp Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I think it belongs here because it's specifying the behaviour of the function rather than the detail of the manifest. It probably should have an accompanying note in the loader docs that mirrors it form the manifest perspective.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with adding it here is that the main spec doesn't know about the loader - or implementation details of a potential loader like the manifest. It does sound good to add it to the loader spec though.


include::{generated}/validity/protos/xrEnumerateInstanceExtensionProperties.adoc[]
--
Expand Down
Loading