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 bad popups offset in editor with single window off #52280

Conversation

jmb462
Copy link
Contributor

@jmb462 jmb462 commented Aug 31, 2021

Fix #47217

Issue description

When single window is disabled, popup menus in editor pops at a bad offset (see GIFs below)

Fix proposal (Updated after Kobewi comment)

Using get_screen_transform() instead of get_global_transform() to set the popup position to deal with offset in multiwindow mode correctly.

BEFORE

bug_bezier
Bezier Editor bug

bug_script_editor
Script Editor bug

bug_shader
Shader Editor Bug

bug_curve
Curve editor bug

bug_node2
Canvas editor bug

AFTER

fix_bezier
Bezier editor fixed

fix_script_editor
Script editor fixed

fix_script_shader
Shader Editor Fixed

fix_curve
Curve Editor Fixed

fix_node2
Canvas Editor Fixed

@jmb462 jmb462 requested review from a team as code owners August 31, 2021 13:48
@YeldhamDev YeldhamDev added this to the 4.0 milestone Aug 31, 2021
@KoBeWi
Copy link
Member

KoBeWi commented Aug 31, 2021

This won't work if the control is rotated or scaled. It's not a problem in the editor, but e.g. TextEdit can't use this. The problem might lie in get_screen_transform() possibly returning wrong value.

@jmb462 jmb462 force-pushed the fix-bad-popup-offset-with-single-window-off branch from 89d9c6d to 9a0f422 Compare August 31, 2021 15:44
@jmb462
Copy link
Contributor Author

jmb462 commented Aug 31, 2021

@KoBeWi get_screen_transform() is working well.

All buggy popup were using get_global_transform() instead.

I've modified the PR to replace these occurence.
A lot of other popups uses get_screen_transform() and works well.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 31, 2021

The change is good, but it doesn't fix all popups. I'd take the chance to replace all wrong usage of global position/transform. Also there are cases like e.g. ConnectionsDock, where the menu isn't properly trimmed:
image
Such popups need popup->set_size(Vector2()) call.

editor/editor_inspector.cpp Outdated Show resolved Hide resolved
@groud
Copy link
Member

groud commented Sep 1, 2021

This PR looks quite similar to #37506, but I am not sure that on e is still being worked on. You might want to have a look to it.

@jmb462
Copy link
Contributor Author

jmb462 commented Sep 1, 2021

This PR looks quite similar to #37506, but I am not sure that on e is still being worked on. You might want to have a look to it.

@groud All cases fixed by this PR are also fixed in #37506 with the same modifications and #37506 is more exhaustive.

I think we could close this one and "reactivate" the review process of #37506.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 1, 2021

@jmb462 If you are up for it, it would likely be preferable if you were to incorporate the changes from the other PR here, add that other contributor as co-author and see it through to the end. The other PR is a year old, they may not be ready to work on it right now. But you are here 🙃

@jmb462 jmb462 force-pushed the fix-bad-popup-offset-with-single-window-off branch from 9a0f422 to 19de8f9 Compare September 6, 2021 19:40
@jmb462 jmb462 requested review from a team as code owners September 6, 2021 19:40
@jmb462
Copy link
Contributor Author

jmb462 commented Sep 6, 2021

OK, I've included most of @giarve modification in this PR (from #37506).
I've also found one or two popups that still use get_global_transform() and fixed it.
I've added some default size to popup to ensure they are trimmed properly (each time I can't find set_size() for these popups)

BUT I didn't include @giarve modification for following files cause it wasn't clear to me :

  • scene/main/window.cpp related to screen set current screen
  • platform/linuxbsd/display_server_x11.cpp cause I don't understand this part

@jmb462 jmb462 force-pushed the fix-bad-popup-offset-with-single-window-off branch 2 times, most recently from 8310e08 to 414b844 Compare September 27, 2021 16:20
@jmb462 jmb462 changed the title Fix bad popups offset in editor with single window off (Fix #47217) Fix bad popups offset in editor with single window off Oct 3, 2021
@jmb462 jmb462 force-pushed the fix-bad-popup-offset-with-single-window-off branch from 414b844 to b4f551f Compare October 3, 2021 10:40
@jmb462 jmb462 force-pushed the fix-bad-popup-offset-with-single-window-off branch from b4f551f to 7d3e8f8 Compare October 23, 2021 15:17
@KoBeWi
Copy link
Member

KoBeWi commented Nov 25, 2021

Needs rebase, also set_size(Vector2(1, 1) needs to be changed to reset_size().
Is this finished aside from that?

@jmb462 jmb462 force-pushed the fix-bad-popup-offset-with-single-window-off branch from 7d3e8f8 to 1ee7ed2 Compare November 27, 2021 18:02
@jmb462
Copy link
Contributor Author

jmb462 commented Nov 27, 2021

Needs rebase, also set_size(Vector2(1, 1) needs to be changed to reset_size(). Is this finished aside from that?

@KoBeWi Done. Seems finished to me.

@TokageItLab
Copy link
Member

Does it fix to #50720? I guess that issue caused by same reason.

@jmb462
Copy link
Contributor Author

jmb462 commented Nov 27, 2021

Does it fix to #50720? I guess that issue caused by same reason.

No. It only deals with popups.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 27, 2021

So I accidentally opened an extremely similar PR 3 days ago, which has most of the changes that this one has. I made sure to go over every single popup, so you can take a look if your PR isn't missing something I did. I know that at least my changes to visual_shader_editor_plugin.cpp are still relevant, so you could just copy them over here.

@jmb462 jmb462 force-pushed the fix-bad-popup-offset-with-single-window-off branch from 1ee7ed2 to e6752dd Compare November 29, 2021 13:37
@jmb462 jmb462 requested a review from a team as a code owner November 29, 2021 13:37
@jmb462
Copy link
Contributor Author

jmb462 commented Nov 29, 2021

So I accidentally opened an extremely similar PR 3 days ago, which has most of the changes that this one has. I made sure to go over every single popup, so you can take a look if your PR isn't missing something I did. I know that at least my changes to visual_shader_editor_plugin.cpp are still relevant, so you could just copy them over here.

Done. I've completed this PR with some of your changes that were missing here.

@jmb462 jmb462 force-pushed the fix-bad-popup-offset-with-single-window-off branch from e6752dd to 5306401 Compare November 29, 2021 13:52
@akien-mga
Copy link
Member

akien-mga commented Nov 29, 2021

I just created a new conflict by merging another PR, so needs a rebase.

Co-authored-by: Gil Arasa Verge <gilarasaverge@gmail.com>
Co-authored-by: Tomasz Chabora <kobewi4e@gmail.com>
@akien-mga akien-mga force-pushed the fix-bad-popup-offset-with-single-window-off branch from 5306401 to 81efebb Compare December 6, 2021 13:30
@akien-mga
Copy link
Member

I just created a new conflict by merging another PR, so needs a rebase.

I did it myself and force pushed the rebase so that we can get it merged.

@akien-mga
Copy link
Member

For the record there's a few more places with get_global_transform().xform which might be worth reviewing, if they haven't been already:

$ rg "get_global_transform\(\).xform"
scene/3d/soft_dynamic_body_3d.cpp
403:                    PhysicsServer3D::get_singleton()->soft_body_move_point(physics_rid, r[i].point_index, r[i].spatial_attachment->get_global_transform().xform(r[i].offset));

scene/3d/visual_instance_3d.cpp
36:     return get_global_transform().xform(get_aabb());

scene/2d/node_2d.cpp
362:    return get_global_transform().xform(p_local);

editor/animation_track_editor.cpp
5229:                   box_selecting_from = scroll->get_global_transform().xform(mb->get_position());
5274:           Vector2 to = scroll->get_global_transform().xform(mm->get_position());

scene/3d/node_3d.cpp
757:    return get_global_transform().xform(p_local);

editor/plugins/node_3d_editor_plugin.cpp
6528:                                   aabb = collision_shape->get_global_transform().xform(collision_shape->get_shape()->get_debug_mesh()->get_aabb());
6534:                                           aabb.merge_with(col_shape->get_global_transform().xform(col_shape->get_shape()->get_debug_mesh()->get_aabb()));

@KoBeWi
Copy link
Member

KoBeWi commented Dec 6, 2021

These are unrelated. The PR was only about popups. get_global_transform() is in window's local coordinates, but popups expect screen coordinates (get_screen_transform()).

@akien-mga akien-mga merged commit e808200 into godotengine:master Dec 6, 2021
@akien-mga
Copy link
Member

Thanks!

@jmb462 jmb462 deleted the fix-bad-popup-offset-with-single-window-off branch December 7, 2021 17:48
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.

DisplayServer: Context menu wrong offset in code editor with single-window mode off
7 participants