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 flicker in control nodes due to pivot offset #46409

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

realkotob
Copy link
Contributor

@realkotob realkotob commented Feb 25, 2021

Fixes #28804 for 3.x
Closes #36974

Based on suggested fix from this comment, which is already implemented on the master branch anyway.
#36087 (comment)

We're using this internally for our project and it doesn't seem to have any regressions anywhere, so it is safe to merge.

Before:

2021-02-25.12-31-49-989.mp4

After:

2021-02-25.12-24-43-812.mp4

@realkotob realkotob requested a review from a team as a code owner February 25, 2021 09:42
@realkotob
Copy link
Contributor Author

Here's an image of the Editor Settings to show that the regression with VScroll does not happen anymore.
image

@akien-mga
Copy link
Member

Please amend the commit message to be more descriptive. Mentioning the issue number is good but it's not sufficient information to know what a commit does when reading a changelog. See https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind

It would also be good to credit @georgwacker for the original fix by adding them to the commit message as Co-authored-by:. Should be something like:

Co-authored-by: Georg Wacker <contact@georgwacker.com>

at the end of the commit message.

@realkotob
Copy link
Contributor Author

@akien-mga Thank you for your feedback, I modified the commit message.

@Calinou
Copy link
Member

Calinou commented Feb 26, 2021

History tells me this PR may not be a good idea, as it may reintroduce a regression in 3.2 at least: realkotob@2bae353

That said, it's been a while since anyone managed to consistently reproduce blurry font issues in the editor.

@realkotob
Copy link
Contributor Author

realkotob commented Feb 26, 2021

@Calinou That is not the case here. If you look at the discussion in this thread: #36087 (comment) you can see that the regression was fixed on master, where the prior commit was not reverted.

This PR simply applies the old fix, plus adds the new fix from master that closes the regression.

@realkotob
Copy link
Contributor Author

My test results are consistent with the results that @georgwacker got, and he did most of the work, which is why he is listed as a co-author.

@georgwacker
Copy link
Contributor

I can confirm that this fix does not cause the old regression in my initial PR, the crucial part is rounding of the coordinates for the scroll container as it is currently in master.

I documented my findings here #36087 (comment) but was not sure how or if I should create a PR for 3.2.

This is based on suggested fix from this comment
godotengine#36087 (comment) -- basically the old rounding workaround is removed, and rounding is now done locally in the scroll_container instead.

Fixes godotengine#28804

Co-authored-by: Georg Wacker <contact@georgwacker.com>
@akien-mga akien-mga merged commit 9ee835a into godotengine:3.2 Mar 2, 2021
@akien-mga
Copy link
Member

Thanks!

@realkotob
Copy link
Contributor Author

This PR is being reverted due to a clipping regression. For now, if you want to animate Control nodes, turn off snap_controls_to_pixels in Project Settings (under GUI/Common).

@akien-mga
Copy link
Member

Yes as discussed on the Godot Chat, we need to dig deeper to better understand how this snapping is supposed to work and make a solution that works reliably.

This PR actually removed the last use of gui/common/snap_controls_to_pixels, so the reason for having had this setting in the first place needs to be reviewed. It was added by @reduz in 1eeda0f to fix various bugs, but the actual code using it from that commit has since been removed by @Piet-G in #22431, which is the commit adding the workaround that this PR removes.

There's definitely stuff to fix there, so I fully acknowledge the bug and the removal of this workaround is likely a good option, but we need to take the time to test things thoroughly to come up with a reliable fix that won't cause regressions. So reverting this PR for now given how close we are to the 3.2.4 release - it's better to keep the current known issues (which can be worked around by disable gui/common/snap_controls_to_pixels) than trade them for new ones.

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.

None yet

5 participants