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

feat(engine): allow multiple contacts between two entities #1457

Merged
merged 2 commits into from
Feb 16, 2025

Conversation

joaomanita
Copy link
Contributor

Description

Remove everything from CollidingWith except entity, add a vector of manifolds.
ContactManifold is no longer a relation.
Same thing for PenetrationConstraint, add a PenetrationtConstraints relation that holds a vector of PenetrationConstraint.

Change narrow phase systems to buildup the vector of manifolds in a collidingWith relation.
Change peneration constraint plugin to create the vector of PenetrationConstraint for each manifold.
Change penetraton constraint solver to buildup the effects of all constraints in PenetrationConstraints

Checklist

  • Self-review changes.
  • Evaluate impact on the documentation.
  • Ensure test coverage.
  • Write new samples.
  • Add entry to the changelog's unreleased section.

@joaomanita joaomanita requested review from RiscadoA, fallenatlas and a team as code owners February 6, 2025 20:49
@joaomanita joaomanita requested review from GCeSilva and removed request for a team February 6, 2025 20:49
@joaomanita joaomanita linked an issue Feb 6, 2025 that may be closed by this pull request
@joaomanita joaomanita requested review from SrGesus and removed request for a team February 6, 2025 20:49
@github-actions github-actions bot added A-Engine B-Collisions B-Physics P-Urgent This issue is a big priority, and it would be good to close it ASAP labels Feb 6, 2025
Copy link
Contributor

github-actions bot commented Feb 6, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://GameDevTecnico.github.io/cubos/preview/pr-1457/

Built to branch gh-pages at 2025-02-09 14:40 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@joaomanita joaomanita force-pushed the 1360-refactor-narrow-phase-collision-pipeline branch 2 times, most recently from 6959b32 to e710fd6 Compare February 6, 2025 21:12
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 2.07715% with 330 lines in your changes missing coverage. Please review.

Project coverage is 53.04%. Comparing base (05ae333) to head (ed6e846).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...c/physics/solver/penetration_constraint/plugin.cpp 0.00% 189 Missing ⚠️
engine/src/collisions/narrow_phase/plugin.cpp 2.98% 130 Missing ⚠️
...src/physics/constraints/penetration_constraint.cpp 0.00% 6 Missing ⚠️
engine/src/collisions/plugin.cpp 0.00% 3 Missing ⚠️
...e/include/cubos/engine/collisions/shapes/voxel.hpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1457      +/-   ##
==========================================
- Coverage   53.13%   53.04%   -0.09%     
==========================================
  Files         466      466              
  Lines       25980    25996      +16     
  Branches     2405     2413       +8     
==========================================
- Hits        13804    13790      -14     
- Misses      12176    12206      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

Copy link
Contributor

@fallenatlas fallenatlas left a comment

Choose a reason for hiding this comment

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

Thank you for working on this 🙏 Overall, looks pretty good to me, I just have a few comments, mostly regarding contact caching in this new way of doing things, and a few optimizations.

@joaomanita joaomanita force-pushed the 1360-refactor-narrow-phase-collision-pipeline branch from e710fd6 to 4f9f612 Compare February 7, 2025 15:50
@joaomanita joaomanita force-pushed the 1360-refactor-narrow-phase-collision-pipeline branch from 4f9f612 to 670ebf0 Compare February 7, 2025 16:37
Copy link
Member

@RiscadoA RiscadoA left a comment

Choose a reason for hiding this comment

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

Other than what I pointed out, LGTM! Ty for working on this, will be really nice to have this working 👀

Copy link
Contributor

@fallenatlas fallenatlas left a comment

Choose a reason for hiding this comment

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

Just a couple more things to point out. Also, since I already merged the warm-starting PR, you can rebase and the convert the new things to work with these, it should be just a couple changes in the penetration constraint plugin.

@joaomanita joaomanita force-pushed the 1360-refactor-narrow-phase-collision-pipeline branch from 670ebf0 to d38632d Compare February 9, 2025 14:38
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

@joaomanita joaomanita force-pushed the 1360-refactor-narrow-phase-collision-pipeline branch from d38632d to 69a97d7 Compare February 10, 2025 00:58
joaomanita and others added 2 commits February 11, 2025 13:45
narrow phase changes unfinished

more changes narrow phase

finished refactor

more changes
@joaomanita joaomanita force-pushed the 1360-refactor-narrow-phase-collision-pipeline branch from 69a97d7 to ed6e846 Compare February 11, 2025 13:45
Copy link
Contributor

@fallenatlas fallenatlas left a comment

Choose a reason for hiding this comment

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

Alright, LGTM! Good work 👍

@RiscadoA RiscadoA removed the request for review from SrGesus February 14, 2025 18:01
Copy link
Contributor

@GCeSilva GCeSilva left a comment

Choose a reason for hiding this comment

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

LGTM!

@joaomanita joaomanita merged commit 00c4a2a into main Feb 16, 2025
12 checks passed
@joaomanita joaomanita deleted the 1360-refactor-narrow-phase-collision-pipeline branch February 16, 2025 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Engine B-Collisions B-Physics P-Urgent This issue is a big priority, and it would be good to close it ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor narrow phase collision pipeline
4 participants