Make MeshType an enum

[ratmice]

I'm uncertain if you'll want this patch, but it moves MeshType to using an enum instead of the alias for the ptex_sys::MeshType u32 wrapper. Here is a patch in case such a thing seems warranted.

[ratmice]

Let me know if you do want this, if so it should probably be done for the rest of the ptex::type aliases of this sort.

[davvid]

Thanks, yeah this does seem like a nice addition to me. I'll hold off on merging this if you plan on taking a stab at converting the others to follow suite.

[ratmice]

Thanks, I will look at the others tomorrow.

[ratmice]

The last patch turned out a little more involved, since FaceInfo was returning ptex_sys::EdgeIds I had to wrap that, and change a signature from returning a reference to an owned value, as I relied on the underlying value being Copy but perhaps it should store a reference instead?

I could break that out to it's own PR if that helps.

[davvid]

Bummer, sorry for leading you astray and suggesting that we should do this. I hadn't looked at the code too closely at the time and I didn't realize that MeshType (and others types) are already enums.

Sure, they're type aliases, but they're enums nonetheless. Thus, I'm not really sure what problem these changes are trying to solve. They add a chunk of copy/conversion code that seems like it's somewhat contrary to the goal of having a lightweight interface over the ptex core so I'm afraid I'm going to have to reject these changes.

I was going to leave review comments noting that library code should almost never panic and that the catch-all _ matches aren't needed at all, but I don't want to waste your time any further if we're not going to go down this path.

Regarding resolution and face_info() / face_info_mut() question ~ I think that's further evidence that we shouldn't create a shadow set of enums for these. We would want to have a face_info_mut() that returns a mutable reference while also supporting face_info() that returns a const reference/borrow. Creating shadow enums are against the grain of the natural semantics and make returning a borrowed reference impossible so that's another sign we probably shouldn't go down this route.

Sorry about that!

[ratmice]

Sure, they're type aliases, but they're enums nonetheless. Thus, I'm not really sure what problem these changes are trying to solve.

For the record, what it was trying to solve is better docs/IDE support, while there are enums under the hood in ptex-sys/lib.rs, proc-macros through the cxx bridge (I believe) turn them into a struct with a constant for each variant.

This effect can be seen in e.g. the docs https://docs.rs/ptex-sys/0.2.0/ptex_sys/ffi/struct.EdgeFilterMode.html#

It is due to this being exported as a struct/u32 that the _ + panics are needed, because we are actually matching on a const variant_name: struct { pub repr: u32 } rather than the actual variant which would allow dropping the catch-all.

That said, thanks for having a look.

Edit: That this repr field is also pub is a bit weird in that it seems like it allows you to construct values not in the enum.

[ratmice]

I hadn't considered it previously but perhaps there is a knob that could be turned to generate plain old enums without the wrapper struct. But looking at https://cxx.rs/shared.html I didn't see one (though it will translate enums with descriminants nicely in the other direction).

[davvid]

For the record, what it was trying to solve is better docs/IDE support

Thanks for the clarification, and sorry for closing this PR too quickly.

If you still have your branch around maybe we can resurrect this topic to improve the docs and IDE support. That's a very good reason, even if it does lead to a bit of a shadow hierarchy. I also wonder if maybe some of the enums could not use repr(i32) (and just be unqualified) and we can just handle the unqualified enum -> sys i32 repr conversions internally. In theory that should let us do without the calls to panic.

Since we're just talking about i32 the overhead should be really minimal.

[ratmice]

No problem, I went restored the branch. I probably won't have time to look at it today, IIRC the panics all go the other way around when going from the sys_repr(u32) -> rust enum . Thus I think the only way these surface is if a u32 besides one of the c++ enum values makes a round trip through the file format, or somehow makes it through the sys bindings.

[ratmice]

So, I definitely do think we can do better avoiding these panic calls, Once thing we could do is implement TryFrom instead, making these return a Result. Another option is we could include an Unknown(u32) variant for unrecognized values we'll likely never see. I would lean towards the former probably, since the latter would induce an extra match statement for something unlikely ever to be experienced internal representation detail.

[ratmice]

So now I see why I did From instead of TryFrom, as it introduces these conversion errors into existing API. That said I pushed it so we can at least look at the fallout.

One thing which might actually be worth doing first, is some tests on what happens when we actually use these unrecognized values passing them through the sys crate. Perhaps the ptex c++ lib already checks the values making the panics unreachable from rust. So, that might be another option.

[davvid]

I changed impl TryFrom<...> into impl From<...> to make it fail-safe, added some docstrings, and got rid of the repr(u32) on the high-level enums so that the repr values are not displayed in the documentation. It also allows rust to use a smaller representation at the trivial cost of a conversion from e.g. u8 to u32.

Thanks again! Hopefully this is an overall improvement.

[ratmice]

I had been waiting on ptex #79 before revisiting this one, but your follow up patches looked good to me, and seem like they both cover everything and clean things up nicely.