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

Store mouse cursor position in Window #940

Merged
merged 1 commit into from
Dec 3, 2020
Merged

Store mouse cursor position in Window #940

merged 1 commit into from
Dec 3, 2020

Conversation

smokku
Copy link
Member

@smokku smokku commented Nov 28, 2020

This PR adds cursor_position field to Window and a system to track CursorMoved events.

This complements already existing set_cursor_position method and aligns with how keyboard input is being handled - allowing for both handling by events or just accessing available state.

@Moxinilian Moxinilian added C-Enhancement A new feature A-Windowing Platform-agnostic interface layer to run your app in labels Nov 28, 2020
@ambeeeeee
Copy link
Contributor

If this is considered for merging, I think that we should have a resource that defines the cursor position and the window that cursor is over, since we'd already be running a system that captures the position.

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
@smokku
Copy link
Member Author

smokku commented Nov 29, 2020

If this is considered for merging, I think that we should have a resource that defines the cursor position and the window that cursor is over, since we'd already be running a system that captures the position.

That would be duplicating information.
I think we should rather add FocusedWindow resource and do something like this:

let focused_window_id = focused_window.id.unwrap();
let cursor_position = windows.get(focused_window_id).unwrap().cursor_position();

But this is a separate PR (building on this one).

@ambeeeeee
Copy link
Contributor

Not a bad idea.

@@ -24,3 +25,30 @@ pub fn exit_on_window_close_system(
app_exit_events.send(AppExit);
}
}

pub fn update_cursor_position(
Copy link
Member

Choose a reason for hiding this comment

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

Random thought: we could handle these events in bevy_winit

pros: window state is correct before schedule execution starts (which means systems in PRE_UPDATE can use the cursor position). one less system running

cons: more backend-specific logic

I personally think moving this into bevy_winit's window event handler is the right call, but I'm curious what y'all think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a good idea, I'm excited to have bevy get rid of the hacky way of doing cursor pos currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean that the code residing in bevy_winit would modify bevy_window's resources?

Copy link
Member

Choose a reason for hiding this comment

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

Yup! We already do that in a few places

Copy link
Member Author

Choose a reason for hiding this comment

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

What about other windowing plugins? SDL, miniquad?
Should they also touch bevy_window internals instead of sending messages and letting bevy_window handle these as it fits?

Copy link
Member

Choose a reason for hiding this comment

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

I totally see your point and for most things I would just agree outright (because the design you're suggesting is "cleaner"). I still think we should make an exception here for a few reasons:

  1. The design as it stands is already "unclean" in some respects. The winit backend already mutates bevy_window state for a number of scenarios: creating windows, resizing windows, touch events, scale factor changes
  2. Having access to up to date window state as early as possible seems valuable
  3. New window backends are relatively uncommon (and are generally built by proficient people), so I'm fine eating extra complexity of moderate size there to make user facing scenarios nicer.

Copy link
Member Author

Choose a reason for hiding this comment

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

And I do see your point.
So, while I'm working with this code already, maybe I should take an opportunity and change other messaging like this and move it to direct mutation? Where it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sold!

Copy link
Member Author

Choose a reason for hiding this comment

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

I added update_cursor_position_from_backend like other update_*_from_backend methods and removed the system reading messages.

I have a feeling that mouse handling in my game works smoother now. 😄

@smokku
Copy link
Member Author

smokku commented Dec 1, 2020

Not a bad idea.

BTW, for main window cursor position I have this extension trait in my game:

trait WindowsCursorPositionExt {
    fn get_cursor_position(&self) -> Option<Vec2>;
}

impl WindowsCursorPositionExt for Windows {
    fn get_cursor_position(&self) -> Option<Vec2> {
        let window = self.get_primary().unwrap();
        window
            .cursor_position()
            .map(|pos| vec2(pos.x, window.height() as f32 - pos.y))
    }
}

It allows me to easily get cursor position remapped to screen coordinates.

@ambeeeeee
Copy link
Contributor

Thats cool!

@smokku smokku requested a review from cart December 2, 2020 23:08
@cart
Copy link
Member

cart commented Dec 3, 2020

Looks good to me!

@cart cart merged commit 1f2e417 into bevyengine:master Dec 3, 2020
@fopsdev fopsdev mentioned this pull request Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants