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

Improve cubic segment bezier functionality #17645

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

hocop
Copy link

@hocop hocop commented Feb 2, 2025

Objective

Solution

  • Implemented method new_bezier(points: [P; 4]) -> Self for CubicSegment<P>
  • Old implementation of new_bezier is now new_bezier_easing(p1: impl Into<Vec2>, p2: impl Into<Vec2>) -> Self (breaking change)
  • added method new_bezier_with_anchor, which can make a bezier curve between two points with one control anchor
  • added methods iter_positions, iter_velocities, iter_accelerations, the same as in CubicCurve (copied code, potentially can be reduced)
  • bezier creation logic is moved from CubicCurve to CubicSegment, removing the unneeded allocation

Testing

  • Did you test these changes? If so, how?
    • Run tests inside crates/bevy_math/
    • Tested the functionality in my project
  • Are there any parts that need more testing?
    • Did not run cargo test on the whole bevy directory because of OOM
    • Performance improvements are expected when creating CubicCurve with new_bezier and new_bezier_easing, but not tested
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
    • Use in any code that works created CubicCurve::new_bezier
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?
    • I don't think relevant

Showcase

// Imagine a car goes towards a local target

// Create a simple `CubicSegment`, without using heap
let planned_path = CubicSegment::new_bezier([
    car_pos,
    car_pos + car_dir * turn_radius,
    target_point - target_dir * turn_radius,
    target_point,
]);

// Check if the planned path itersect other entities
for pos in planned_path.iter_positions(8) {
   // do some collision checks
}

Migration Guide

This section is optional. If there are no breaking changes, you can delete this section.

  • Replace CubicCurve::new_bezier with CubicCurve::new_bezier_easing

Copy link
Contributor

github-actions bot commented Feb 2, 2025

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

crates/bevy_math/src/cubic_splines/mod.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/cubic_splines/mod.rs Show resolved Hide resolved
Comment on lines +1045 to +1075
#[inline]
pub fn iter_samples<'a, 'b: 'a>(
&'b self,
subdivisions: usize,
mut sample_function: impl FnMut(&Self, f32) -> P + 'a,
) -> impl Iterator<Item = P> + 'a {
self.iter_uniformly(subdivisions)
.map(move |t| sample_function(self, t))
}

/// An iterator that returns values of `t` uniformly spaced over `0..=subdivisions`.
#[inline]
fn iter_uniformly(&self, subdivisions: usize) -> impl Iterator<Item = f32> {
let step = 1.0 / subdivisions as f32;
(0..=subdivisions).map(move |i| i as f32 * step)
}

/// Iterate over the curve split into `subdivisions`, sampling the position at each step.
pub fn iter_positions(&self, subdivisions: usize) -> impl Iterator<Item = P> + '_ {
self.iter_samples(subdivisions, Self::position)
}

/// Iterate over the curve split into `subdivisions`, sampling the velocity at each step.
pub fn iter_velocities(&self, subdivisions: usize) -> impl Iterator<Item = P> + '_ {
self.iter_samples(subdivisions, Self::velocity)
}

/// Iterate over the curve split into `subdivisions`, sampling the acceleration at each step.
pub fn iter_accelerations(&self, subdivisions: usize) -> impl Iterator<Item = P> + '_ {
self.iter_samples(subdivisions, Self::acceleration)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of the code duplication, but I guess that's kind of unavoidable 🤔

I think there's a reasonable shot that all of these will be outmoded by the Curve API implementations in the future, but for now it's more ergonomic, I guess.

(E.g. one right now can write something like

cubic_segment.with_two_derivatives().map(|j| j.second_derivative).samples(n)

to similar effect, and the intention is for this to become more ergonomic in the near future.)

Copy link
Author

Choose a reason for hiding this comment

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

Maybe it's better to move these functions to a separate trait, and implement it on both CubicCurve and CubicSegment. This can help avoid some duplication. What do you think?

@mweatherley
Copy link
Contributor

You can delete the entire cfg_attr block at like 1016 if you want CI to stop complaining :)

Copy link
Contributor

@Carter0 Carter0 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 code and doc comments look good. You might want to add some labels to your PR though if you can. Sometimes people skip over PR's if they don't have labels.

@hocop hocop requested a review from mweatherley February 6, 2025 18:52
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Feature A new feature, making something new possible S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CubicSegment lacks functionality of CubicCurve
4 participants