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

Math primitives cleanup #13020

Merged
merged 12 commits into from
Apr 19, 2024
Merged

Math primitives cleanup #13020

merged 12 commits into from
Apr 19, 2024

Conversation

lynn-lumen
Copy link
Contributor

@lynn-lumen lynn-lumen commented Apr 18, 2024 •
edited
Loading

Objective

  • General clenup of the primitives in bevy_math
  • Add eccentricity() to Ellipse

Solution

  • Moved Bounded3d implementation for Triangle3d to the bounded module
  • Added eccentricity() to Ellipse
  • Ellipse::semi_major() and ::semi_minor() now accept &self instead of self
  • Triangle3d::is_degenerate() actually uses f32::EPSILON as documented
  • Added tests for Triangle3d-maths

@NthTensor NthTensor added C-Code-Quality A section of code that is hard to understand or change C-Testing A change that impacts how we test Bevy or how users test their apps A-Math Fundamental domain-agnostic mathematical operations labels Apr 18, 2024
@alice-i-cecile alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Apr 18, 2024
Jondolf
Jondolf reviewed Apr 18, 2024
View reviewed changes
Copy link
Contributor

@Jondolf Jondolf 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 overall, left a couple of nits

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
lynn-lumen and others added 2 commits April 18, 2024 19:22
@lynn-lumen @Jondolf
Improve eccentricity docs
1a151ea 
Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
@lynn-lumen @Jondolf
Use crate::Quat, add tests for eccentricity
6f86bc1 
Co-Authored-By: Joona Aalto <jondolf.dev@gmail.com>
@Jondolf Jondolf 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 18, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 18, 2024
NthTensor
NthTensor suggested changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

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

I'd like to hold off on this, as I'm not sure if the machine epsilon is appropriate. The original PR specifically moved away from this approach.

Otherwise looks great!

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
@NthTensor NthTensor removed 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 18, 2024
@lynn-lumen @NthTensor
Revert to custom epsilon
7bce78d 
Co-Authored-By: Miles Silberling-Cook <nth.tensor@gmail.com>
@alice-i-cecile alice-i-cecile 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 18, 2024
NthTensor
NthTensor suggested changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Still need to fix the docs so to have the correct epsilon.

@NthTensor NthTensor removed 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 18, 2024
NthTensor
NthTensor approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Tysm & Lgtm! Sorry to make you jump through hoops a bit.

@NthTensor NthTensor 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 18, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 18, 2024
Merged via the queue into bevyengine:main with commit cd80b10 Apr 19, 2024
28 checks passed
@aristaeus aristaeus mentioned this pull request May 22, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 23, 2024
@aristaeus @vitorfhc
Add triangle_math tests and fix Triangle3d::bounding_sphere bug (#13467)
bd5148e 
# Objective

Adopted #12659.

Resolved the merge conflicts on #12659;

* I merged the `triangle_tests` added by this PR and by #13020.
* I moved the [commented out
code](#12659 (comment))
from the original PR into a separate test with `#[should_panic]`.

---------

Co-authored-by: Vitor Falcao <vitorfhc@protonmail.com>
Co-authored-by: Ben Harper <ben@tukom.org>
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

@Jondolf Jondolf Jondolf approved these changes

@NthTensor NthTensor NthTensor approved these changes

Assignees
No one assigned
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Code-Quality A section of code that is hard to understand or change C-Testing A change that impacts how we test Bevy or how users test their apps C-Usability A targeted quality-of-life change that makes Bevy easier to use 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.
4 participants
@lynn-lumen @alice-i-cecile @NthTensor @Jondolf