-
Notifications
You must be signed in to change notification settings - Fork 67
chore: upgrade OpenXR-SDK 1.1.54 and fix build issues #205
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
base: master
Are you sure you want to change the base?
Conversation
I'm not sure what you're referring to here. I don't see any references to |
Update the generator to handle new types and requirements introduced in OpenXR SDK 1.1.54. Generator changes: - Derive `Default` for enums with a `0` value (required by `SpatialBufferEXT`). - Relax `has_non_default` check for enums; added workaround for `XrNegotiateLoaderInfo` to avoid unwanted implementations. - Fix identifier normalization for `XrSpatialMarkerArucoDict` and `XrSpatialMarkerAprilTagDict` (prevents invalid starting digits). - Exclude `XrRaycastHitResultANDROID` re-exports in high-level generation to avoid conflicts. - Temporarily exclude `XrSpatialAnchorsCreateInfoBaseHeaderML` and `XrFutureCompletionBaseHeaderEXT` from polymorphic builders due to complexity changes. Manual adjustments: - Update `sys/lib.rs` and `openxr/lib.rs` with necessary use statements and basetype workarounds. - Refactor `SpaceUserIdFB` and `FutureEXT` to use the `wrapper!` macro.
961bc88 to
a7bcba2
Compare
|
Thanks for the clarification! To explain my original confusion: I noticed that For this PR, I'll proceed with:
Since I'm not yet sure how strictly the generated code relies on the EXT variant (given the XML situation), I'd prefer to land this safely first. I will investigate the proper way to handle deprecation and unification in a future iteration. |
This commit addresses several warnings and compilation errors across the crate: 1. Generator: - Modified `generate_reader` to conditionally apply `#[allow(dead_code)]` to struct fields when no getters are generated. - This fixes the "field 0 never read" warning observed in empty event structs like `InteractionRenderModelsChangedEXT`. 2. Refactor: - Changed `UuidEXT` from a direct re-export to a type alias of `Uuid`. - Note: The `#[deprecated]` attribute is currently commented out to prevent internal warnings, pending a future cleanup. 3. Fixes: - Added missing `unsafe` blocks in `opengles.rs` required by recent edition/compiler updates. - Added hardcoded `AIBinder` definition in `platform.rs` to fix build errors.
| } else if meta.has_pointer || meta.has_array && name != "XrUuid" { | ||
| quote! { #[derive(Copy, Clone, Debug)] } | ||
| } else if meta.has_non_default { | ||
| } else if meta.has_non_default || name == "XrNegotiateLoaderInfo" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a special case? Document.
| .iter() | ||
| .cloned() | ||
| .filter(|&x| { | ||
| x != "XrRaycastHitResultANDROID" && x != "XrTrackableMarkerDatabaseEntryANDROID" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document.
| let has_zero = e | ||
| .values | ||
| .iter() | ||
| .any(|v| matches!(v.value, ConstantValue::Literal(0))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic appears twice. Should we compute it once up front and store the result in Enum?
| }); | ||
|
|
||
| let field_attr = if readers.clone().next().is_none() { | ||
| quote! { #[allow(dead_code)] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Document.
| MAX_COLOCATION_DISCOVERY_BUFFER_SIZE_META, MAX_VIRTUAL_KEYBOARD_COMMIT_TEXT_SIZE_META, Path, | ||
| SystemId, Time, Uuid, Version, | ||
| }; | ||
| // #[deprecated(note = "Use `Uuid` instead")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding commented-out code doesn't serve any useful purpose. Replace with an explanatory TODO or omit entirely.
| SystemId, Time, Uuid, Version, | ||
| }; | ||
| // #[deprecated(note = "Use `Uuid` instead")] | ||
| pub type UuidEXT = Uuid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit to introducing a new type alias here rather than reexporting the one we already generate regardless?
Notes regarding Generator Workarounds:
Polymorphic Builders Exclusion: I've temporarily excluded XrFutureCompletionBaseHeaderEXT. It seems the SDK update introduced a need for manual handle processing here, which is out of scope for this PR.
Android Structs: Excluded XrRaycastHitResultANDROID from simple_structs generation due to re-export conflicts.
Default Trait: Added logic to derive Default for enums (needed for SpatialBufferEXT), but had to patch XrNegotiateLoaderInfo manually to prevent it from cascading incorrectly.
Questions:
Uuid Usage: I noticed Uuid and UuidEXT are used separately in lib.rs. Is this the intended behavior or should they be unified?
Warnings: There is currently a dead_code warning on InteractionRenderModelsChangedEXT (field 0 never read). I plan to address this in a follow-up commit, but heads-up if you see it in CI.