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

enum array output parameter validation of xrEnumerate functions is awkward #14

Closed
pH5 opened this issue Mar 29, 2019 · 6 comments
Closed
Assignees
Labels
bug Something isn't working synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab

Comments

@pH5
Copy link

pH5 commented Mar 29, 2019

The xrEnumerateViewConfigurations valid usage section states:

Valid Usage (Implicit)

  • If viewConfigurationTypeCapacityInput is not 0, viewConfigurationTypes must be a pointer to an array of viewConfigurationTypeCapacityInput XrViewConfigurationType values

Consequently, the core validation layer checks that all array members have a valid XrViewConfigurationType enum value before the function is called, even though viewConfigurationTypes is purely an output parameter. Since there is no enum value for an undefined / uninitialized view configuration type, the user has to awkwardly pick a valid value and initialize the array with that:

XrViewConfigurationType viewConfigurations[viewConfigurationCount];
for (uint32_t i = 0; i < viewConfigurationCount; i++) {
    viewConfigurations[i] = XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO;
}
result = xrEnumerateViewConfigurations(instance, systemId,
    viewConfigurationCount, &viewConfigurationCount, viewConfigurations);

This enforced initialization loop is confusing at best, especially if the runtime doesn't even support XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO. It would be better to allow passing an uninitialized or at least zero-initialized viewConfigurations array.

xrEnumerateReferenceSpaces has the same issue:

Valid Usage (Implicit)

  • If spaceCapacityInput is not 0, spaces must be a pointer to an array of spaceCapacityInput XrReferenceSpaceType values
@rpavlik rpavlik self-assigned this Jul 2, 2019
@rpavlik rpavlik added the bug Something isn't working label Jul 30, 2019
@rpavlik
Copy link
Contributor

rpavlik commented Jul 30, 2019

Good catch - looks like the valid usage generation (and validation layer generator) aren't paying attention to the fact that this is an output array of scalars. Output arrays of structs with type/next need their type/next initialized, but output arrays of scalars don't need to be initialized.

Although, on the other hand, it is true that it's a requirement on the runtime that all those values must be valid values - after the call. Maybe we just need to refine that generated validity.

cc @daveh-lunarg for validation layer

@rpavlik
Copy link
Contributor

rpavlik commented Jul 30, 2019

I have opened internal issue 1195 to track working group discussion on this.

@rpavlik rpavlik added the synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab label Sep 5, 2019
@jherico
Copy link
Contributor

jherico commented Oct 17, 2019

FYI, this was fixed in the validation layer by KhronosGroup/OpenXR-SDK-Source#138

@rpavlik-bot
Copy link
Collaborator

An issue (number 1195) has been filed to correspond to this issue in the internal Khronos GitLab.

If you have a Khronos account, you can access that issue at KHR:openxr/openxr#1195.

1 similar comment
@rpavlik-bot
Copy link
Collaborator

An issue (number 1195) has been filed to correspond to this issue in the internal Khronos GitLab.

If you have a Khronos account, you can access that issue at KHR:openxr/openxr#1195.

@rpavlik
Copy link
Contributor

rpavlik commented Apr 14, 2021

Fixed long ago, as noted.

@rpavlik rpavlik closed this as completed Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab
Projects
None yet
Development

No branches or pull requests

4 participants