Skip to content

Adding a bezier curve example#8194

Merged
alice-i-cecile merged 11 commits intobevyengine:mainfrom
Kjolnyr:bezier-example
Apr 17, 2023
Merged

Adding a bezier curve example#8194
alice-i-cecile merged 11 commits intobevyengine:mainfrom
Kjolnyr:bezier-example

Conversation

@Kjolnyr
Copy link
Copy Markdown
Contributor

@Kjolnyr Kjolnyr commented Mar 24, 2023 •
edited
Loading

Objective

Examples on how to use the freshly merged Bezier struct ( #7653 ) are missing.

Solution

  • Added a bezier_curve.rs example in the animation/ folder.

@alice-i-cecile alice-i-cecile added C-Examples An addition or correction to our examples A-Animation Make things move and change over time A-Math Fundamental domain-agnostic mathematical operations labels Mar 24, 2023
tim-blackbird
tim-blackbird reviewed Mar 24, 2023
View reviewed changes
@Kjolnyr Kjolnyr requested a review from tim-blackbird March 24, 2023 17:11
@aevyrie
Copy link
Copy Markdown
Member

aevyrie commented Mar 24, 2023

Do we have any guidance on what level of features should get examples? I have a bunch of example code for cubic curves, but wasn't sure if it would be valuable outside of a higher-level context, e.g. general animation or shape rendering.

aevyrie
aevyrie suggested changes Mar 25, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

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

Some small changes, but otherwise looks good. Would it be more precise to call this cubic_curve? The techniques shown here can be applied to any of Bevy's supplied cubic curves, not just Beziers.

@Kjolnyr
Copy link
Copy Markdown
Contributor Author

Kjolnyr commented Mar 25, 2023

Changed the example name to cubic_curve, I've also changed the relevant comments to talk about cubic_curve instead of bezier.

@james7132 james7132 requested review from aevyrie and tim-blackbird and removed request for tim-blackbird March 28, 2023 19:50
aevyrie
aevyrie approved these changes Mar 29, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@aevyrie aevyrie 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 this is a nice addition as-is. I've included some nits, but wouldn't block on them.

examples/animation/cubic_curve.rs
Comment on lines +89 to +90
// position takes a point from the curve where 0 is the initial point
// and 1 is the last point
Copy link
Copy Markdown
Member

@aevyrie aevyrie Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd remove this comment to avoid duplicating the docs. I'd also prefer using t instead of step, as it is the standard input into a parametric function, and is what is used in the docs: https://docs.rs/bevy/latest/bevy/math/cubic_splines/struct.CubicCurve.html#method.position

As above, this is a pretty minor nit, and could be ignored.

Copy link
Copy Markdown
Contributor Author

@Kjolnyr Kjolnyr Mar 29, 2023

Choose a reason for hiding this comment

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

Good point for t instead of step, however, I'd keep the comment even though it's duplicating the docs to avoid any confusion the user would have.

Copy link
Copy Markdown
Member

@alice-i-cecile alice-i-cecile Apr 17, 2023

Choose a reason for hiding this comment

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

I very much like keeping the duplicative comment here :) The information is important and relevant here.

@hymm
Copy link
Copy Markdown
Contributor

hymm commented Apr 16, 2023

Should the cube be coming off the curve on the right side?
image

@aevyrie
Copy link
Copy Markdown
Member

aevyrie commented Apr 16, 2023

@hymm pretty sure it's this bug: #8049

hymm
hymm approved these changes Apr 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

pretty sure it's this bug: #8049

I merged in main and it fixed it. Thanks for the info.

@hymm hymm added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 16, 2023
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Apr 17, 2023 •
edited
Loading

Do we have any guidance on what level of features should get examples? I have a bunch of example code for cubic curves, but wasn't sure if it would be valuable outside of a higher-level context, e.g. general animation or shape rendering.

@aevyrie I would be happy to see these in bevy_math. If it has pedagogical value or can be used to quickly visually check correctness, it's worth including IMO. In this case, many users won't be familiar with the exact mechanisms of spline curves, which are extremely useful even outside of graphics and animation.

alice-i-cecile
alice-i-cecile approved these changes Apr 17, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@alice-i-cecile alice-i-cecile left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good: the debug line is particularly helpful! I'm going to push a fix up for CI; please re-review when you see it :)

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Apr 17, 2023

@Kjolnyr I couldn't get pushing to your branch to cooperate: can you remove the empty //! at the start of the example and run cargo fmt so CI will pass?

@Kjolnyr
Copy link
Copy Markdown
Contributor Author

Kjolnyr commented Apr 17, 2023

That's probably because I synced my fork with bevy's current main, sorry for that. I'll remove the empty //! and fix CI

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2023
Merged via the queue into bevyengine:main with commit cfa750a Apr 17, 2023
@Kjolnyr Kjolnyr deleted the bezier-example branch April 18, 2023 09:46
This was referenced Jun 28, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@tim-blackbird tim-blackbird Awaiting requested review from tim-blackbird

+2 more reviewers

@hymm hymm hymm approved these changes

@aevyrie aevyrie aevyrie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Animation Make things move and change over time A-Math Fundamental domain-agnostic mathematical operations C-Examples An addition or correction to our examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Kjolnyr @aevyrie @hymm @alice-i-cecile @tim-blackbird