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

Adding a bezier curve example #8194

Merged
merged 11 commits into from
Apr 17, 2023
Merged

Conversation

Kjolnyr
Copy link
Contributor

@Kjolnyr Kjolnyr commented Mar 24, 2023

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
Co-authored-by: ira <JustTheCoolDude@gmail.com>
@Kjolnyr Kjolnyr requested a review from tim-blackbird March 24, 2023 17:11
@aevyrie
Copy link
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.

Copy link
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.

examples/animation/bezier_curve.rs Outdated Show resolved Hide resolved
examples/animation/bezier_curve.rs Outdated Show resolved Hide resolved
@Kjolnyr
Copy link
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
Copy link
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 Outdated Show resolved Hide resolved
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
Member

@aevyrie aevyrie Mar 29, 2023

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
Contributor Author

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
Member

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
Contributor

hymm commented Apr 16, 2023

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

@aevyrie
Copy link
Member

aevyrie commented Apr 16, 2023

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

Copy link
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
Member

alice-i-cecile commented Apr 17, 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 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.

Copy link
Member

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

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
Member

@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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants