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

Fix Parallax2D physics interpolation #91706

Merged
merged 1 commit into from
May 28, 2024

Conversation

rburing
Copy link
Member

@rburing rburing commented May 8, 2024

The camera_screen_center value was stale in case of interpolation, so in that case we compute it using the affine inverse of the interpolated camera transform.

@rburing rburing added this to the 4.3 milestone May 8, 2024
@rburing rburing requested a review from a team as a code owner May 8, 2024 11:07
@rburing

This comment was marked as outdated.

@akien-mga
Copy link
Member

CC @markdibarry

@akien-mga akien-mga requested a review from lawnjelly May 8, 2024 12:29
Copy link
Contributor

@markdibarry markdibarry left a comment

Choose a reason for hiding this comment

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

I did some quick tests, and checked the screen center values while using camera drag with and without physics interpolation. LGTM.

scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
@lawnjelly
Copy link
Member

I'm not clear on what adj_screen_pos should be in the case of camera rotation. Should it be the top left of the rotated screen, or should it be the centre minus half the screen size?

I suspect they can both just derive the adj_screen_pos from the xform, this would make the code a bit simpler and less bug prone in future. The cost of a single xform for each camera doesn't seem worth worrying about if it makes a single path for all cases.

@markdibarry
Copy link
Contributor

markdibarry commented May 9, 2024

@lawnjelly Oh good catch. I didn't test with rotation and totally should have. The original behavior is still correct, but the interpolated version now has different behavior. I used a camera with position and rotation smoothing to show the difference.

Without interpolation:

untitled.mp4

With interpolation:

2024-05-09.08-21-26.mp4

Even worse when you try to do a full 360:

untitled.mp4

@rburing
Copy link
Member Author

rburing commented May 9, 2024

@markdibarry Could you share your test scene as a ZIP file?

@markdibarry
Copy link
Contributor

Here you go!
parallaxmrp.zip

@rburing
Copy link
Member Author

rburing commented May 26, 2024

Thanks for the reviews and the test project! This should work fine now.

@rburing rburing requested a review from markdibarry May 26, 2024 09:16
@markdibarry
Copy link
Contributor

We're getting close! The only problems I found was when I zoomed a camera in or out.

When zoomed out I noticed it was scrolling differently with interpolation on, and eventually broke when it got to the the loop point:
A

Just to preempt the question of why the first image has the looping looking weird too, that's outside the normal bounds of the viewport if the zoom was set to 1. If you were to increase the repeat_times it works correctly, but still doesn't with interpolation:
B

@rburing
Copy link
Member Author

rburing commented May 26, 2024

@markdibarry I don't understand how you got to the situation in the screenshot, please upload the new MRP.

@markdibarry
Copy link
Contributor

@rburing I just set the zoom on the camera to 0.5.

The camera_screen_center value was stale in case of interpolation.
@rburing
Copy link
Member Author

rburing commented May 26, 2024

@markdibarry I made a silly mistake, the PR should be good to go now.

It works because the final lines of Camera2D::get_camera_transform() are:

camera_screen_center = xform.xform(0.5 * screen_size);
return xform.affine_inverse();

Copy link
Contributor

@markdibarry markdibarry left a comment

Choose a reason for hiding this comment

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

Ran through tests for most things I think would be affected. LGTM. Great job!

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

I think this looks as correct as possible, and I guess testing will reveal any problems here.

(It's been a while since I looked at Camera2D, and the whole logic of doing processing in get_camera_transform() has long needed refactoring.)

@akien-mga akien-mga merged commit 1e76e83 into godotengine:master May 28, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Physics 2D Interpolation does not work on Parallax2D.
4 participants