Skip to content

3 dimension keycode preset #89

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

Merged
merged 8 commits into from
Apr 29, 2025

Conversation

Aceeri
Copy link
Contributor

@Aceeri Aceeri commented Apr 24, 2025

Adds a preset for 3 dimensional input actions.

Open to better names, SixDOF is just what came to mind for this but it sounds a bit odd.

I did a little builder pattern for wasd/arrow vs up down mappings since people might want to mix and match them. If there are any other up/down presets you think would be common lemme know :)

Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

Thanks you!

  • Could you also add reference in Cardinal docs to SixDOF and edit Axial to point to only Cardinal?
  • Ccould you also add an integration test in presets.rs?

src/preset.rs Outdated
/// struct Move;
/// ```
#[derive(Debug, Clone, Copy)]
pub struct SixDOF<I: IntoBindings> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about Spatial or Tertial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spatial maybe, but I'm not sure it is better than SixDOF...

I guess it requires a bit less knowledge since you don't need to know DOF = Degrees of Freedom and what that means.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alice-i-cecile what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide to keep SixDOF, maybe worth to rename it to SixDof instead. The convention is to capitalize only the first letter in abbreviations - like Aabb.

Another option to consider: name these presets <Something>1D, <Something>2D and <Something>3D. Just brainstorming 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya maybe Axis1D/Axis2D/Axis3D?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that might make axial vs axis2d confusing... Directional*D?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I Directional*D is also an option. It's a bit long, but Dir would be confusing and Bevy already have Dir* types.
Let's ask Alice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I like "Spatial" best :) SixDoF is really confusing to new gamedevs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Aceeri let's go with Spatial then!

Copy link

codecov bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.30%. Comparing base (5f0923b) to head (17e2528).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/preset.rs 92.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   86.17%   86.30%   +0.13%     
==========================================
  Files          34       34              
  Lines        1034     1059      +25     
==========================================
+ Hits          891      914      +23     
- Misses        143      145       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Aceeri
Copy link
Contributor Author

Aceeri commented Apr 25, 2025

* Ccould you also add an integration test in `presets.rs`?

Do you think I should add a new file for the 3 dimensions or modify the existing stuff to just be Vec3? I see Bidirectional is in there piggy backing off it but just using the X directions.

@Shatur
Copy link
Contributor

Shatur commented Apr 25, 2025

I would modify the action output and constants to Vec3.

@Shatur
Copy link
Contributor

Shatur commented Apr 25, 2025

Or maybe even remove the constants in the test and use the constants from glam itself.

Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

Everything looks good! But I'd like to get @alice-i-cecile's opinion on the naming. Here are the options we've considered:

  • SixDof (renamed from SixDOF since in abbreviations only the first letter should be capitalized)
  • Spatial
  • Directional*D for all presets

If you have other suggestions, please let us know!

@Shatur Shatur requested a review from alice-i-cecile April 25, 2025 08:09
Copy link
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I like this addition (it was in LWIM for a reason!), but we should rename first.

@Shatur Shatur requested a review from alice-i-cecile April 28, 2025 12:10
@alice-i-cecile alice-i-cecile enabled auto-merge (squash) April 29, 2025 16:11
@Shatur
Copy link
Contributor

Shatur commented Apr 29, 2025

@Aceeri sorry, could you resolve the conflicts?

@Shatur
Copy link
Contributor

Shatur commented Apr 29, 2025

Never mind - resolved myself 🙂

@alice-i-cecile alice-i-cecile merged commit 21ddc08 into projectharmonia:master Apr 29, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants