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

Project/Editor Settings contents displayed weirdly depending on the scroll position (regression from #32959) #36087

Closed
YeldhamDev opened this issue Feb 10, 2020 · 15 comments

Comments

@YeldhamDev
Copy link
Member

Godot version:
78074fe

Issue description:
Depending on the position of scroll, the contents of both Project and Editor Settings dialogs display weirdly. This doesn't happen in 3.2:
Screenshot_20200210_174114

@akien-mga
Copy link
Member

akien-mga commented Feb 11, 2020

Confirmed, I've seen that today too.
It also happens in the Inspector.
Screenshot_20200211_103506

@akien-mga
Copy link
Member

Bisected, that's a regression from #32959 (@georgwacker @groud).
However I don't get the bug in dfb7d46 itself (the original commit of the PR, pre-3.2), so it seems to be the merge which triggers the bug due to an intermediate change.

@georgwacker
Copy link
Contributor

Depending on the position of scroll

What does position of scroll mean exactly, do you have to do something to trigger the glitch?

I don't see this bug in my build of 3e3f8a4, if I just open the project settings for example and scroll in one of the view containers. I also tested this with a fresh Godot profile in Windows.

@groud groud self-assigned this Feb 12, 2020
@akien-mga
Copy link
Member

What does position of scroll mean exactly, do you have to do something to trigger the glitch?

Reduce the size of the Editor Settings dialog to have a long enough scrollable section, and scroll down. At some specific scroll values (likely non-integer ones), the bug is triggered.

@akien-mga
Copy link
Member

Also seems to affect game controls. I think we'd better revert for now (including the cherry pick in 3.2.1 RC 1).

@georgwacker
Copy link
Contributor

I had a hard time reproducing this bug on my machine, I took another look with master and finally saw a similar one pixel line cutoff, but nothing like the above.

It seems we need another solution for control node animation after all.

Looking at the code again, it seems like the control nodes currently rely on the xform[2].round() that gets called by scrolling and thus calling NOTIFICATION_DRAW. This must mean that without the rounding, all the control nodes in the editor somehow have non-integer transforms?
The workaround isn't that old, I wonder which other change made this rounding necessary.

Anyways, until a better solution is found, the old workaround should be restored. Sorry for the regression.

@georgwacker
Copy link
Contributor

image

This is the output of control node positions in the project settings after scrolling and before the rounding. They have integer positions when drawing initially and have fractions after scrolling.

@georgwacker
Copy link
Contributor

I tracked this down to ScrollContainer, where scroll is directly used for the Rect2D to reposition the children.

Rect2 r = Rect2(-scroll, minsize);

Now the question is if we just round the x/y components of scroll before using it as a position for child nodes OR round the scrolling data in gui_input?

void ScrollContainer::_gui_input(const Ref<InputEvent> &p_gui_input) {

@bojidar-bg
Copy link
Contributor

I would suggest rounding the scroll, since the input event value is divided by 8 and multiplied by some OS-defined factor at

v_scroll->set_value(v_scroll->get_value() - v_scroll->get_page() / 8 * mb->get_factor());

@georgwacker
Copy link
Contributor

Your answer could be interpreted both ways :-)

I guess you mean to round all the calculations in gui_input, including the one you linked on Line 104? Since in the context of the ScrollContainer, all control nodes should be spaced with integer numbers, always?

@akien-mga
Copy link
Member

Reverted the cherry-pick from the 3.2 branch for now until we find a fix for the regression. I did not revert from the master branch though so we can still experience and fix the bug there first.

@akien-mga akien-mga changed the title Project/Editor Settings contents displayed weirdly depending on the scroll position Project/Editor Settings contents displayed weirdly depending on the scroll position (regression from #32959) Mar 2, 2020
akien-mga added a commit to akien-mga/godot that referenced this issue Mar 4, 2020
@akien-mga
Copy link
Member

I'm not sure if this is still reproducible, I couldn't see the issue in Project Settings anymore in a recent master build.

There's now a similar issue in popup menus (e.g. Scene menu), but it might be a different cause, as it doesn't seem to be caused by #32959 unlike what was debugged here back in March.

@georgwacker
Copy link
Contributor

I’ll take a look at the original jitter problem and the fix I have for the ScrollContainer. As mentioned above it seems to be a rounding issue.

@georgwacker
Copy link
Contributor

georgwacker commented Feb 2, 2021

In master the workaround

// We use a little workaround to avoid flickering when moving the pivot with _edit_set_pivot()
is removed and
r.position = r.position.floor();
flooring the position fixes the regression when scrolling.

I tested these two changes on the 3.2 branch and they work as expected. As of my original findings, the position based on the scroll value should always be an integer.

Should I create a cherrypick pull request for this?

realkotob added a commit to realkotob/godot that referenced this issue Feb 28, 2021
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>
realkotob added a commit to realkotob/godot that referenced this issue Feb 28, 2021
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>
@georgwacker
Copy link
Contributor

This is fixed with #46409

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

No branches or pull requests

5 participants