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.

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

[Idea] Never panic when calling extension methods #129

Open
zmerp opened this issue Jan 21, 2023 · 4 comments
Open

[Idea] Never panic when calling extension methods #129

zmerp opened this issue Jan 21, 2023 · 4 comments

Comments

@zmerp
Copy link
Contributor

zmerp commented Jan 21, 2023

Currently when some extensions are disabled and you try to call some associated methods, for example request_display_refresh_rate, it would lead to a crash because there is one expect() call:

.expect("XR_FB_display_refresh_rate not loaded");

In such cases it seems OpenXR would return XR_ERROR_FUNCTION_UNSUPPORTED. We could also return that error code. In this way it would be simpler to just do .ok() for some calls like request_display_refresh_rate which don't require any special handling when it fails.

@Ralith
Copy link
Owner

Ralith commented Jan 21, 2023

In such cases it seems OpenXR would return XR_ERROR_FUNCTION_UNSUPPORTED

What's the basis for this conclusion? In general, extension functions that are not implemented by a runtime cannot return any particular value because they do not exist at all.

@zmerp
Copy link
Contributor Author

zmerp commented Jan 21, 2023

All functions belonging to extensions have this XR_ERROR_FUNCTION_UNSUPPORTED return value. it's not well documented, except for xrGetInstanceProcAddr:

xrGetInstanceProcAddr must return XR_ERROR_FUNCTION_UNSUPPORTED if instance is a valid instance and the string specified in name is not the name of an OpenXR core or enabled extension function.

So it seems that extensions functions could return this value if the platform specific loader already have them defined statically but the associated extension is not enabled for a particular instance. So, even if in openxrs all extension functions are loaded dynamically, I think it would be semantically correct and equivalent to return XR_ERROR_FUNCTION_UNSUPPORTED when the associated extension was not enabled.

@Ralith
Copy link
Owner

Ralith commented Jan 22, 2023

That's documenting the behavior of xrGetInstanceProcAddr, not the extension functions. An extension function which does not exist cannot have a specified return value.

@zmerp
Copy link
Contributor Author

zmerp commented Apr 6, 2023

I'm working on adding new high level wrappers for some extensions. Some existing wrappers (like Session::enumerate_color_spaces) panic when the relative extension was not loaded, while some (like HandTracker::create) return Err(sys::Result::ERROR_EXTENSION_NOT_PRESENT). I'm thinking that extension methods called from to Session or Instance + extension objects constructors should return ERROR_EXTENSION_NOT_PRESENT when the relative extension is not loaded; while it's ok for methods relative to extension objects (like HandTracker) to panic when the relative extension is not loaded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants