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

Move frame prediction logic for camera from Sector to Camera #3173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mstoeckl
Copy link
Contributor

This commit avoids interpolating the camera position when a Camera::move or other discontinuous transition is requested.

Note: jumps in the camera position that are purely driven by changes to Tux's position will continue to be interpolated. (For example, when Tux.set_pos() is called in bonus1/mario.stl.)

@mstoeckl
Copy link
Contributor Author

mstoeckl commented Jan 12, 2025

I forgot to say why I am making this PR now: I spent a few hours today trying to figure out how to, when the frame prediction option is enabled, change the Camera logic to extrapolate the camera position instead of interpolating with between 0 and 1 frames of delay. (Done right, this should make the jump cuts when Tux goes through pipes in bonus1/mario.stl work a bit more smoothly than before, and maybe also help a bit when there is sudden acceleration.) However, my experiments suggest that doing this properly requires specific logic for each individual camera mode, depending on what the camera is tracking. A universal solution of just linearly extrapolating instead of interpolating based on the last camera position introduces overshoot which can look weird. While I did not make much progress with the camera extrapolation logic and refactoring, I am now certain that the camera interpolation logic is much easier to work with when it is in the Camera class and not in Sector :-). Since doing this has an immediate benefit, and I can't be certain that I'll finish the camera extrapolation change any time soon, I have made this PR now.

@tobbi tobbi requested review from Vankata453 and MatusGuy January 12, 2025 02:40
@tobbi
Copy link
Member

tobbi commented Jan 12, 2025

I tested this briefly. I think with this change it looks even smoother.

@mstoeckl
Copy link
Contributor Author

mstoeckl commented Jan 12, 2025

I tested this briefly. I think with this change it looks even smoother.

Err.... when making this change I forgot to, as the code had done before, disable the camera interpolation when frame prediction is not used. (I've updated the commit to fix this.) The camera interpolation logic adds up to a frame of lag to the camera, so it's possible that you are just noticing the accidentally increased camera inaccuracy when frame prediction was off. This change should not affect how the frame prediction option works, except in the few rare cases where e.g. Camera.set_pos/Camera.set_scale are used in scripts.

Edit: have pushed second update to adjust formatting.

Comment on lines 221 to 234
Vector
Camera::get_predicted_translation(float time_offset) const
{
if (g_config->frame_prediction) {
// TODO: extrapolate forwards instead of interpolating over the last frame
float x = time_offset * LOGICAL_FPS;
return get_translation() * x + (1 - x) * m_last_translation;
} else {
return get_translation();
}
}

float
Camera::get_predicted_scale(float time_offset) const {
if (g_config->frame_prediction) {
// TODO: extrapolate forwards instead of interpolating over the last frame
float x = time_offset * LOGICAL_FPS;
return get_current_scale() * x + (1 - x) * m_last_scale;
} else {
return get_current_scale();
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

To reduce repetition, I think it would be cleaner if you made another (static/anon namespaced) function taking the appropriate function and m_last_... variable, as later when someone completes the TODO it will require less refactoring. What do you think?

Also use 1.f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think?

I'm not entirely sure what you mean, but agree that some of the logic can be deduplicated. I've merged get_predicted_scale and get_predicted_translation into a single function (get_predicted_transform) that operates on and returns the scale+transform together. (I'm using a std::pair to return them since the function is only called by once, by Sector; another option would be to define a struct for the return type.)

Also use 1.f

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pardon me, I thought the code for both functions returned Vector. Whoops. But yes, this solution is easier to maintain later, so thank you for achieving it. :)

This makes it possible to avoid interpolating the camera position
when a Camera::move or other discontinuous transition is requested.
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