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

Replace axis-like input configuration with new input processors #494

Merged
merged 64 commits into from
Apr 12, 2024

Conversation

Shute052
Copy link
Collaborator

@Shute052 Shute052 commented Feb 28, 2024

Objective

Solution

Added input processors for SingleAxis, DualAxis, VirtualAxis, and VirtualDpad to refine input values:

  • Traits:
    • AxisProcessor: Handles single-axis values.
    • DualAxisProcessor: Handles dual-axis values.
  • Built-in Processors:
    • Pipelines: Combine multiple processors into a pipeline.
      • AxisProcessingPipeline: Chain processors for single-axis values.
      • DualAxisProcessingPipeline: Chain processors for dual-axis values.
    • Inversion: Reverses control (positive becomes negative, etc.)
      • AxisInverted: Single-axis inversion.
      • DualAxisInverted: Dual-axis inversion.
    • Sensitivity: Adjusts control responsiveness (doubling, halving, etc.).
      • AxisSensitivity: Single-axis scaling.
      • DualAxisSensitivity: Dual-axis scaling.
    • Value Bounds: Define the boundaries for constraining input values.
      • AxisBounds: Restricts single-axis values to a range.
      • DualAxisBounds: Restricts single-axis values to a range along each axis.
      • CircleBounds: Limits dual-axis values to a maximum magnitude.
    • Deadzones: Ignores near-zero values, treating them as zero.
      • Unscaled versions:
        • AxisExclusion: Excludes small single-axis values.
        • DualAxisExclusion: Excludes small dual-axis values along each axis.
        • CircleExclusion: Excludes dual-axis values below a specified magnitude threshold.
      • Scaled versions:
        • AxisDeadZone: Normalizes single-axis values based on AxisExclusion and AxisBounds::default.
        • DualAxisDeadZone: Normalizes dual-axis values based on DualAxisExclusion and DualAxisBounds::default.
        • CircleDeadZone: Normalizes dual-axis values based on CircleExclusion and CircleBounds::default.
  • App extensions: register_axis_processor and register_dual_axis_processor for registering processors in the bevy::prelude::App.
  • Procedual macro attribute: serde_typetag for adding type tags for processors.

TODO

  • Add new examples for processors.
  • Add new examples for inputs using processors.

Known Issues

  • Deserializing generic trait objects directly in Rust presents a current limitation. As a workaround, we need to define a corresponding concrete type for each desired value type used with the trait.

Changelog

  • Replace DualAxisShape with new deadzones.
  • Replace functions for inversion, sensitivity, and deadzone creation with processor constructors.

Internal changes

  • Add dependency to typetag, dyn_clone, dyn_eq, dyn_hash for boxed dyn trait objects, ensuring object-safety.
  • Refactor the logic of InputStreams::input_value to use these processors.

@Shute052 Shute052 marked this pull request as draft February 28, 2024 16:01
@Shute052 Shute052 added enhancement New feature or request usability Reduce user friction breaking-change A breaking change that requires a new major version labels Feb 28, 2024
@Shute052 Shute052 marked this pull request as ready for review February 28, 2024 18:13
@Shute052 Shute052 changed the title Add input processors and input settings Add processors and settings for axis-like inputs Feb 28, 2024
@Shute052
Copy link
Collaborator Author

Shute052 commented Mar 3, 2024

the rounded square deadzone seems to be a bit... dead

Copy link
Contributor

@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.

Generally quite pleased with this abstraction, and the builder pattern used! It makes a lot of sense to me. Code quality and docs are exceptional, you've done an awesome job there.

However:

  1. The pipeline work is super cool, but not needed for an MVP. I'm opposed to merging it in our initial implementation, and will require benchmarks to merge it in a followup. Simply storing a Vec<Box<dyn AxisProcessor>> is a fine design to start with.
  2. Some cleanup to do: the region comments should all be gone.
  3. Fewer macros. The Reflect for trait objects one can stay, but the others should be cut. They're relatively hard to maintain, and will be a serious barrier to upstreaming into Bevy.
  4. typetag cannot be used, flat out. It's incompatible with WASM builds, so we need to redesign to strip it out. See Support static constructors (ctors) rustwasm/wasm-bindgen#1216.

@alice-i-cecile
Copy link
Contributor

The processors still need to be removed from this PR. I also don't quite understand why all of the typetag stuff is needed in the first place. Is this just for serialization of processors? I would really like to avoid introducing the associated complexity and dependencies, and would frankly rather sacrifice the ability to store arbitrary preprocessor functions than add it.

I understand if that's not the path you'd prefer to take: I'm happy to adopt this excellent work and finish it up on my own if you'd like.

Copy link
Contributor

@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.

Okay, I'm satisfied now. To rehash from our conversation on Discord:

  1. Pipelines are much simpler now, and just store a Vec<Boxy dyn Processor>.
  2. We need typetag-style functionality for serialization of boxed trait objects. While we could move away from trait objects here, in favor of a finite set of operations, it won't actually trim complexity / dependencies in the end.

The case for using trait objects for input types is dramatically stronger, as is the need for serialization.

Merging!

@alice-i-cecile alice-i-cecile merged commit 7408157 into Leafwing-Studios:main Apr 12, 2024
4 checks passed
@Shute052 Shute052 deleted the input-processors branch April 12, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A breaking change that requires a new major version enhancement New feature or request usability Reduce user friction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sensitivity applied before deadzone for dual axis inputs
2 participants