bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Published 20 hours ago • verified publisher icon bevyengine

Make grab_mode a bool instead of enum

Open IceSentry opened this issue 3 years ago • 7 comments

Objective

  • Closes https://github.com/bevyengine/bevy/issues/6590
  • The grab mode is platform dependent, this is problematic for bevy users since we can't easily use the recommended way to detect if the feature works like the winit docs recommend https://docs.rs/winit/0.27.5/winit/window/struct.Window.html#method.set_cursor_grab

Solution

Use a bool to represent the grab_mode on the bevy window and internally try to use the available mode like recommended by the winit docs.

Changelog

  • Renamed set_cursor_grab_mode and cursor_grab_mode to set_cursor_grabbed() and cursor_grabbed()
  • set_cursor_grabbed() now takes a bool instead of an enum
  • cursor_grabbed() now returns a bool

Migration Guide

// 0.9
// macos
window.set_cursor_grab_mode(CursorGrabMode::Locked);
// windows
window.set_cursor_grab_mode(CursorGrabMode::Confined);

// Now
window.set_cursor_grabbed(true);

Notes

~~Should we keep the grab_mode terminology? I'm not sure if I should rename this to something else.~~

Should we keep the existing api and add this new one to not break semver?

IceSentry avatar Nov 13 '22 21:11 IceSentry

Yeah we should rename this to reflect the bool nature

alice-i-cecile avatar Nov 13 '22 21:11 alice-i-cecile

Ugh, this is technically a breaking change :( I'm probably fine violating semver here: the provided API basically doesn't work.

alice-i-cecile avatar Nov 13 '22 21:11 alice-i-cecile

One way to keep semver would be to keep the api and add a new one with bools. Maybe even tag the broken one as deprecated, but I'm not sure if deprecation warning break semver.

IceSentry avatar Nov 13 '22 21:11 IceSentry

this was set_cursor_lock_mode in the 0.8 times

mockersf avatar Nov 13 '22 22:11 mockersf

Yes, but I decided to keep the grab terminology to stay closer to winit. Would you say it's better to keep it named grab_mode instead of grabbed?

IceSentry avatar Nov 14 '22 00:11 IceSentry

Yeah I like the idea of keeping the old API with a deprecation so we can put this in the point release

alice-i-cecile avatar Nov 14 '22 00:11 alice-i-cecile

I reintroduced the api, but I modified the implementation a bit to avoid the error. This means that we technically don't need the boolean api, and this gives a tiny bit more control to the user. I left both in the PR for now, but I'm not sure what to do here.

IceSentry avatar Nov 14 '22 02:11 IceSentry

Updated to only use the fixed api and don't introduce a new api or remove anything.

IceSentry avatar Nov 25 '22 23:11 IceSentry

I'm very happy with this now. Can you update your PR description?

alice-i-cecile avatar Nov 25 '22 23:11 alice-i-cecile

I updated the title and changed the docs a little bit to reflect that it's possible that the getter will return a value that is different from the one sent to winit.

IceSentry avatar Nov 25 '22 23:11 IceSentry

Which commit is that? This PR shouldn't have any breaking commit.

IceSentry avatar Nov 26 '22 01:11 IceSentry

Oh, is the issue that I made this PR from main? If that's the case then yeah the 0.9.1 release will only cherry pick this PR.

IceSentry avatar Nov 26 '22 01:11 IceSentry

bors r+

alice-i-cecile avatar Nov 26 '22 13:11 alice-i-cecile

Pull request successfully merged into main.

Build succeeded:

bors[bot] avatar Nov 26 '22 13:11 bors[bot]