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

Coordinate systems #3891

Open
madsmtm opened this issue Aug 26, 2024 · 3 comments
Open

Coordinate systems #3891

madsmtm opened this issue Aug 26, 2024 · 3 comments
Labels
B - bug Dang, that shouldn't have happened C - needs discussion Direction must be ironed out S - api Design and usability

Comments

@madsmtm
Copy link
Member

madsmtm commented Aug 26, 2024

While doing #3890, I reviewed all our instances of [Physical|Logical|]Position, and categorized them relative to their origin point:

Desktop coordinates:

  • MonitorHandle::position
  • WindowEvent::Moved
  • Window::outer_position

Window coordinates:

Surface coordinates:

  • Window::set_ime_cursor_area
  • Window::show_window_menu

MouseScrollDelta::PixelDelta is a delta, so it has no origin and hence no coordinate system.

I believe that we should make set_cursor_position and the pointer APIs use surface coordinates, since that's the easier coordinate system for users to work with.

@madsmtm madsmtm added B - bug Dang, that shouldn't have happened S - api Design and usability C - needs discussion Direction must be ironed out labels Aug 26, 2024
@madsmtm madsmtm added this to the Version 0.31.0 milestone Aug 26, 2024
@kchibisov
Copy link
Member

I believe that we should make set_cursor_position and the pointer APIs use surface coordinates, since that's the easier coordinate system for users to work with.

it's not, it's easy as long as it matches pointer events, otherwise you'd have to convert back and forth all the time.

So, generally, coordinates in mouse events should be surface local, thus making all the set position stuff a surface local.

Window::show_window_menu is window coordinates, since it's a hint for system where to place menu over the window.

@madsmtm
Copy link
Member Author

madsmtm commented Aug 26, 2024

I believe that we should make set_cursor_position and the pointer APIs use surface coordinates, since that's the easier coordinate system for users to work with.

it's not, it's easy as long as it matches pointer events, otherwise you'd have to convert back and forth all the time.

It's more cumbersome to do things like hit-testing if the pointer position is in window coordinates instead of surface coordinates.

So, generally, coordinates in mouse events should be surface local, thus making all the set position stuff a surface local.

That's what I'm saying? As in, I agree that mouse/pointer events should be relative to the surface's top-left corner, not the window's top-left corner.

Window::show_window_menu is window coordinates, since it's a hint for system where to place menu over the window.

Idk what we are doing here, it's only implemented on Windows and Wayland, and I understand neither of those, I just took a guess from the Windows impl using ClientToScreen internally.

I think that, if we make everything else use surface coordinates, it might make sense to also make this use surface coordinates for consistency, regardless of how it works internally.

@kchibisov
Copy link
Member

I think that, if we make everything else use surface coordinates, it might make sense to also make this use surface coordinates for consistency, regardless of how it works internally.

No, this API is only present for the top-level, top-level is a window coordinates, so any other coordinates don't make any sense.

You can pretty much figure out which coordinate system should be used based on whether subview/subsurfaces have them or based on the wording in the protocols.

In general, it would mean for both windows/wayland backend to convert from local to window coordinates which is not really trivial sometimes, and it doesn't make much sense...

Though if majority backends do surface local, then maybe we can use surface local as well...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened C - needs discussion Direction must be ironed out S - api Design and usability
Development

No branches or pull requests

2 participants