Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the character controller to allow walking and jumping #277

Merged
merged 6 commits into from
Oct 24, 2023

Conversation

patowen
Copy link
Collaborator

@patowen patowen commented Mar 14, 2023

Resolves #127, resolves #60

Player movement

The rough high-level algorithm is as follows:

  • Check if the character is on the ground
  • Initiate a jump if appropriate
  • Accelerate the player based on which keys are held down
  • Apply gravity, air resistance, and speed cap
  • Use the computed velocity to move the player, resolving collisions as necessary

Camera orientation

The logic that keeps the player oriented vertically relative to the world is handled client-side.

  • If the player is almost facing vertically, yaw and potentially pitch the view to reach alignment.
  • Otherwise, roll the view to reach alignment.

There are several subtleties needed to do the following:

  • Keep the camera alignment consistent during player movement
  • Interpret mouse-based pitch and yaw controls
  • Convert an arbitrary orientation to an aligned orientation (algorithm described above)
  • Ensure that the player does not get lifted up from the ground by slanted walls

@patowen patowen force-pushed the character-controller branch 2 times, most recently from d3036d2 to bae56f2 Compare March 23, 2023 03:11
@patowen
Copy link
Collaborator Author

patowen commented May 21, 2023

Besides the things I've inevitably missed, there are a few changes that I know are needed before this PR is merged, but limits in my own level of experience and sense of good code structure mean that I will need feedback before I make these changes:

  • A lot of character_controller's methods have way too many arguments. (Need feedback on which abstractions make sense to combine arguments. Should there be a character state struct? Should functions take fewer mutable arguments in favor of return values? Should there be an overarching struct with a bunch of &mut self functions?)
  • Client's sim.rs has too much functionality. I believe it would make sense to have client-side-only character controller logic in a separate file. This would also allow orientation-changing code to be testable. (Need feedback on whether you agree and how much of this refactor should be in a separate PR)
  • There are multiple modules within character_controller, which should probably be separate files. (Need feedback on whether you agree with the idea of having character_controller be a folder within common with the character_controller module and its two submodules)

The character controller is definitely not perfect. The most glaring issue is that walking into an obtuse wall corner will result in some shaking. However, this is expected behavior, and fixing this I believe should be outside the scope of this PR. In addition, the necessary input lag when jumping means that some coyote time would likely be a good idea, but I also consider that out of scope.

I believe the most important aspects of the character controller are as follows:

  • It should feel relatively fun and intuitive
  • The code should be readable and malleable enough that improvements can be made by someone other than me.

Most other aspects would likely have a lower priority than adding other essential features to this game, like block manipulation and saving/loading.

@patowen
Copy link
Collaborator Author

patowen commented May 21, 2023

To better playtest the character controller, I created a temporary branch on my fork with block placing/breaking: https://github.com/patowen/hypermine/tree/collision-wip-block-placing-temp.

As of the writing of this comment, the character controller code between this branch and that branch are identical, and the only difference is that the collision-wip-block-placing-temp branch has hastily-put-together block manipulation functionality borrowed from the collision demo and adapted to partially work with netcode (but not with multiple clients).

@patowen patowen marked this pull request as ready for review May 21, 2023 17:10
@Ralith
Copy link
Owner

Ralith commented Jun 1, 2023

If the player is almost facing vertically, [...] Otherwise, [...]

Could we simplify this to always rotating around whatever axis produces the minimum rotation?

I believe it would make sense to have client-side-only character controller logic in a separate file.

Strong agree. In an ideal world where we fully understand how to factor everything, I'd expect sim.rs to contain only the most trivial logic, instead mainly dispatching to other modules.

Need feedback on whether you agree with the idea of having character_controller be a folder within common with the character_controller module and its two submodules

That seems fine.

I believe the most important aspects of the character controller are as follows: [...]

SGTM.

@patowen
Copy link
Collaborator Author

patowen commented Jun 19, 2023

Could we simplify this to always rotating around whatever axis produces the minimum rotation?

I didn't go this route because the math is more complicated, and I don't think it's any better for the player. One case to consider is the fact that a rotation about the look-axis (roll) is less jarring than a rotation about a perpendicular axis, since the object the player is looking at stays fixed. A roll is almost always ideal, and the main exception is when the camera is close enough to vertical that it's no longer obvious which way is right-way up, since then, the correction starts to feel random. That's when yaw and/or pitch becomes nicer.

For a concrete example, if the player is facing forward but is upside-down, I believe the right move is to turn it 180 degrees to make the player right-way-up, but one can reach a valid orientation by having it face straight up or straight down instead, which is only a 90 degree rotation.

All your other suggestions seem good to me, and I should get a chance to address them soon.

@patowen patowen force-pushed the character-controller branch 2 times, most recently from 546c43b to 9885ca3 Compare June 28, 2023 05:41
common/src/character_controller/vector_bounds.rs Outdated Show resolved Hide resolved
common/src/character_controller/vector_bounds.rs Outdated Show resolved Hide resolved
client/src/local_character_controller.rs Outdated Show resolved Hide resolved
client/src/local_character_controller.rs Outdated Show resolved Hide resolved
common/src/math.rs Show resolved Hide resolved
common/src/sim_config.rs Outdated Show resolved Hide resolved
common/src/sim_config.rs Outdated Show resolved Hide resolved
common/src/character_controller/mod.rs Show resolved Hide resolved
common/src/character_controller/vector_bounds.rs Outdated Show resolved Hide resolved
@patowen
Copy link
Collaborator Author

patowen commented Jul 4, 2023

I am getting some "WARN Unsatisfied existing bound is parallel to new bound. Is the character squeezed between two walls?" messages (a warning I added for an edge case that should ideally be rare). I'm not sure whether my latest changes on July 4th caused them or whether it was the case before then, but when I have more time, I should be able to find the root cause.

EDIT: This seems to be caused by displacement_reduction_factor becoming negative, which should not be allowed. There's a good chance this is caused by a math error given how frequently this occurs when walking from one slope to another, but I'll also want to handle similar errors that could be caused by machine epsilon.

EDIT2: I believe the main source of error here is now resolved. The displacement_reduction_factor needed to use bounded_vectors.displacement().magnitude() instead of the expected_displacement that would be outdated after the first iteration.

EDIT3: My next plan is to enhance the vector_bounds module to hopefully prevent all other instances of this unless a player is actually squeezed against two walls with opposing normals in a way that lacks a proper resolution. To do this, I'll need to (1) forbid the use of temporary bounds without applying them first, and (2) add a minimum cap for error_margin.

Step (1) is necessary because if a temporary bound is added but isn't already fulfilled, unless it's applied directly, it can cause weird interactions with the bound actually being applied. Step (2) is necessary to ensure error_margin doesn't become negative or too close to zero, as floating point precision could then violate the assumption that applying a bound makes it pass the check.

EDIT4: Step (1) is now done. As for step (2), I don't think that should be in this PR, as adding a cap like this for floating point precision could easily hide actual logic errors like the one I found initially. I believe the concern I found in this PR comment has finally been resolved.

common/src/node.rs Outdated Show resolved Hide resolved
client/src/graphics/window.rs Outdated Show resolved Hide resolved
common/src/sim_config.rs Outdated Show resolved Hide resolved
@patowen patowen force-pushed the character-controller branch 2 times, most recently from 65794f1 to 4b6996a Compare October 24, 2023 01:44
common/src/sim_config.rs Outdated Show resolved Hide resolved
common/src/sim_config.rs Outdated Show resolved Hide resolved
@patowen patowen merged commit 40a5a1e into Ralith:master Oct 24, 2023
6 checks passed
@patowen patowen deleted the character-controller branch October 24, 2023 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Walking Orient camera perpendicular to gravity
2 participants