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

refactor collide code #4485

Closed
wants to merge 1 commit into from
Closed

refactor collide code #4485

wants to merge 1 commit into from

Conversation

dbofmmbt
Copy link

@dbofmmbt dbofmmbt commented Apr 15, 2022 •
edited
Loading

Just a refactor.

Objective

Improve collide code.

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, feel free to skip this section.

  • What changed as a result of this PR?

The code was refactored and tests were added.

Added

Collision now implements PartialEq.

I needed to add this trait impl to be able to test collide, hope you don't mind.

improve collide handling
c48066b 
bug fix + refactor. Closes bevyengine#4478
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 15, 2022
@dbofmmbt dbofmmbt marked this pull request as draft April 15, 2022 16:11
@rparrett
Copy link
Contributor

Cool to see this function getting some tests.

With the refactor in the same commit, it's difficult to understand what the fix is. Could you split that up, perhaps?

@dbofmmbt dbofmmbt marked this pull request as ready for review April 15, 2022 16:34
@dbofmmbt
Copy link
Author

dbofmmbt commented Apr 15, 2022 •
edited
Loading

@rparrett that's exactly why I have put it in draft... Someone fixed it already 😬 here's their PR: #2489. I noticed it when I was reviewing my changes. So this is only a refactor now, with the addition of PartialEq to Collision.

I also added a sentence to the documentation about the meaning of Collision on collide because it took me a while to understand that the collision side was in function of b and not a 😅.

@dbofmmbt dbofmmbt changed the title improve collide handling refactor collide code Apr 15, 2022
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Math Fundamental domain-agnostic mathematical operations and removed S-Needs-Triage This issue needs to be labelled labels Apr 16, 2022
alice-i-cecile
alice-i-cecile approved these changes Apr 17, 2022
View reviewed changes
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.

Those diagrams are great. I love this.

@mnmaita
Copy link
Member

mnmaita commented May 19, 2023

@alice-i-cecile is this post relicencing? Can we somehow resolve conflicts and incorporate this? Seems like a good, "low hanging fruit", addition.

cc @dbofmmbt

@alice-i-cecile
Copy link
Member

Yep, this is post relicensing :) Agreed on the adoption.

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label May 20, 2023
@tygyh tygyh mentioned this pull request Dec 27, 2023
Merged
@alice-i-cecile
Copy link
Member

Adopted in the PR linked above.

github-merge-queue bot pushed a commit that referenced this pull request Jan 1, 2024
@tygyh
refactor collide code (Adopted) (#11106)
55c9958 
# Objective

- Refactor collide code and add tests.

## Solution

- Rebase the changes made in #4485.

Co-authored-by: Eduardo Canellas de Oliveira <eduardo.canellas@bemobi.com>
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

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 S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dbofmmbt @rparrett @mnmaita @alice-i-cecile