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

💥 Add collisions methods #9

Merged
merged 24 commits into from
Oct 10, 2024
Merged

Conversation

landryarki
Copy link
Collaborator

collisions methods to collide boxes and spheres

landryarki and others added 5 commits September 25, 2024 17:56
BoxVsBox
BoxVsSphere
SphereVsBox
SphereVsSphere
…methods

🧑‍🚀 Physics engine utils: movement methods
negative size object are now consider object with a positive size but in the opposite direction
Copy link

linear bot commented Sep 27, 2024

Copy link
Collaborator

@Marius-P1 Marius-P1 left a comment

Choose a reason for hiding this comment

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

Sounds good to me, but I'm not a fan of the "VS" naming 👀

rename from "ShapeVsShape" to "ShapeCollideShape"
@landryarki landryarki marked this pull request as ready for review September 30, 2024 12:48
@Marius-P1 Marius-P1 changed the title Feature/epi 46 collisions methods 💥 Add collisions methods Sep 30, 2024
Copy link
Collaborator

@AubaneNourry AubaneNourry left a comment

Choose a reason for hiding this comment

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

I agree with the functions but not sure about the use,
We will add components like RectCollider, RoundCollider that will use these functions in their inner logic and will refer to a Transform Component to get the position / dimensions of these geometrics elements?

Update CI and good for me :)

Copy link
Collaborator

@6im0n 6im0n left a comment

Choose a reason for hiding this comment

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

Good for me ! 👍

Copy link
Member

@RenardFute RenardFute left a comment

Choose a reason for hiding this comment

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

I'm not going to approve or disapprove this.
I believe what you did is working, there is no problem with that but it looks like your system is really 'rigid'. What I mean by that is, that whenever we will want to add another X collide with Y we will need to update Physics::Collision class.
My idea would have been to make an ICollider interfaces that would provide some way of mathematically know wether it's colliding with something.
The result code in Physics::Collision would look like this

bool isColliding(const ICollider &a, const IColllider &b) {
  const aEquation = a.getCollisionEquation();
  const bEquation = b.getCollisionEquation();
  const mixed = // some math stuff
  return // math result
}

This is all theoretical but I believe, if possible, a system like this would be better.

Maybe SDF ??

@landryarki landryarki force-pushed the feature/epi-46-collisions-methods branch from de305d7 to 5734f35 Compare October 7, 2024 12:11
ICollider, box and sphere are now on a fille Collider.hpp/cpp
Copy link
Collaborator

@Marius-P1 Marius-P1 left a comment

Choose a reason for hiding this comment

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

👌

Copy link
Member

@RenardFute RenardFute left a comment

Choose a reason for hiding this comment

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

Can you just split your classes like don't put Sphere and Box Collider in the same file. Same for Physics.hpp

remove collider cpp since it was basically the code for boxes and shperes but keeped the hpp with the interfact og a collider
@landryarki landryarki merged commit 83764e9 into develop Oct 10, 2024
2 of 3 checks passed
@landryarki landryarki deleted the feature/epi-46-collisions-methods branch October 10, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants