Introduce two new unit vector types for MVector

[patowen]

Fixes #264

Introduce two new unit vector types for MVector:

• MPoint
• MDirection

MPoint has the constraint v.mip(v) == -1, while MDirection has the constraint v.mip(v) == 1. Every method signature requiring an MVector with one of these constraints has been updated to take one of these more constrained types to reduce the chance of mistakes.

[patowen]

This PR is an updated version of https://github.com/patowen/hypermine/pull/19, targeting the main Hypermine repo instead of my own fork.

@Ralith: One unresolved comment thread is https://github.com/patowen/hypermine/pull/19#discussion_r1957441609. I don't think any action is needed here, but I would like to double-check that I'm not missing anything important.

[Ralith]

The root problem there could be addressed with the proper serde annotations but we can leave that for future work (indefinitely, if you like). It's not actually important.

[patowen]

Ah, good to know there's an option there. I do agree that it's not really important, though. All MVector and its related types need to accept is f32 and f64, so as long as they do, I'm happy to leave the type constraints slightly more restrictive for simplicity.

[Ralith]

These names strike me as very verbose. If you're not comfortable dropping "Vector", can we at least drop "Unit" as implied?

[patowen]

I agree that they feel verbose.

Now that the PR is out, I'm now more comfortable trying to think of a better name for these types (without any restrictions). Since they will probably be used frequently, brevity is probably more important than clarity, and any clarity gaps can be filled with documentation comments.

I think it makes sense to include the terms "point" and "direction" in their name, since that is easier to understand than "time" and "space", and it seems precise enough for our purposes. Focusing on the "point" one, I'd be comfortable with any of these names, and I may also be comfortable with something I hadn't thought of:

• MUnitPointVector
• MUnitPoint
• MPoint
• MPointVector

Out of these 4, my main concern with MPointVector is that it seems like the type with the weaker constraint v.mip(v) < 0 that I had thought of making. However, since the current decision is not to make that type, it's probably fine to steal its name. I do like "unit" in the name mainly because it makes it clearer that the vector is normalized, but it does add 4 extra characters.

What would your suggestion be here? I'd like it to be as un-confusing as possible for someone unfamiliar with the codebase, and I want them to be usable with mip without seeming like a huge stretch. I'm hopeful that the "M" at the beginning will fulfill that purpose, as it stands for Minkowski, hopefully making it clear that we're working within that framework.

EDIT: I went ahead with MPoint because it is the one with the shortest name.

[patowen]

I don't love abusing Deref this way. It gets a bit rough having to guess where and how many explicit unary *s to use, and makes it easy to accidentally bypass invariants. What issues did you have implementing the interesting operators directly on the newtypes?

Hmm, I was afraid you'd say that. I would like some advice on how to proceed, as Deref seems like the best option based on my own experience and limited knowledge.

To directly answer your question, to add every mip implementation we want, we would need 9 of them, one for each combination of two types. While 8 of them could call the main MVector-MVector implementation, that still doesn't seem like good design to me.

I disagree with your objection that it makes it easy to accidentally bypass invariants, as the moment you try to bypass an invariant with an MPoint, for instance, you get an MVector back. Bypassing an invariant would be having an MPoint that lacks the MPoint invariant, which Deref cannot do.

However, your other objections are hard to argue against.

I used Deref to solve what I consider essential for a good design of the MPoint/MDirection/MVector trio: It must be ergonomic to define and use these types. One thing I really don't want to give up is the ability to use mip on any combination of these types, as it's such a common operation of doing any vector math here, and discoverability is important. I also don't want to require 9 implementations of mip to pull that off. I'd be okay with requiring 3 such implementations, since 2 of them can call the MVector implementation.

The problem is that none of the solutions to the above problem are ideal. It sometimes makes me think that the current version of Rust isn't as conducive to the newtype pattern as one might think, especially when the newtype is conceptually a strict subset of the type it wraps (instead of being disjoint).

Here are some solution ideas I've considered:

• Option 1 (current state of PR): Deref: It gets the job done, but it goes against the design of Deref and requires counterintuitive uses of * under certain circumstances. Note that this is how nalgebra handles unit vectors, so there is a precedent for this, but this might be bad design on nalgebra's end.

• Option 2: impl Into<&MVector>: Using impl Into<&MVector> in function signatures could allow the kind of substitution we want, but I expect this to disobey the principle of least surprise even more than option 1, as we may end up wanting to implement the From trait in other places for convenience.

• Option 2b: This may also be a use case of the AsRef trait.

• Option 3: AsMVector trait: There could be a custom trait for all MVector types with an fn as_mvector(&self) -> &MVector function. Functions that want to be flexible could accept an impl AsMVector, while functions that are called more rarely can just have the user call .as_mvector() explicitly.

• Option 3b: Option 3 may be a use case for the AsRef trait, but I don't really understand how that trait is meant to be used, and it might still be an abuse of a trait.

• Option 4: Don't define mip for MPoint or MDirection: One option would be to sidestep this problem entirely and just require everything to be explicit. However, I don't think we should be creating a stumbling block for anyone who uses MPoint or MDirection and wants to do any math with it, as it makes the code harder to both read and write (read because of meaningless noise in the code).

I'm okay with option 1 because I cannot think of a scenario where it would trip us up. Perhaps very briefly at compile time, but not at runtime. My second choice would be option 3, possibly with a better trait name.

[patowen]

Based on discussion on Discord, I'm going with option 3b for now.

[patowen]

@Ralith, one thing I'd like your thoughts on is how the operator traits should be implemented. Currently, they take value types (e.g. MVector instead of &MVector) for their arguments, so if you have two vector references, you have to dereference them with * before applying these operators.

It seems like, based on StackOverflow (https://stackoverflow.com/questions/38811387/how-to-implement-idiomatic-operator-overloading-for-values-and-references-in-rus) and nalgebra's codebase, the answer is to separately implement each operation for all four combinations, where each side of the binary option is a reference or a value. It might be worth doing that for our types as well to avoid needing to dereference when applying these operators, but it might be worth doing in a separate PR.

One way this caveat makes this PR messier is that the standardized use of as_ref to convert from, for example, an MPoint to an MVector also turns value types into reference types, so * has to be used more often than before. That would be an argument for adding these four implementations in this PR.

[Ralith]

It's a bit annoying, but the solution is indeed to just implement the operations for values, references, and every combination thereof.

[patowen]

Got it. I went ahead and made these changes. Since they seem to be rather involved, I decided to split this into three commits, where only the last one actually adds MPoint and MDirection.