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 set_cursor_position method to Window #917

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Add set_cursor_position method to Window #917

merged 1 commit into from
Nov 26, 2020

Conversation

smokku
Copy link
Member

@smokku smokku commented Nov 22, 2020

This allows programatically setting cursor position in Window.

window.set_cursor_grab(locked).unwrap();
window
.set_cursor_grab(locked)
.expect("Unable to un/grab cursor");
Copy link
Member

@cart cart Nov 22, 2020

Choose a reason for hiding this comment

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

Now that we have logging set up by default, I think it makes sense to make these types of errors "soft errors" (ex: just use error!("Unable to un-grab cursor")).

I'd much prefer using "recoverable errors" when possible, but in this case the "command" has already been sent.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you suggest changing this error handling?

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'm not sure you can call these recoverable. If the game expects the cursor grabbed, it most probably won't work as expected with cursor ungrabbed.
Similarly game expects cursor to be at the set position and behavior when it is not may be unpredictable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially there could be a mechanic to set a WindowCommand to panic on failure (rather than print an error), so then it could be defined whether exclusive cursor grab (or other window commands) can fail gracefully in the context of the configuring application.

Copy link
Member

Choose a reason for hiding this comment

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

While I'm sure some people would find that useful, adding a bunch of "knobs" like that have negative implications too:

  1. more implementation complexity
  2. more code paths. this makes the behavior harder to document, support, and understand

I think in this case we should just make a decision. It won't please everyone, but it will probably please 99.9% of people.

And if we're going to choose one, I think we should log it as a "soft" error. This is the sort of thing that will probably fail for "os support reasons". I don't think devs will want their games to crash because the cursor didn't lock. If someone must have WindowCommands panic in this case, they can register a new tracing subscriber that panics on incoming logs. Or (probably even better) we could create a new Events<WindowError> collection, which developers could consume to detect these cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the way you would like to go then I support it. I especially support the soft errors, and the events would be a good way of doing it.

@Moxinilian Moxinilian added C-Feature A new feature, making something new possible A-Windowing Platform-agnostic interface layer to run your app in labels Nov 23, 2020
@smokku
Copy link
Member Author

smokku commented Nov 23, 2020

OK. I added error! calls, but I don't feel like adding the whole Events<WindowError> mechanics just to handle these unlikely errors. ;-)

@ambeeeeee
Copy link
Contributor

I can do that if you want, as a different pr

@cart cart merged commit b2c8295 into bevyengine:master Nov 26, 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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants