TheButlah / ovr_overlay

Rust OpenVR bindings using an up-to-date OpenVR and autocxx
https://docs.rs/ovr_overlay
Other
7 stars 8 forks source link

Implement getting TrackedDeviceProperties. #8

Closed kitlith closed 1 year ago

kitlith commented 1 year ago

I wish that we weren't essentially duplicating the FFI enum here. I put the regexes I used to generate the type-safe enums in a comment, at least.

do you want this feature gated?

any other concerns?

TheButlah commented 1 year ago

All new subsystems should be feature gated, yeah

kitlith commented 1 year ago

This is still technically missing the following properties:

kitlith commented 1 year ago

should maybe make wrapper around sys::TrackedDeviceIndex_t, but it should be outside the system module as overlay module will also want it.

TheButlah commented 1 year ago

will review later this week

kitlith commented 1 year ago

I forgot to make the props module pub >_> will do that when i get the chance.

kitlith commented 1 year ago

Open question: how do we want to deal with the non-camel case property names in enums? currently we generate warnings.

We could:

TheButlah commented 1 year ago

Open question: how do we want to deal with the non-camel case property names in enums? currently we generate warnings.

We could:

* Munge the names further

* Silence the warnings, and leave them as-is.

Its ok to #[allow()] these enums, since they have a specific meaning in OpenVR and they are autogenerated from a regex

kitlith commented 1 year ago

ah i didn't check the test. sigh

Will allow the enum and fix the test thing.

kitlith commented 1 year ago

Had a thought while I was working on this: ideally we'd be referencing the enum values from the enum in the sys crate instead of embedding them directly here, right? But this might be something to punt on for now.

TheButlah commented 1 year ago

But this might be something to punt on for now.

Well, that might remove a ton of (possibly brittle) code.

Yes if the sys crate exposes it as an enum then we should directly reference that. IDK why the sys crate is using enums thoguh - C++ enums can be any integer it doesn't have to be one of the specified variants. So for safety, things like CXX and bindgen don't use enums in rust to represent c/c++ enums.

Is autocxx being a bit evil here?

kitlith commented 1 year ago

i might be blanking and they're constants instead of an enum.

It won't change the amount of code either way, just makes it less brittle.

EDIT: to clarify: even if the sys crate specifies it as an enum, it specifies it as a single enum, whereas I've split it up into multiple enums based on the type of the property involved. I already have a bypass hatch available so that you can use the sys type.

kitlith commented 1 year ago

Per discord discussion, will be splitting into MVP (just the bits that deal with the sys enum) and another PR for the new enums that are clear on types.

TheButlah commented 1 year ago

To help with the merge conflicts, I'm almost done with #17 rn

kitlith commented 1 year ago

hold on a second, I found a solution to the &CStr bit. lifetimes are a bit messy though.

TheButlah commented 1 year ago

hold on a second, I found a solution to the &CStr bit. lifetimes are a bit messy though.

later pr, not this one. About to merge.

kitlith commented 1 year ago

oh. sorry.

kitlith commented 1 year ago

there, extra commit removed, will punt to PR.

kitlith commented 1 year ago

ah, yes, CStr is unused, because the relevant impl is commented out.

TheButlah commented 1 year ago

bless, its done 😅 Now I can go get my food lol