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.

Sign up for GitHub

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

Jump to bottom

Ensure CubicCurves with the default value have at least one CubicSegment #11211

Closed
wants to merge 1 commit into from
Closed

Ensure CubicCurves with the default value have at least one CubicSegment #11211

wants to merge 1 commit into from

Conversation

garychia
Copy link
Contributor

@garychia garychia commented Jan 4, 2024

Objective

Solution

  • The CubicCurve::position function assumes the given CubicCurve has at least one CubicSegment. However, a CubicCurve with the default value does not have any CubicSegment.
  • Implement the default function for CubicCurve which returns a CubicCurve with one default CubicSegment.

@matiqo15 matiqo15 added C-Bug An unexpected or incorrect behavior A-Math Fundamental domain-agnostic mathematical operations labels Jan 4, 2024
@hymm
Copy link
Contributor

hymm commented Jan 7, 2024

what does CubicCurve::default() return?

@garychia
Copy link
Contributor Author

garychia commented Jan 7, 2024

what does CubicCurve::default() return?

It returns a CubicCurve having one CubicSegment with a default value.

@mockersf
Copy link
Member

mockersf commented Jan 7, 2024

CubicCurve can't be updated once created... I would prefer to remove the Default impl as it doesn't make sense

@IQuick143
Copy link
Contributor

Yeah, I don't see CubicCurve::Default being useful, as far as I can read the code it produces a single point curve (all 4 control points are whatever Point::Default returns (I assume [0;0]). Is there a trait bound somewhere that requires this impl?

@BD103 BD103 mentioned this pull request Jan 13, 2024
Merged
@alice-i-cecile
Copy link
Member

Closing in favor of #11335

github-merge-queue bot pushed a commit that referenced this pull request Jan 14, 2024
@BD103
Remove Default impl for CubicCurve (#11335)
6f6269e 
# Objective

- Implementing `Default` for
[`CubicCurve`](https://docs.rs/bevy/latest/bevy/math/cubic_splines/struct.CubicCurve.html)
does not make sense because it cannot be mutated after creation.
- Closes #11209.
- Alternative to #11211.

## Solution

- Remove `Default` from `CubicCurve`'s derive statement.

Based off of @mockersf comment
(#11211 (comment)):

> CubicCurve can't be updated once created... I would prefer to remove
the Default impl as it doesn't make sense

---

## Changelog

- Removed the `Default` implementation for `CubicCurve`.

## Migration Guide

- Remove `CubicCurve` from any structs that implement `Default`.
- Wrap `CubicCurve` in a new type and provide your own default.

```rust
#[derive(Deref)]
struct MyCubicCurve<P: Point>(pub CubicCurve<P>);

impl Default for MyCubicCurve<Vec2> {
    fn default() -> Self {
        let points = [[
            vec2(-1.0, -20.0),
            vec2(3.0, 2.0),
            vec2(5.0, 3.0),
            vec2(9.0, 8.0),
        ]];

        Self(CubicBezier::new(points).to_curve())
    }
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
6 participants
@garychia @hymm @mockersf @IQuick143 @alice-i-cecile @matiqo15