Skip to content

Fix CubicCurve::iter_samples iteration count#8049

Merged
cart merged 1 commit intobevyengine:mainfrom
jannik4:main
Mar 31, 2023
Merged

Fix CubicCurve::iter_samples iteration count#8049
cart merged 1 commit intobevyengine:mainfrom
jannik4:main

Conversation

@jannik4
Copy link
Copy Markdown
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
Copy Markdown
Contributor

github-actions bot commented Mar 12, 2023

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 requested a review from aevyrie March 12, 2023 02:32
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@jannik4 @cart
Fix CubicCurve::iter_samples iteration count (#8049)
2bc7140 
# 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.
@aevyrie aevyrie mentioned this pull request Apr 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cart cart cart approved these changes

@james7132 james7132 james7132 approved these changes

+1 more reviewer

@aevyrie aevyrie aevyrie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

0.10.1

Development

Successfully merging this pull request may close these issues.

6 participants

@jannik4 @cart @aevyrie @james7132 @alice-i-cecile @JMS55