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

Fix CubicCurve::iter_samples iteration count #8049

Merged
merged 1 commit into from
Mar 31, 2023
Merged

Conversation

jannik4
Copy link
Contributor

@jannik4 jannik4 commented Mar 12, 2023

Objective

Fix CubicCurve::iter_samples iteration count.

Solution

If I understand the function and the docs correctly, this should iterate over 0..=subdivisions instead of 0..subdivisions.
For example: Now the iteration returns 3 points at subdivisions = 2, as indicated in the documentation.

@github-actions
Copy link
Contributor

Welcome, new contributor!

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

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Math Fundamental domain-agnostic mathematical operations labels Mar 12, 2023
@JMS55 JMS55 added this to the 0.10.1 milestone Mar 12, 2023
@james7132 james7132 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 Mar 16, 2023
@cart
Copy link
Member

cart commented Mar 27, 2023

I'm not sure the docs are right here. The docs essentially say that "subdivisions" is the count of the number of "sections". However in my experience "subdivisions" is a count of the number of dividing lines. 1 subdivision = 2 sections, 2 subdivisions = 3 sections. Blender avoids this by using "number of cuts" terminology instead of "subdivisions". Godot calls this "subdivisions". We also call this "subdivisions" for the Plane shape.

Imo we should either:

  1. Use different terminology for this curve functionality : sections, samples (although this is confusing because it is unclear whether the start and end are included), number of cuts, etc
  2. Synchronize the "subdivision" behavior everywhere in the engine. I believe it should be "number of cuts".

@jannik4
Copy link
Contributor Author

jannik4 commented Mar 28, 2023

I agree that the terminology may be unclear and may need to be aligned with other parts from bevy.

However, I still think this PR has value in the short term regardless, since currently all methods like iter_positions do not include the endpoint, which is definitely wrong.

@cart
Copy link
Member

cart commented Mar 31, 2023

Ok I buy that!

@cart cart added this pull request to the merge queue Mar 31, 2023
Merged via the queue into bevyengine:main with commit f201a9d Mar 31, 2023
cart pushed a commit that referenced this pull request Mar 31, 2023
# Objective

Fix `CubicCurve::iter_samples` iteration count.

## Solution

If I understand the function and the docs correctly, this should iterate
over `0..=subdivisions` instead of `0..subdivisions`.
For example: Now the iteration returns 3 points at `subdivisions = 2`,
as indicated in the documentation.
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-Bug An unexpected or incorrect behavior 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.

6 participants