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 Camera2D jitter #35635

Closed
wants to merge 1 commit into from
Closed

Conversation

forbjok
Copy link

@forbjok forbjok commented Jan 27, 2020

Fix #35606 by rounding Camera2D transform origin to the nearest whole pixel before setting canvas transform.

Bugsquad edit: fixes #2667

@Calinou
Copy link
Member

Calinou commented Jan 27, 2020

Thanks for opening a pull request 🙂

I believe this should only be done if pixel snap is enabled. In a non-pixel art game, you generally want the motion to look as smooth as possible (even if it ends up being a little blurry).

@forbjok
Copy link
Author

forbjok commented Jan 27, 2020

Changed it to only round if Pixel Snap is enabled.

@fossegutten
Copy link
Contributor

fossegutten commented Jan 28, 2020

Great addition! Would love to have this for 3.2!

@Calinou what is the downside to having this enabled for all projects? I think the jitter is bad in all projects, just more noticeable in lower resolution games. The monitor can never draw sub pixels anyway, so I don't see the reason it will be more smooth? If anything, I think this will look more smooth for all games. Please correct me if I am wrong

@Calinou
Copy link
Member

Calinou commented Jan 28, 2020

@fossegutten I don't know exactly, I was just guessing. I apologize for the confusion.

@forbjok Feel free to edit the pull request so the coordinate snapping always applies.

@forbjok forbjok force-pushed the fix-camera2d-jitter branch from b986dc1 to e3cf965 Compare January 28, 2020 21:37
@forbjok
Copy link
Author

forbjok commented Jan 28, 2020

Ok, removed the Pixel Snap check again, so now it should be back to always being applied.

@forbjok forbjok force-pushed the fix-camera2d-jitter branch from e3cf965 to 33515a6 Compare January 28, 2020 21:56
@akien-mga akien-mga added this to the 4.0 milestone Jan 28, 2020
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 28, 2020
@Seel
Copy link

Seel commented Jan 29, 2020

I agree with snapping the camera always. It shouldn't be a problem for overall smoothness as long as the original coordinates are still being tracked for the motion of the camera, right?

I actually think blurriness is a problem for non-pixelart games in general, and It's a bit under discussed.

@fossegutten
Copy link
Contributor

I agree with snapping the camera always. It shouldn't be a problem for overall smoothness as long as the original coordinates are still being tracked for the motion of the camera, right?

I actually think blurriness is a problem for non-pixelart games in general, and It's a bit under discussed.

My C++ is not the best, but I think the camera Transform2D is copied, then edited. In this case, it should not interfere with the Camera2D node position.

@LuisDiaz3E8
Copy link

I was looking for a long time for a solution to this problem.

@eon-s
Copy link
Contributor

eon-s commented Jan 30, 2020

I always had problems when using round to solve this kind of issues, floor use to be more consistent (but adds extra complexity in some configurations, like on really slow motions).
I think this should be tested in different configurations, on high and low resolution scenes, also on rotating and scaling cameras and situations of cameras inside moving nodes (probably comparing the global transform).

@Seel
Copy link

Seel commented Jan 30, 2020

At the moment I have my camera setup to be exactly on top of the player, that way the player sprite should stay perfectly centered on the screen when moving around the map. If I use round it works, but if I use floor the player sprite jitters around, because the camera is 1 pixel behind the player half of the time.

I currently use gdscript to do this, but I have to use force_update_scroll() to achieve a perfect result.

@fossegutten
Copy link
Contributor

fossegutten commented Jan 30, 2020

I also got better results with round, compared to floor. Using a workaround GDscript.

@akien-mga
Copy link
Member

This needs testing with different zooming levels, margins and offsets to confirm that it always performs as expected. But a priori this seems like a good solution for the issue :)

@LuisDiaz3E8
Copy link

I have compiled your Camere2d code and tested myself I can tell the jittering problem is gone, but I ran into another problem related with round to the nearest position nad it´s the camera smoothing is too sharp, it doesn't seem right.
my project has a resolution of 324*120 perhaps with higher resolution the problem disappears, however, I haven't tested yet.

@Seel
Copy link

Seel commented Feb 4, 2020

I have compiled your Camere2d code and tested myself I can tell the jittering problem is gone, but I ran into another problem related with round to the nearest position nad it´s the camera smoothing is too sharp, it doesn't seem right.
my project has a resolution of 324*120 perhaps with higher resolution the problem disappears, however, I haven't tested yet.

Yeah... I don't think smoothing will ever work at such low resolutions unless you render at a higher resolution internally, or use a (sub)pixel shader, where you scale the whole image up (5 to 6 times in your case) and then smoothly interpolate using the shader.

Like so: https://code-disaster.com/2016/02/subpixel-perfect-smooth-scrolling.html

But even in this case the camera should always snap to whole pixels on the grid to avoid rendering artifacts, then the offset will be applied with the shader. You could skip the subpixel step if you want.

Side by side comparison: https://code-disaster.com/gdx/gdx-mellow-demo-2/

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

This seems fine. I can't think of a concrete reason why you would ever want the canvas transform to have sub-pixel units.

I'm still a little concerned for non-pixel art games. To me it makes sense that the camera transform could be off pixel units because your sprite may have non pixel units. For example, imagine you have a sprite with an X-coordinate of 10.5 and a Camera at 0.5. Rightly, your sprite would appear exactly at the 10th pixel on screen. However, with this change, the sprite would appear at 10.5, which could cause issues. To me, this indicates that this should only take place if pixel_snap is enabled.

I know this has gone back and forth, but what is the use case for enabling this while pixel snap is disabled?

@fossegutten
Copy link
Contributor

fossegutten commented Feb 7, 2020

Someone should probably test it in a high resolution game, with smoothing enabled ( because it makes the jitter easier to spot). I see your point, however i think that this is the problem that actually exists in the camera now, that this issue is fixing.

Imagine sprite1 at position 10.4 and sprite2 at position 20.6. If camera is at position 15.0, these will be drawn at perfectly at position 10.0 and 21.0. However, If camera moves to position 15.3, sprite 1 will draw at position 11.0 and sprite2 will still draw at position 21.0. This is the whole problem, and those two sprites will appear to jump 1 pixel back and forth.
If we always round the camera rect, this back and forth jumping will never happen.

@clayjohn
Copy link
Member

clayjohn commented Feb 7, 2020

If we always round the camera rect, this back and forth jumping will never happen.

Sure it will. It will just happen when the sprites move, not if the camera moves. And we solved that problem by using pixel_snap.

@forbjok
Copy link
Author

forbjok commented Feb 7, 2020

If we always round the camera rect, this back and forth jumping will never happen.

Sure it will. It will just happen when the sprites move, not if the camera moves. And we solved that problem by using pixel_snap.

You can still get other forms of visual jitter, for example between the camera and the sprite it's following (I made some changes to my gdscript workaround to fix this a few days ago), but with the canvas transform being rounded, you should never get any inconsistencies between different sprites as the camera moves, which is what this issue is about.

@clayjohn
Copy link
Member

clayjohn commented Feb 7, 2020

You can still get other forms of visual jitter, for example between the camera and the sprite it's following (I made some changes to my gdscript workaround to fix this a few days ago), but with the canvas transform being rounded, you should never get any inconsistencies between different sprites as the camera moves, which is what this issue is about.

I agree. The question is, whether this should be enabled with pixel_snap, or be always on. Pixel_snap removes jitter from sprite positions, but its an optional toggle. Similarly, this fixes jitter caused by Camera positions, why should it be treated differently?

@forbjok
Copy link
Author

forbjok commented Feb 8, 2020

The question is, whether this should be enabled with pixel_snap, or be always on.

Is there any downside at all to always doing it? It's not like it's possible to render subpixels, and even if the jitter is less noticeable with higher resolutions, it's still not IMO correct or desirable behavior.

@Seel
Copy link

Seel commented Feb 11, 2020

Alright, I tested this a bunch and I now think it should be a separate option, not just tacked on to pixel snap.

  • I tested a "high res" 2D game with texture filtering on and it is a trade off there. It is indeed a bit smoother without snapping, but instead of jittering you get shimmering, which isn't as bad. Snapping the camera makes it slightly less smooth, but you don't have that shimmering effect. You also have a lot more pixels to work with in a HD 2D game, so it's not like it feels choppy or anything.

  • On a pixel graphics game this commit works very nicely and I didn't even need to turn pixel snap on, making pixel snap feel almost unnecessary to me.

  • If you have a rotating camera it might be better to leave it off, but it didn't feel too bad with it on either.

Personally I'd want to use this, even in a high res 2D game, but without having to turn on pixel snap to do so. It leads to a more consistent look imo.

I'd 100% use it in a pixel art game of course, but as I said, I'm not sure pixel snap is needed if you use this and unfiltered textures.

I almost feel like this new camera snap option should be on by default, as most of Godot's 2D use cases will probably benefit.

@ghost
Copy link

ghost commented Apr 14, 2020

Moving sprites with pixel art results in some undesired visual artifacts (even more if smoothing is enabled).
The artifacts are more noticeable when moving diagonally, but also occurs when moving in one axis.

@fossegutten
Copy link
Contributor

fossegutten commented Apr 27, 2020

Definately needs to be optional. Figured out that when using zoom on the 2D camera, this snapping is undesired.
Edit: I am now snapping the camera to the "zoom" property, instead of rounding the position. Seems to work better for my use case

@MarcusElg
Copy link
Contributor

@forbjok please continue on this! It would be very useful to have

@lawnjelly
Copy link
Member

lawnjelly commented Jun 14, 2020

Presumably if you want to do any snapping to the camera, it might be better to do it on creation of the view matrix, not by setting the origin.

The reason being, that even a pixel camera might be required to smoothly move by sub pixel amounts. Only the visual representation needs to be snapped. Otherwise you would need to maintain the float position outside of the xform.origin.

Although the camera snapping is tempting, I'm still not convinced it is the best solution. Once you start zooming, scaling, rotating, or pretty much anything that takes it out of that 1:1 pixel in editor to pixel on screen relationship, the whole system falls apart (as people have found above). It has the potential to work for some subset of games but is not a general solution.

Optional snapping to the scene might solve these problems, or perhaps to the canvas layer? You could then even have different parallax layers snapping within themselves, but with sub pixel offset (consider the effect when scaled).

@Giwayume
Copy link
Contributor

Presumably if you want to do any snapping to the camera, it might be better to do it on creation of the view matrix, not by setting the origin. The reason being, that even a pixel camera might be required to smoothly move by sub pixel amounts. Only the visual representation needs to be snapped

Absolutely this. Even if you're snapping to pixels, the camera smoothing can look jerky you don't let the lerp algorithm reach sub-pixels where you're moving less than 1 pixel per frame.

Is there any downside at all to always doing it? It's not like it's possible to render subpixels, and even if the jitter is less noticeable with higher resolutions, it's still not IMO correct or desirable behavior.

In terms of the HTML5 canvas, if you render at a half-coordinate like [0, 0.5] you will see 2 light gray pixels next to each other rather than a single black pixel like if you render at [0, 0]. There is definitely a sub-pixel smoothing effect that can be useful for low resolution games that aren't trying to be pixel-perfect.

@skullleeep
Copy link

Sorry I am kinda new so how do I use it??

@Calinou
Copy link
Member

Calinou commented Sep 19, 2020

@skullleeep This pull request hasn't been merged yet, so you'd have to compile the engine from source with the pull request applied. However, since this pull request is based on the master branch, you'd effectively be compiling an unstable pre-release 4.0 version, which you most likely don't want. (We recommend using Godot 3.2.x in a production environment.)

All in all, it might not be trivial to use this until the pull request is merged and a new release is made. We don't know for certain if this pull request will be merged still (as always). #41535 may also fix this in a more robust way.

@skullleeep
Copy link

@skullleeep This pull request hasn't been merged yet, so you'd have to compile the engine from source with the pull request applied. However, since this pull request is based on the master branch, you'd effectively be compiling an unstable pre-release 4.0 version, which you most likely don't want. (We recommend using Godot 3.2.x in a production environment.)
All in all, it might not be trivial to use this until the pull request is merged and a new release is made. We don't know for certain if this pull request will be merged still (as always). #41535 may also fix this in a more robust way.

OK thanks

@Calinou
Copy link
Member

Calinou commented Mar 20, 2021

@lawnjelly Now that dust has settled around 2D smoothing (for now), is this pull request still relevant?

@lawnjelly
Copy link
Member

lawnjelly commented Mar 20, 2021

@lawnjelly Now that dust has settled around 2D smoothing (for now), is this pull request still relevant?

I think it has been superseded by #43194 (which did camera snapping as the viewport / canvas). Although these approaches have worked in a few test cases, the whole area of transform snapping has proved problematic for users in practice (with trials through the 3.2.4 betas).

Personally, I'm now of the opinion that we are better off implementing this as an addon / tutorial, at least for now. Usually one of the barriers to putting something in core is that it cannot be worked around with a few lines of gdscript, and this can (maybe with a bit of node jiggling in places, but nothing major). More importantly - it is not clear that the approach used in these PRs is the best one, or highly applicable. The best approach may be game dependent.

So rather than commit to supporting a particular approach at this stage imo it would be better to produce an addon or tutorial, battle test the method for some time, then consider if any aspects can be added to core. There are parallels with the smoothing addon, which first met with scepticism, but after being battle tested and proving popular there is now agreement to have integrated in 4.x in some form in core.

@lawnjelly lawnjelly closed this Mar 20, 2021
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet