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 mouse button bindings #792

Merged
merged 7 commits into from
Nov 19, 2023
Merged

Add mouse button bindings #792

merged 7 commits into from
Nov 19, 2023

Conversation

tomas7770
Copy link
Contributor

Description

Adds support for binding mouse buttons to input actions, similar to existing implementation of keyboard/gamepad. Serialization functions are also implemented in order to support adding bindings in an asset.
Adds a new showcase to the input sample to test the bindings.

Checklist

  • Self-review changes.
  • Evaluate impact on the documentation.
  • Ensure test coverage.
  • Write new samples.

@tomas7770 tomas7770 linked an issue Nov 9, 2023 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Nov 9, 2023

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-11-19 11:04 UTC

@tomas7770 tomas7770 force-pushed the 577-add-mouse-button-bindings branch from 7e325b0 to 716bf25 Compare November 10, 2023 19:53
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (3f97f0e) 41.74% compared to head (dcc5cc0) 41.60%.

Files Patch % Lines
core/src/cubos/core/io/window.cpp 0.00% 22 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #792      +/-   ##
==========================================
- Coverage   41.74%   41.60%   -0.14%     
==========================================
  Files         108      108              
  Lines        6998     7020      +22     
==========================================
  Hits         2921     2921              
- Misses       4077     4099      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomas7770 tomas7770 self-assigned this Nov 10, 2023
Copy link
Member

@RiscadoA RiscadoA left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @tomas7770! LGTM, just needs some changes to the input sample's page before being merged :)

engine/samples/input/main.cpp Show resolved Hide resolved
Copy link
Member

@RiscadoA RiscadoA left a comment

Choose a reason for hiding this comment

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

Looks great! Ty! Just one thing before merging: some of your commit messages don't follow the format - you can use git rebase -i HEAD~7 to edit your previous 7 commits, and change each pick to reword (or just r) to change their messages. Most of your commits would fall under the feat(input) scope - for formatting one, you should use style: fix formatting. The commit you have which touches both sides, I would use feat(window) since its mostly window related code that's being changed. Usually when we can't choose a scope we try to split the commits into multiple ones or just omit the scope entirely.

Also, some tips for future PRs:

  • you can press the 'auto merge' button to merge this automatically when all conversations are resolved and when the PR is approved.
  • feel free to press the resolve button on reviews after pushing fixes for them.

@tomas7770
Copy link
Contributor Author

Looks great! Ty! Just one thing before merging: some of your commit messages don't follow the format - you can use git rebase -i HEAD~7 to edit your previous 7 commits, and change each pick to reword (or just r) to change their messages. Most of your commits would fall under the feat(input) scope - for formatting one, you should use style: fix formatting. The commit you have which touches both sides, I would use feat(window) since its mostly window related code that's being changed. Usually when we can't choose a scope we try to split the commits into multiple ones or just omit the scope entirely.

Something like this?

feat(input): partially implement mouse button bindings
feat(window): add serialization for mouse buttons
fix(input): add mouse binds to input sample, to fix crash
feat(input): add mouse button showcase to input sample
style: fix formatting
feat(input): remove mouse for next-showcase in input sample
docs(input): add mouse showcase to input sample docs

@RiscadoA
Copy link
Member

Looks great! Ty! Just one thing before merging: some of your commit messages don't follow the format - you can use git rebase -i HEAD~7 to edit your previous 7 commits, and change each pick to reword (or just r) to change their messages. Most of your commits would fall under the feat(input) scope - for formatting one, you should use style: fix formatting. The commit you have which touches both sides, I would use feat(window) since its mostly window related code that's being changed. Usually when we can't choose a scope we try to split the commits into multiple ones or just omit the scope entirely.

Something like this?

feat(input): partially implement mouse button bindings
feat(window): add serialization for mouse buttons
fix(input): add mouse binds to input sample, to fix crash
feat(input): add mouse button showcase to input sample
style: fix formatting
feat(input): remove mouse for next-showcase in input sample
docs(input): add mouse showcase to input sample docs

LGTM!

@tomas7770 tomas7770 force-pushed the 577-add-mouse-button-bindings branch from a4a4f31 to 4a17331 Compare November 13, 2023 13:26
@tomas7770 tomas7770 force-pushed the 577-add-mouse-button-bindings branch from 4a17331 to dcc5cc0 Compare November 19, 2023 10:53
@tomas7770 tomas7770 merged commit ee7852c into main Nov 19, 2023
10 checks passed
@tomas7770 tomas7770 deleted the 577-add-mouse-button-bindings branch November 19, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add mouse button bindings
2 participants