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

Remove 2D transform snapping to rely instead on the GPU #87058

Closed

Conversation

adamscott
Copy link
Member

@adamscott adamscott commented Jan 10, 2024

More or less applies #46657 to the master branch (4.x). It fixed issues on 3.x, but wasn't forward-ported in 4.x.

This PR removes the need of rounding brought by #84380. As the GPU is doing the rounding, we don't have to floor the transform values on the DisplayServer side.

Comparison

Merlin: Scale of the Magic

Using the open-source pixel perfect game Merlin: Scale of the Magic by @nicolasbize.

w/o this PR

GodotRoundVsFloor-floor.mp4

w/ this PR

fixed.mp4

#56793

w/o this PR

image

w/ this PR

image

Relates to

Supersedes #84380.
Fixes #56793.
Fixes #84632.

@adamscott adamscott added topic:rendering topic:2d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 10, 2024
@adamscott adamscott added this to the 4.3 milestone Jan 10, 2024
@adamscott adamscott requested a review from a team as a code owner January 10, 2024 20:23
@markdibarry
Copy link
Contributor

markdibarry commented Jan 10, 2024

Seeing a lot of issues using the pixel perfect test project provided in #84380. I see similar jitter and scaling issues in the Merlin video provided above.

master branch:

Master.mp4

#84380:

Rounding.mp4

This PR:

Remove.mp4

@adamscott
Copy link
Member Author

@markdibarry @KeyboardDanni I'll try to port PixelPerfectTest.zip to Godot 3.x, if there's issues there too.

@adamscott adamscott marked this pull request as draft January 10, 2024 21:50
@KeyboardDanni
Copy link
Contributor

I looked at your commits and it looks like it's just removing the snapping that's applied to all 2D transforms (leaving in the snapping that's done on Sprites). Feels like something is missing here. I would expect changes in the shader code as well if we are to rely on GPU snapping. My understanding is that the GPU snap was only done on the 3.x branch.

@clayjohn
Copy link
Member

I looked at your commits and it looks like it's just removing the snapping that's applied to all 2D transforms (leaving in the snapping that's done on Sprites). Feels like something is missing here. I would expect changes in the shader code as well if we are to rely on GPU snapping. My understanding is that the GPU snap was only done on the 3.x branch.

GPU snap is here and its implemented the same as it was in 3.x

if (canvas_data.use_pixel_snap) {
vertex = floor(vertex + 0.5);
// precision issue on some hardware creates artifacts within texture
// offset uv by a small amount to avoid
uv += 1e-5;
}

The difference is that 3.x removed the transform snapping which is what this PR does.

@KeyboardDanni
Copy link
Contributor

That particular code doesn't seem to be doing much snapping, unless something got configured wrong. The third video looks functionally identical to disabling snapping (with the exception of Sprites which are still being snapped). Which is why it seems like something is missing here.

@adamscott
Copy link
Member Author

adamscott commented Jan 11, 2024

@clayjohn @KeyboardDanni We were doing this PR thinking that Godot 3.x was doing fine. But I ported your PixelPerfectTest.zip test to Godot 3.x (PixelPerfectTest3.zip). And it seems that Godot 3.x code is failing your test too, @KeyboardDanni.

PixelPerfectTest3.mp4

@KeyboardDanni
Copy link
Contributor

Looking at this, I'm wondering if the camera isn't getting snapped to pixel. It looks like the vertices of individual 2D elements are snapped. But you're still going to get jitter if the camera isn't being snapped as well. Technically you could work around this on the script side, but I feel like it'd be better to just have this happen in the shader as part of the vertex snap, so that everything would "just work".

I also feel that the way the snap options are presented in settings is a bit confusing. Users might think that you'd need to enable both snap settings (transform and vertex), when really you should only be using one, as having both enabled might cause unforeseen issues.

@adamscott
Copy link
Member Author

Looking at this, I'm wondering if the camera isn't getting snapped to pixel. It looks like the vertices of individual 2D elements are snapped. But you're still going to get jitter if the camera isn't being snapped as well. Technically you could work around this on the script side, but I feel like it'd be better to just have this happen in the shader as part of the vertex snap, so that everything would "just work".

Oh my, I didn't enable the 2D snapping in my Godot 3.x test. I'll capture stuff and come back with a video.

@KeyboardDanni
Copy link
Contributor

I'd also like to add that if we're going to remove 2D transform snapping, it should be done thoroughly. Remove the project setting, transform snap variables, etc. Replacing the transform snapping mode with a no-op would only make things more confusing for users.

@KeyboardDanni
Copy link
Contributor

cc @lawnjelly since they've worked on this stuff for 3.x and might have some insight.

@adamscott
Copy link
Member Author

Here's Godot 3.5.2 running PixelPerfectTest3 using GPU snap (failing the test too).

PixelPerfectTest3-snapped.mp4

@adamscott
Copy link
Member Author

adamscott commented Jan 11, 2024

@KeyboardDanni @markdibarry I've sent you private chat messages on https://chat.godotengine.org/. I'm currently organizing a meeting next week to talk about pixel perfect rendering issues. I think you'd be welcome to join and share your insight with us.

@akien-mga
Copy link
Member

Superseded by #87297.

@akien-mga akien-mga closed this Feb 12, 2024
@akien-mga akien-mga added bug archived and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 12, 2024
@AThousandShips AThousandShips removed this from the 4.3 milestone Feb 13, 2024
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.

Sprite "crunch" when placed in certain positions on screen Vulkan: 2D Pixel Snap not working
6 participants