awtterpip / bevy_oxr

Apache License 2.0
253 stars 39 forks source link

Main crate shouldn't include locomotion code #78

Closed marceline-cramer closed 4 months ago

marceline-cramer commented 9 months ago

The main bevy_oxr crate includes xr_input::prototype_locomotion, which is general, application-specific behavior that shouldn't be included in low-level Bevy-to-OpenXR bindings. If someone needs alternative locomotion methods, bevy_oxr needs to either be forked and extended with the locomotion features, or the in-tree locomotion support will be rendered redundant.

ewized commented 9 months ago

Why do you have to fork? You could just not use the locomotion plugin. Then build your own motion controls.

SafariMonkey commented 9 months ago

Are you asking that the code be removed from the repo, or just factored into a separate crate developed in the same repo? The latter seems reasonable to me.

marceline-cramer commented 9 months ago

Why do you have to fork? You could just not use the locomotion plugin. Then build your own motion controls.

Right, that's what I mean by the in-tree locomotion support being rendered redundant. You don't have to fork. If you do, you can contribute your improved support and functionality upstream. However, if you don't want to go through the contribution process to bevy_oxr during your own project (which seems like a much more likely option for something as directly user-facing like locomotion),bevy_oxr suddenly contains this extraneous locomotion code that doesn't really belong there anymore.

Are you asking that the code be removed from the repo, or just factored into a separate crate developed in the same repo? The latter seems reasonable to me.

I agree, I think that locomotion support could benefit from being moved into a separate crate, perhaps named something like bevy_oxr_utils. Other things I think could benefit from the same move are the debug gizmo system and the debug hand rendering.

An iffier thing that could maybe be moved to a utils crate is the OculusController system. The reason I'm iffy on it is that since controller definitions are bound to physical hardware, it's easy to maintain upstream abstractions for them with all feature bases covered, when it's not so easy to do that with locomotion controls. At the same time though, it could still be expected that new physical controller abstractions would be contributed to the core crate really frequently, meaning lots of new releases will be necessary. Moving Oculus and future controllers into the utils crate would be the safe bet for maintainability but could also signify that it's non-essential, even though in reality it's extremely difficult to deal with the OpenXR actions system on the lowest level.

I think that the general problem I'm seeing here is a matter of inconsistent technical level. For example, my personal expectation going into a crate like bevy_oxr is that it should set up the lowest-level features like OpenXR initialization, integration into the Bevy renderer, and abstractions over input mechanisms. Instead, the crate really inconsistently spans multiple technical levels at once, such as that low-level support, but also mid-level controller abstractions and high-level locomotion and debug utilities. These changes in scope aren't reflected across the organization of the crate, which makes it really confusing to parse.

Schmarni-Dev commented 9 months ago

i think OculusController should be removed and the action api improved to be usable directly

marceline-cramer commented 9 months ago

i think OculusController should be removed and the action api improved to be usable directly

Maybe a good solution would be to have a more usable action API that can be easily used directly but controller abstractions in the utils crate for convenience?

Schmarni-Dev commented 9 months ago

i think we should drastically improve the actions api. but i also think that controller abstractions are a really bad idea since openxr is designed to be used with actions to be able to rebind them seperatly. which not be possible if a controller abstraction is used

SafariMonkey commented 9 months ago

To explain the problem with OculusController, but also why it exists:

The action system in OpenXR is based on interaction profiles, including e.g. the Oculus Touch controller profile (/interaction_profiles/oculus/touch_controller). Applications should ideally support as many profiles as they're able to (natively) test on, while runtimes may support profiles that don't match the hardware you are using (though I would expect them to preferentially select the "native" profile of their hardware). The current approach of exposing an abstraction built on the Oculus profile can be useful for prototyping, as most runtimes probably support it and it's more advanced than /interaction_profiles/khr/simple_controller, but it nullifies a major advantage of the OpenXR interaction system: the ability to provide optimised bindings for different controllers, and have the runtime choose the closest match.

ForTehLose commented 9 months ago

While I agree that the core crate shouldn't include anything but the basics, the prototype locomotion code was added when I was simply trying to even get a basic demo working. I believe that once the refactor that will enable webxr is complete code like the prototype locomotion systems should be in a separate create like utila or toolkit or something similar.

Schmarni-Dev commented 4 months ago

Main crate(s) no longer contain locomotion code