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 right-click on tilset atlas popup wrong position #71618

Merged

Conversation

groud
Copy link
Member

@groud groud commented Jan 18, 2023

Fixes #71327

@groud groud added this to the 4.0 milestone Jan 18, 2023
@Sauermann
Copy link
Contributor

Technically you are adding points of different coordinate systems:

  • get_screen_position() is in Screen/Viewport coordinates
  • get_local_mouse_position() is in CanvasItem-local coordinates

I didn't look in depth into this, but could you test, if the PR works, if the editor has a non-default Display Scale and is not in Single-Window-Mode? If not, you could try instead of the addition:
get_screen_transform().xform(get_local_mouse_position())

Conceptually the conversion to the Screen coordinates was definitively missing.

Maybe you could extend this PR to address

alternative_tile_popup_menu->popup(Rect2i(get_global_mouse_position(), Size2i()));

@groud
Copy link
Member Author

groud commented Jan 18, 2023

get_screen_transform().xform(get_local_mouse_position())

This seems indeed cleaner. I'll test it.
And yeah, I need to fix the issue in the other place, you are right. I'll amend the PR.

Fix similar bug in AnimationLibraryEditor
@groud groud force-pushed the fix_tileset_popup_wrong_position branch from 0ed12bd to c1de6dc Compare January 18, 2023 15:14
@groud
Copy link
Member Author

groud commented Jan 18, 2023

Done. I also fixed a similar issue in AnimationLibraryEditor.

@akien-mga akien-mga merged commit 1f72530 into godotengine:master Jan 18, 2023
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

For the record, there's a few more cases of this get_screen_position() + get_local_mouse_position() which could be worth reviewing:

$ rg get_screen_pos.*get_local_mouse
scene/gui/line_edit.cpp
251:                    menu->set_position(get_screen_position() + get_local_mouse_position());

doc/classes/Control.xml
427:                            popup_menu.position = get_screen_position() + get_local_mouse_position()

editor/editor_audio_buses.cpp
759:    delete_effect_popup->set_position(get_screen_position() + get_local_mouse_position());

editor/plugins/visual_shader_editor_plugin.cpp
3821:                   Point2 gpos = get_screen_position() + get_local_mouse_position();
3840:           Point2 gpos = get_screen_position() + get_local_mouse_position();
4682:                   _comment_title_popup_show(get_screen_position() + get_local_mouse_position(), selected_comment);
4685:                   _comment_desc_popup_show(get_screen_position() + get_local_mouse_position(), selected_comment);

editor/plugins/animation_blend_tree_editor_plugin.cpp
390:    add_node->get_popup()->set_position(graph->get_screen_position() + graph->get_local_mouse_position());

editor/plugins/script_editor_plugin.cpp
3086:   context_menu->set_position(get_screen_position() + get_local_mouse_position());

editor/animation_track_editor.cpp
2821:                           menu->set_position(get_screen_position() + get_local_mouse_position());

editor/editor_inspector.cpp
709:            menu->set_position(get_screen_position() + get_local_mouse_position());

editor/scene_tree_dock.cpp
2604:                   menu_properties->set_position(get_screen_position() + get_local_mouse_position());
2972:           filter_quick_menu->set_position(get_screen_position() + get_local_mouse_position());

editor/debugger/editor_debugger_tree.cpp
120:    item_menu->set_position(get_screen_position() + get_local_mouse_position());

@Sauermann
Copy link
Contributor

From a quick look, there are several occurrences of get_screen_position which look problematic in "scene/gui". I should probably investigate them after my get_screen_transform-fix gets merged.

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.

TileSet editor cell's context menu position is incorrect
3 participants