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

spring dampened camera #1589

Closed

Conversation

loveridge
Copy link
Collaborator

@loveridge loveridge commented Jul 9, 2024

Adds a new option for spring dampened camera transitions.

https://discord.com/channels/549281623154229250/724924957074915358/1260277856370561145

About a microsecond faster than the old transition mode:
image
image

rts/Game/CameraHandler.cpp Outdated Show resolved Hide resolved
rts/System/SpringDampers.cpp Outdated Show resolved Hide resolved

float spring_damper_damping(float halflife)
{
return halflife_to_damping(halflife) / 2.0f;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return halflife_to_damping(halflife) / 2.0f;
static constepxr float why_halve? = 0.5f;
return halflife_to_damping(halflife) * well_named_constant;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These constants are all related to the derivation of stiffness and damping inputs to a single halflife value. The math is a bit too complicated for me so you'll have to read the guy's blogpost if you want more info. But my take away from it is that we can have a single input value that gives it a more or less "springiness".

Comment on lines 437 to 439

timed_spring_damper_exact_vector(currentPos, camTransState.posVelocity, camTransState.startPos,
targetPos, camTransState.timeEnd, camTransState.halflife, damping, eydt, dt, 2.0f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
timed_spring_damper_exact_vector(currentPos, camTransState.posVelocity, camTransState.startPos,
targetPos, camTransState.timeEnd, camTransState.halflife, damping, eydt, dt, 2.0f);
static constexpr float APPREHENSION = 2.0f; // value chosen on the basis of XYZ
timed_spring_damper_exact_vector(currentPos, camTransState.posVelocity, camTransState.startPos,
targetPos, camTransState.timeEnd, camTransState.halflife, damping, eydt, dt, APPREHENSION);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is what the guy writes about apprehension:

apprehension parameter controls how far into the future we try to track the linear interpolation. A value of 2 means two-times the half life, or that we expect the blend-out to be 75% done by the goal time.

rts/Game/CameraHandler.cpp Outdated Show resolved Hide resolved
@sprunk
Copy link
Collaborator

sprunk commented Jul 10, 2024

The spring damping demo linked as a source shows that it overshoots (0:05s and onwards). Large discrete movements happen often for cameras (move by clicking minimap, selecting important units like ctrl+c that focus the unit, etc.) so this specific behaviour of the spring-damping mode sounds quite undesirable

@loveridge
Copy link
Collaborator Author

The spring damping demo linked as a source shows that it overshoots (0:05s and onwards). Large discrete movements happen often for cameras (move by clicking minimap, selecting important units like ctrl+c that focus the unit, etc.) so this specific behaviour of the spring-damping mode sounds quite undesirable

These are specifically the spring functions for a critically dampened spring that should approach the goal without additional oscillations: https://theorangeduck.com/page/spring-roll-call#critical

@@ -20,7 +20,7 @@ CONFIG(float, FPSFOV).defaultValue(45.0f);
CONFIG(bool, FPSClampPos).defaultValue(true);


CFPSController::CFPSController(): oldHeight(300.0f)
CFPSController::CFPSController(): oldHeight(300.0f), rot(2.677f, 0.0f, 0.0f)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's 2.677?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same default as spring camera.

Comment on lines +67 to +68
rot.y += mouseScale * move.x;
rot.x = std::clamp(rot.x + mouseScale * move.y * move.z, 0.01f, math::PI * 0.99f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

so what's the difference between rot and dir?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

rot is the rotation vector, encoding pitch, yaw and roll. dir is the 3d direction vector.

@@ -19,16 +19,19 @@ class CRotOverheadController : public CCameraController
void MouseWheelMove(float move, const float3& newDir) { MouseWheelMove(move); }

void SetPos(const float3& newPos);
void SetRot(const float3& newRot) { rot = newRot; };
Copy link
Collaborator

@sprunk sprunk Jul 13, 2024

Choose a reason for hiding this comment

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

Does this (and the same for the Spring camera below) mean that passing rot with a non-zero z component nets you a rolled (potentially upside-down etc) camera? Since previously GetRotFromDir always ensured zero z:

r.z = 0.0f;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this allows rotatable overhead camera to roll if the newRot has a z component. Unfortunately there is no mechanism to set this in practice, should I add a config value to enable a modifier to roll the camera? Perhaps MOVE_STATE_TLT. And/or set it with SetCameraState.

As for switching to other cameras, the more constrained cameras manage their constraints to prevent rolls. Spring camera does it with float3 GetRot() const { return (float3(rot.x, GetAzimuth(), 0.0f)); }, so the current rot.z has no effect. The rest of the cameras don't override the CameraController SetRot, so they just set the dir and therefore cannot roll. Except for overview camera, which throws out the x and z components.

But in general, if a camera is expected to operate under certain constraints, then it should manage those constraints internally. For example, as overview camera does by throwing out or otherwise constraining the values. And we do see this all over, such as spring camera clamping the rot.x to the 90 degree range of .5pi to pi.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately camera rolling breaks a number of other things, so I won't be adding any way for the rotatable overhead camera to be rolled from gameside.

@@ -379,6 +569,7 @@ void CCameraHandler::SetCameraMode(unsigned int newMode)
CCameraController* newCamCtrl = camControllers[currCamCtrlNum = newMode];

newCamCtrl->SetPos(oldCamCtrl->SwitchFrom());
newCamCtrl->SetRot(oldCamCtrl->GetRot());
Copy link
Collaborator

Choose a reason for hiding this comment

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

See previous comment, is this a vector to pass a previously-impossible rotation by switching from a less constrained camera to a more constained one?

@sprunk
Copy link
Collaborator

sprunk commented Jul 14, 2024

The PR is now big and scary so I've extracted an uncontroversial part #1598

@sprunk
Copy link
Collaborator

sprunk commented Jul 18, 2024

If the rot refactor involves no logic change on its own then it could also be split off to a separate PR for easier review/merge.

@lhog
Copy link
Collaborator

lhog commented Oct 6, 2024

Does this look merge-able @sprunk @loveridge ?

@sprunk
Copy link
Collaborator

sprunk commented Oct 6, 2024

It looks fine and I didn't find any obvious code problems, I was just too scared to give a full "Approved" mark.

@loveridge loveridge mentioned this pull request Oct 6, 2024
@loveridge
Copy link
Collaborator Author

Does this look merge-able @sprunk @loveridge ?

I'd be happy to get it in

@loveridge loveridge closed this Nov 2, 2024
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.

3 participants