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

Add screen-related attributes to mouse input events #82800

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Oct 4, 2023

Add screen velocity and screen relative position to InputEventMouseMotion and InputEventScreenDrag.
Based on discussion in RC.

@Calinou
Copy link
Member

Calinou commented Oct 4, 2023

Does this PR address #30950?

@Sauermann
Copy link
Contributor Author

Does this PR address #30950?

No, because it doesn't target 3.x. In 4.x #30950 should already be resolved.

@Sauermann Sauermann force-pushed the fix-screen-mousemotion branch 3 times, most recently from 49eead0 to 266b8d2 Compare October 7, 2023 19:36
@Sauermann Sauermann marked this pull request as ready for review October 7, 2023 21:16
@Sauermann Sauermann requested review from a team as code owners October 7, 2023 21:16
@Sauermann Sauermann modified the milestones: 4.x, 4.3 Oct 12, 2023
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

This was discussed in an Input PR meeting, and was approved!
Thanks.

core/input/input_event.cpp Show resolved Hide resolved
core/input/input_event.h Outdated Show resolved Hide resolved
@Sauermann Sauermann force-pushed the fix-screen-mousemotion branch from 266b8d2 to e01c68b Compare November 13, 2023 16:43
@Riteo
Copy link
Contributor

Riteo commented Nov 13, 2023

How should those fields be filled by platforms that can't query such data? Wayland is mostly window-centric and the mouse position can only be queried in window-space when it's on a surface.

@akien-mga
Copy link
Member

akien-mga commented Jan 4, 2024

How should those fields be filled by platforms that can't query such data? Wayland is mostly window-centric and the mouse position can only be queried in window-space when it's on a surface.

Poke @Sauermann @bruvzg

@Sauermann
Copy link
Contributor Author

This point was discussed on rocket chat. See https://chat.godotengine.org/channel/platforms?msg=777ZLDf26xiuswuou
If I remember correctly, On wayland the relative attributes can also be calculated.

@akien-mga
Copy link
Member

Sounds good. I guess we should evaluate whether we want to merge #86180 first, or whether the needed changes for this PR could also be added to #86180 (as you prefer @Riteo).

@Riteo
Copy link
Contributor

Riteo commented Jan 4, 2024

@akien-mga I suppose that they could be merged independently. The WDS doesn't support every feature yet and I'm purposely avoiding to touch it too much to help with reviewing (outside of the pending rebase obviously)

Edit: to clarify, my plan is to make smaller PRs right away after merging. Hopefully this will be easier to handle.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 63d6bda), it works as expected on Linux in all stretch modes.

This seems to be exactly what I want with relative_unscaled and velocity_unscaled coordinates, albeit with a different name. (reduz also proposed global_relative and global_velocity terms for this feature, but I can't find the exact issue/PR.)

In terms of naming, I still prefer relative_unscaled and velocity_unscaled (as I think they're the most likely terms to be searched by users), even if they are less technically correct.

We should make sure to update documentation and demos to use these new properties if we merge this PR.

Testing project: test_unscaled_relative_input_2.zip

@Sauermann
Copy link
Contributor Author

(reduz also proposed global_relative and global_velocity terms for this feature, but I can't find the exact issue/PR.)

In terms of naming, I still prefer relative_unscaled and velocity_unscaled (as I think they're the most likely terms to be searched by users), even if they are less technically correct.

My reasoning for not following the "global"-naming scheme is based on godotengine/godot-proposals#3866.
I would prefer to explicitly state the name of the relevant coordinate system (screen) in the function name.

Please let me know, if a different naming-scheme is preferred. In that case I will make the necessary changes.

The comment-additions look good at a first glance. Will include them later.

@kitbdev
Copy link
Contributor

kitbdev commented Feb 5, 2024

If unscaled is more likely to be searched, could we add that word into the documentation instead of changing the name?

@Calinou
Copy link
Member

Calinou commented Feb 5, 2024

If unscaled is more likely to be searched, could we add that word into the documentation instead of changing the name?

#78990 would allow adding a search alias 🙂

@Sauermann Sauermann force-pushed the fix-screen-mousemotion branch from e01c68b to f2a538a Compare February 5, 2024 21:59
@Sauermann Sauermann requested a review from a team as a code owner February 5, 2024 21:59
@Sauermann Sauermann force-pushed the fix-screen-mousemotion branch from f2a538a to 2235a1c Compare February 5, 2024 22:30
@Sauermann
Copy link
Contributor Author

I have updated the PR:

  • Rebased on master after Add Wayland support #86180 and added changes to Wayland. (Untested)
  • Included multiple documentation additions by Calinou. Please let me know if I should mention you as a co-author.
  • Included word "unscaled" into descriptions.

@Riteo
Copy link
Contributor

Riteo commented Feb 5, 2024

I'm trying to review this to help with the Wayland part, but I'm having some troubles understanding some things.

I see that you're just setting the same value both for the screen velocity and relative position, yet one is supposed to be scaled while the other not. Where is the scaling happening?

@Sauermann
Copy link
Contributor Author

Sauermann commented Feb 5, 2024

In the displayserver-context both are identical.

Scaling happens in Viewport::push_input so that the event is converted to viewport coordinates, while the event is accessed inside of the viewport:

godot/scene/main/viewport.cpp

Lines 3338 to 3343 in d335281

Ref<InputEvent> ev;
if (!p_local_coords) {
ev = _make_input_local(p_event);
} else {
ev = p_event;
}

The code that actually scales the values is in InputEventMouseMotion::xformed_by.

@Riteo
Copy link
Contributor

Riteo commented Feb 5, 2024

@Sauermann I wonder then if we shouldn't eventually make some of those values "hidden" or anyways set up independently of each DS. There's also the whole issue with fetching the velocity from Input when it could just do so itself.

Anyways, this is probably stuff for another PR, just pointing that out.

@Sauermann
Copy link
Contributor Author

@Riteo partially these values have been in the engine for a very long time. After #37317 perhaps it makes sense to reevaluate these aspects.

@akien-mga akien-mga merged commit 74b03ed into godotengine:master Feb 9, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

2d or viewport stretch mode does not scale mouse coordinates
7 participants