GodotVR / godot_openxr_vendors

Godot 4 wrapper for OpenXR vendors loaders and extensions
MIT License
99 stars 22 forks source link

Initialize all values of XrHandTrackingScaleFB struct #100

Closed devloglogan closed 7 months ago

devloglogan commented 7 months ago

@dsnopek told me he was experiencing some bugs in the demo regarding the mesh scale for the XR_FB_hand_tracking_mesh implementation. He believed it was occurring because the override properties of the XrHandTrackingScaleFB struct were not being initialized, so junk data could be populating them. Hopefully this fixes that!

m4gr3d commented 7 months ago

Thanks!

Yeah, I was getting freaky giant hands about 50% of the time. It seemed most likely to happen when I completely quit the app on the headset and then launched it (as opposed to having the app already running, and then launching it again). So, basically, I'd start it, my hands would be crazy, I'd deploy it again, and they'd be normal.

I just tested this patch, fully quitting and restarting the demo app 4 times, and trying a re-launching when it's already running 2 times, and I got normal-sized hands every time :-)

I ran into a similar issue, but I wasn't able to reproduce it so I attributed to my device behaving weirdly.

@devloglogan To avoid those type of issue, I think we should standardize on using braced-initialization for enums.

You can see an example in https://github.com/godotengine/godot/blob/9b94c80e9aff2a4f363ae6d8e2bbe837aa5876bc/modules/openxr/extensions/openxr_hand_tracking_extension.cpp#L94. @BastiaanOlij and I had run into a similar issue in Godot 3.x, and standardized on that approach to address it, but I guess we forgot to bring it forward.

dsnopek commented 7 months ago

I think we should standardize on using braced-initialization for structs.

I also like the braced-initialization for structs with the names in comments like that, although, I've also not used it every time. :-)

I'd support standardizing on it!

devloglogan commented 7 months ago

Makes sense to me! I've swapped to braced-initialization.