Skip to content

Conversation

@Tokey
Copy link

@Tokey Tokey commented Sep 3, 2022

  • The Sprint Multiplier slider to player controls. MoveRate will be multiplied with Sprint Multiplier to make the player move faster as long as LShift is held.

  • HeadBob Amplitude and Frequency can be changed from Player Movement Panel.

  • Setting movementDeceleration and movementAccelaration both to 1 from Player Movement Panel would remove acceleration.

HeadBob Amplitude and Frequency can be changed from Player Movement panel.
@Tokey Tokey changed the title Parameterized Sprinting Functionality Added Parameterized Sprinting and Headbob Functionality Added Sep 3, 2022
Setting movementDeceleration and movementAccelaration both to 1 would remove acceleration.
@Tokey Tokey changed the title Parameterized Sprinting and Headbob Functionality Added Parameterized Sprinting, Headbob Functionality & Movement Acceleration Added Sep 3, 2022
Copy link
Contributor

@bboudaoud-nv bboudaoud-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking this is a nice solid improvement to FPSci player movement mechanics. It would be interesting to see how something like head bob might impact moving player aim strategies.

I would like to see more comments incorporated into the PlayerEntity::updateFromInput() code explaining what various conditions/lerp() calls are handling. The primary bug I would like to see fixed is the dependence of various parameters on the call rate of PlayerEntity::updateFromInput() rather that the simulation time delta, which should be used instead. Along these lines the positional update would preferably occur in PlayerEntity::onSimulation() where you have access to the sdt value.

I also think we should align on default values that maintain the historical operation of FPSci so head bob/sprint off (always the preferred approach), there are several ways to do this..


if (!m_headBobPolarity)
{
m_headBobCurrentHeight = lerp(m_headBobCurrentHeight, -*headBobAmplitude * 4.0f, *headBobFrequency * m_acceleratedVelocity / 200);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not entirely clear how this is working to me, seems like a sort of triangle wave approach to head bob, which is scaling based on more than the head bob amplitude? I would (naively) assume head bob amplitude was the raw peak/center-to-peak value of the bob.

Anything could be fine here if its documented, but if "magic numbers" like 4 need to be hard coded here there should be extensive comments explaining why they are present. Same goes for the 200 value at the end of the lerp() call.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments and tied the lerp to deltaTime.

m_linearVector = m_acceleratedVelocity * m_lastDirection;
}

m_headBobCurrentHeight = lerp(m_headBobCurrentHeight, 0.0f, 0.1f);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again this fixed lerp will likely be dependent on frame rate (# of times this function is called per second). We should work to make this more uniform by using a time-based metric for interpolation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@bboudaoud-nv
Copy link
Contributor

A few additional ideas while we are working on this branch:

  1. You may want to add a crouchMultiplier or crouchSpeed to allow designers to reduce player speed when crouched
  2. It may also make sense to add a sprintWhileCrouched option to disable sprinting when crouched (this would be the "normal" game dynamic, so default to False?)
  3. I'd be interested in comparing the idea of sprint multipler to a constant specified sprintSpeed in m/s (I'm realizing moveRate should really be renamed to moveSpeed while I type this, @jspjutNV for input)

@jspjutNV
Copy link
Contributor

jspjutNV commented Sep 7, 2022

  1. You may want to add a crouchMultiplier or crouchSpeed to allow designers to reduce player speed when crouched

I agree.

  1. It may also make sense to add a sprintWhileCrouched option to disable sprinting when crouched (this would be the "normal" game dynamic, so default to False?)

I believe the normal way it works if you press sprint while crouched is that the crouch is cancelled at least and some games switch to walking speed while others switch to running. I personally lean toward "do what I said most recently" and would switch to running speed, but I can see either making sense. The uncommon thing would be to just keep crouching after pressing sprint. Though now that I think of it, the games I'm thinking about are all toggle to crouch. I'm not sure how to break the tie if both crouch and sprint are pressed at the same time with a hold to crouch and hold to sprint function.

  1. I'd be interested in comparing the idea of sprint multipler to a constant specified sprintSpeed in m/s (I'm realizing moveRate should really be renamed to moveSpeed while I type this, @jspjutNV for input)

I can see reasons why someone might want to set speeds directly, or might want to do multipliers on top of a base speed. Using something like baseMoveSpeed with runSpeedMultiplier, sprintSpeedMultiplier, and crouchSpeedMultiplier (maybe even proneSpeedMultiplier?) would give you the chance to assign scale factors and globally change all of the speeds with 1 value. Of course someone might dial in a particular speed they want like sprint and then want to dial in the others, but the coupled design would make that more difficult. I don't have a good feeling for which method of design is more likely for experiments, so I'm okay with either.

I agree with the renaming of moveRate, and maybe it should even become moveSpeedmps to try to get "m/s" in there, but I'm not sure those units are totally clear in the name. In general, it's nice to have units in the variable name when there are units to make it easier for people to know what they're setting without going back to the docs every time.

… in DeltaTime into consideration.

Delta-time multiplied with the parameters of lerp function, now the effect of "Acceleration/Deceleration" and "HeadBob" will look/feel similar irrespective of FPS.

if (headBobEnabled != nullptr && *headBobEnabled) {
if (!m_headBobPolarity) {
m_headBobCurrentHeight = lerp(m_headBobCurrentHeight, -*headBobAmplitude, *headBobFrequency * m_acceleratedVelocity * deltaTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, it is not clear to me how this lerp() is working. I assume the theory is that it would scale with number of "footsteps per meter" which in turn scales with speed (higher speed ==> fewer steps per meter) but I'd like to either see more comments here explaining this or something introduced in the documentation describing how it relates.

Ideally you'd be able to set the head bob in a fixed unit that relates to speed (say bobs/(m/s)) and have this code be more deterministic, but for head bob I'm okay with a more fluid approach. Still I'm not sure this does what we want, more displacement (faster speed/longer sdt) implies stronger interpolation of the bob here where my intuition would be the opposite... Maybe I am confused.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The amplitude is fixed (It will not go higher than what we set). The frequency of the head-bob will change based on players' velocity. So the faster the player moves, the faster his "head/camera" will oscillate. I have updated it with more comments. Let me know if I need to add a few more comments, break it down even further, or change the whole system all-together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to point out that the statement "it will not go higher than what we set" isn't necessarily true here. Again lerp(a,b,c) works using a + (b-a)*c so if c is >1 it will produce a value larger than b (in this case the headBobAmplitude).

Since your c (interpolation) term is headBobFrequency * acceleratedVelocity * deltaTime its hard to say this quantity will always be <1. I'd prefer to move to a model here where we can safely guarantee this behavior across all possible values of headBobFrequency and acceleratedVelocity if possible.

@bboudaoud-nv
Copy link
Contributor

@jspjutNV I'm not sure what the .vs/.idea files in this branch are, should we remove them or are they something new to VS 2022?

@bboudaoud-nv bboudaoud-nv self-requested a review September 9, 2022 16:42
Copy link
Contributor

@bboudaoud-nv bboudaoud-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is moving in a positive direction, but there are still a number of small fixes, documentation updates, and minor functionality changes/comments that need to be added before it will be merged.

@bboudaoud-nv bboudaoud-nv added the enhancement New feature or request label Sep 9, 2022
@jspjutNV
Copy link
Contributor

jspjutNV commented Sep 9, 2022

@jspjutNV I'm not sure what the .vs/.idea files in this branch are, should we remove them or are they something new to VS 2022?

I agree, these files don't seem needed. @Tokey , could you remove them from the PR? Basically we don't want anything in the vs/.idea/.idea.FirstPersonScience/.idea/ or similar paths to come with this pull request.

…ed comments and updated general_config.md file.
@Tokey
Copy link
Author

Tokey commented Sep 9, 2022

The .idea files came when I opened the project with Rider. I was having issues with VS 2019, and had to re-install G3D and VS 2019 to get it to working again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants