Add initial sphere-voxel collision detection

[patowen]

This PR adds collision detection logic. There is a sphere_cast method in the common::graph_collision module that answers the question of "How far can a sphere of a given radius move in a given direction, up to a given distance, before colliding with a voxel? Does it collide at all?" If there is a collision, it also finds the normal, which can be used for collision resolution.

For testing purposes, a basic form of collision resolution is also included with these changes. As long as noclip is disabled by pressing "V", if a character collides with a wall, it will stop. This has the advantage of simplicity, but it has the disadvantage of making walls "sticky", so smoother movement is planned in a future PR.

[Ralith]

Initial incomplete pass; more to come over coming days, but this should get things rolling.

[patowen]

I've updated this PR and added most of the necessary tests. The main shape casting method in the collision module still needs tests, as I've saved that for last, and it might take some time. I believe there are enough new changes that it's worth a review (especially reviewing the design of the test). When I find time, I'll add the remaining missing tests.

I believe everything other than collision::shape_cast is well-covered.

EDIT: I just added tests for collision::shape_cast, so everything should now be covered, and this PR should now be ready for another review.

[Ralith]

This is looking great, thanks!

[patowen]

Another round of changes has been completed. This time, I have added all the tests that I believe should be added, but I am open to suggestions if you think I missed something important. I also believe all your other suggestions have been addressed.

I made a few other minor changes as well that I noticed while rereading the code. Probably the most important change is that sphere_cast now takes a Position position instead of a matrix start_node_transform and a ChunkId start_chunk. There are at least two benefits to this: (1) The caller doesn't have to awkwardly pass in an arbitrary vertex, as that is handled internally, and (2) It's much easier to explain what the position parameter means in the documentation.

Thank you for reviewing this so carefully! It's been a really helpful learning experience for me.

[patowen]

@Ralith, looking at this code again, I think it might make more sense to try to make sphere_collider.rs more stand-alone. It doesn't need a ChunkId or a transformation matrix to compute anything internally; that's only used on the result. Is this something worth pursuing in this PR, or would you prefer it as a future refactor in a separate PR?

[Ralith]

That sounds like a nice improvement. I don't think it needs to block this PR, but if you get it into a state you're happy with before we merge this, then you might as well fold them together.

[patowen]

That sounds like a nice improvement. I don't think it needs to block this PR, but if you get it into a state you're happy with before we merge this, then you might as well fold them together.

Sounds good to me. Once you're reviewed it to your satisfaction and believe it is ready to merge as-is, I think the right move is to merge it, with the spirit of not letting "perfect" be the enemy of "good".

However, if I find time to make this adjustment before we reach that point, I'll go ahead and add it to this PR, adjusting the commit history as if this was the plan from the start. If we go this route, it may be a good idea for me to prepare another diff for you to reference so that it's easier for you to see what changed.

[Ralith]

it may be a good idea for me to prepare another diff for you to reference so that it's easier for you to see what changed.

Nah, no need to go that far.

[patowen]

Nah, no need to go that far.

Got it. I won't go that far, especially if you don't have a need for it. The real diff history will be available in https://github.com/patowen/hypermine/tree/collision-wip, since that's the branch I usually work on, but I won't spend any effort to clean that up.

EDIT: Looking at this again, it's definitely not good to merge as-is. Based on the abstraction-level changes that have been made, the collision module names no longer make sense. Since I've started working on the improvement I suggested, I think it makes sense to let it block the PR. I don't think it will take too long.

[patowen]

That sounds like a nice improvement. I don't think it needs to block this PR, but if you get it into a state you're happy with before we merge this, then you might as well fold them together.

@Ralith, I ended up making the changes I proposed. It does make some code more verbose and repetitive (especially chunk_collision::chunk_sphere_cast), but I would say it's an overall win for readability.

[Ralith]

Good progress; loved the refactor!

[patowen]

Ah, I just today realized that Github has a "Compare" button that works with force pushes. No wonder you didn't need me to prepare a diff.

[patowen]

Thanks for merging! I'm optimistic that we're past the hardest part of getting player physics that works similarly to the collision demo.

Besides code style, the biggest remaining open questions are how to make sure prediction logic feels right, especially when jumping, but assuming these questions can get resolved, we should be pretty close to being able to close #127. Depending on what criteria we're looking for, we might be already be able to close #34.

EDIT: Based on a discussion on Discord, we should be fine just having jumping activate on the next step, which should be good enough at least for a first draft.