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 Axis API #227

Closed
wants to merge 8 commits into from
Closed

Add Axis API #227

wants to merge 8 commits into from

Conversation

zenMaya
Copy link

@zenMaya zenMaya commented Aug 18, 2020

This references the Axis API issue #59

I've added generic axes, mouse axes and cursor axes

Axes

  1. generic axes:
    they use Axis<AxisId> that can be retrieved from winit
    always have f32 dimension. Winit doesn't support multidimensional axes, so you need to register each one

  2. mouse axes
    I have tried to use WindowEvent::CursorMoved event first, but as I found out WindowEvent::CursorMoved problem

So I decided to separate those two, essentially moving CursorPosition from UI to Axis<Cursor>, as well as registering mouse data with Axis<Mouse>

Since generic axes don't support multiple dimensions, Cursor and Mouse are enums, where

enum Cursor {
  Horizontal(WindowId),
  Vertical(WindowId),
}

enum Mouse {
  Horizontal,
  Vertical,
}

Cursor and Mouse differences

Cursor does support local2window position, but is out of sync once cursor leaves the window, and synchronizes once it enters (maybe exiting and entering window events should be exposed as well)

Mouse does not support local2window position and still needs to be locked with winit manually, also has a problem that position isn't platform independent (that means it is different step on every platform) but is not disabled by going out of window and independent on cursor acceleration performed outside of bevy.

Issues

There are still some issues:

  • UI still uses it's own cursor position, I essentially just patched it for now
  • bevy could easily support Touch with Axis API since winit supports it
  • Cursor movements now cause 2 events instead of one with Vec2 since it's easier to parse it (one for horizontal and vertical), not sure if it matters
  • There is no easy way to get generic axes ids that doesn't involve messing with winit itself
  • I had to expose some variables and I am not sure if all of them should be in prelude. That is to be decided

This is definitely not final, but I would love some feedback on what to change. But it is in a functional state

@zenMaya zenMaya marked this pull request as draft August 18, 2020 14:31
@zenMaya zenMaya marked this pull request as ready for review August 18, 2020 14:32
@karroffel karroffel added C-Feature A new feature, making something new possible A-Input Player input via keyboard, mouse, gamepad, and more labels Aug 18, 2020
@cart
Copy link
Member

cart commented Aug 20, 2020

In general this is very welcome and looks like its on the right track. Starting a review now

@cart
Copy link
Member

cart commented Aug 25, 2020

Thanks for your patience. Can we add an example to illustrate how people can use this?

@zenMaya
Copy link
Author

zenMaya commented Aug 26, 2020

Of course, here in is!

Comment on lines 36 to 39
match self.axes.get(&axis_id) {
Some(axis) => Some(*axis),
None => None,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
match self.axes.get(&axis_id) {
Some(axis) => Some(*axis),
None => None,
}
self.axes.get(&axis_id).copied()

Copy link
Author

Choose a reason for hiding this comment

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

yes, this seem much better

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I think the "Axis is a single axis" approach is reasonable, but it makes two dimensional axis consumption (which I consider to be a "common" case), much harder.

I would prefer it if this api was multi-dimensional like the original issue.

.mouse_movement_event_reader
.iter(&mouse_movement_events)
{
mouse_axis.add(Mouse::Horizontal, event.delta.x());
Copy link
Member

Choose a reason for hiding this comment

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

Using add() in these systems results in broken output. In the example, i get gigantic and/or incorrect numbers:

mouse: [x: :-1843, y: :321] cursor: [x: :4682519.5, y: :2292061.5]

I'm pretty sure we want set()

Copy link
Author

Choose a reason for hiding this comment

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

no, add adds mouse delta. I've tried this in my simple FPS example and this is correct. You need to multiply by sensitivity which is about 0.05 to get smooth mouse movement, but this works for proper rotation. Set wouldn't work for mouse_delta

Copy link
Member

Choose a reason for hiding this comment

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

First, its right to call out that "mouse delta" is a little different from the other inputs. I think, if anything, we should consider removing mouse delta from the axis api as it doesn't really fit into the same "bucket" as the other inputs. We might have a number of "mouse delta" events per frame, so we can't consider them individually to determine the "state" of the mouse. I would argue that there is no "fixed state" of the mouse. Only a list of "relative changes in state" each frame.

In my mind the purpose of the Axis api is to provide a simple interface to consume raw inputs that represent the current state of an axis. For example, if we add joysticks to this api, I would want the x-axis to be in the range [-1, 1]. Users could then use those numbers to determine how far the joystick has been tilted to the left or right (and adjust things like character speed based on that state). Currently all of the systems use add(), including "non delta" systems. This results in very large, "aggregate" numbers. In the joystick example, I wouldn't want it to increment to very large numbers as I wiggle the joystick, as that prevents the data from being used to determine "how fast should i move this character this frame and in what direction?".

The current aggregate mouse delta works for rotation in your fps because rotations can "wrap around". If you want that behavior, I think you should maintain an aggregation in a separate resource. But it would probably be better to take the raw "delta" events, multiply them by some weight, and rotate the camera directly (relative to its current position). That way you don't need to maintain an aggregation (which is pretty meaningless across frames).

Copy link
Author

Choose a reason for hiding this comment

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

But what about Cursor? Cursor also returns delta and not it's actual position. That is the same with joysticks. Should they use aggregate function? Because otherwise it'll be something like 0.0 0.0 33.0 0.1 0.1 -34.0 0.1... I don't see why this should not be aggregate.

Copy link
Member

Choose a reason for hiding this comment

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

Gilrs joystick events are absolute values in the range [-1, 1]: https://docs.rs/gilrs/0.7.4/gilrs/ev/enum.EventType.html#variant.AxisChanged. For example, -1 is "all the way left" and 1 is "all the way right"

Winit's CursorMoved event is also an absolute value: https://docs.rs/winit/0.22.2/winit/event/enum.WindowEvent.html#variant.CursorMoved

Copy link
Author

Choose a reason for hiding this comment

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

okay, have read that wrong. Then I have no objections, it didn't make sense.

// but it's inconsistent across platforms
// same mouse move can have a different scale
// ideal for 3D camera movement etc.
mouse_axis.register(Mouse::Horizontal);
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer it if the "default" axes are registered automatically. This is a common enough case that we should handle it on behalf of users.

let mut mouse_motion_events =
app.resources.get_mut::<Events<MouseMotion>>().unwrap();
mouse_motion_events.send(MouseMotion {
delta: Vec2::new(delta.0 as f32, delta.1 as f32),
});
}
}
DeviceEvent::Motion { axis, value } => {
let mut axis_events = app.resources.get_mut::<Events<Motion>>().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This leaks the internals of winit (via the winit axis id), which means users consuming this are taking a hard dependency on winit. I would prefer it if we provided our own abstraction here. Lets remove this for now and spec it out later.

Copy link
Author

Choose a reason for hiding this comment

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

So do you mean something like bevy_device in the future? same as bevy_window, reimplementing winit.

Copy link
Member

Choose a reason for hiding this comment

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

I think we would probably just extend bevy_input, but yeah it would be the same idea as bevy_window applied to controllers.

pub enum Cursor {
Vertical(WindowId),
Horizontal(WindowId),
}
#[derive(Debug, Clone)]
pub struct CursorMoved {
Copy link
Member

Choose a reason for hiding this comment

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

Splitting out the event like this makes it much harder to consume. In almost every case, users (and engine devs) will want to consume x and y at the same time. Splitting them means that getting the original x,y requires reading an event, checking that its "x", then reading the next event and assuming it will be "y".

I think this should be reverted.

Copy link
Author

@zenMaya zenMaya Sep 2, 2020

Choose a reason for hiding this comment

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

So Axes should exist only as 2D?

Copy link
Member

Choose a reason for hiding this comment

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

At the very least, I don't think we should be breaking up 2d events to meet the needs of the Axis api. I personally think 2d devices should have 2d axis apis because that will make them simpler and more natural to consume.

@cart
Copy link
Member

cart commented Sep 8, 2020

I think I've come around to leaving axes as 1-dimensional. This is how unity, godot, and gilrs do it. And "axis" is also inherently 1d, so we would need to rename the api to Axes instead of Axis to avoid confusion.

I still would prefer it if we didnt change event apis to line up with axis apis. 2d events should stay 2d.

@simpuid
Copy link
Contributor

simpuid commented Sep 12, 2020

@cart @WesterWest I think we can use something like fn get_2d(&self, axis_2d: (T, T)) -> Option<Vec2> wrapped over the current implementation to get 2d vector as axis output.

Or we can expose a trait which returns the tuple of axis handles and consume structs that implement those traits instead of tuple in the above function.

@cart
Copy link
Member

cart commented Sep 14, 2020

@simpuid yeah its definitely worth trying to build an abstraction on top of the 1d api, but at least in this pr I think we should keep it scoped to 1d.

@WesterWest no pressure, but if we can get comments / merge conflicts resolved in the next day or so, this (and @simpuid's joystick support) will land in the bevy 0.2 release.

@cart
Copy link
Member

cart commented Sep 18, 2020

I've thought awhile about this, and I think I'm just going to merge the joystick pr and close this out. I'm not convinced that mouse state is a good fit for the axis api. In general I think its a good idea to let the Axis api be for "normalized axes" in the [-1, 1] range. Instead I think we should probably add a new resource type specific to the cursor (or alternatively, tack it on to the existing Window api).

@cart cart closed this Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants