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

Added keyboard movement #63

Merged
merged 4 commits into from
Aug 19, 2024
Merged

Added keyboard movement #63

merged 4 commits into from
Aug 19, 2024

Conversation

Toniman575
Copy link
Contributor

@Toniman575 Toniman575 commented Aug 15, 2024

Simply added constant keyboard movement to the do_camera_movement system and MoveMode and DirectionKeys fields to PanCam and registered them in Bevy.

As I do not know if this change is even desired, this seemed to be the least intrusive API/code change.
Further changes I thought about but discarded because of reason above:

  • Option for accelerated movement.
  • Splitting the do_camera_movement into multiple systems.
  • Simultaneous mouse and keyboard MoveMode.

Addresses #52

@johanhelsing
Copy link
Owner

I think this makes sense scope-wise. Curious to hear anybody else's opinion as well...

API-wise: I think simultaneous movement should be implemented and be the default. Perhaps even the only mode, and remove the mode field. If you don't want keyboard controls, you can set direction_keys = DirectionKeys::NONE.

Curious for other thoughts on this as well.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@Toniman575
Copy link
Contributor Author

The implementation I went for seemed the most sane to me but I'm curious what u guys think.
Further thoughts:

  • We could move projection.area.size() behind if checks so it doesn't happen if unnecessary, but the performance gain seems rather insignificant and I am hesitant to add code complexity.
  • I think check_egui_wants_focus makes enough sense as it is right now, but we could check for wants_pointer_input and wants_keyboard_input separately depending on the input of the user. The easiest way seems to be to split do_camera_movement system into two separate systems for keyboard movement and mouse movement. Again though, this might be a bit too intrusive of a change, especially as this change would only be relevant for a crate feature.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

Thanks, API-wise, I think this adds it in a way that fits well with the scope and the rest of this crate.

We should update the readme/crate docs, maybe examples and this is good to go :)

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@johanhelsing johanhelsing added the enhancement New feature or request label Aug 17, 2024
@Toniman575
Copy link
Contributor Author

I added a bit of Documentation, but I'm not sure if its too wordy so I'll defer to @johanhelsing on that. Otherwise I'm fairly happy with the changes, hope u guys are as well.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

Great work!

Thanks also @tomara-x for reviewing!

❤️

@johanhelsing johanhelsing merged commit ec828f4 into johanhelsing:main Aug 19, 2024
3 checks passed
@Toniman575 Toniman575 deleted the keyboard-movement branch August 20, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants