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: Add broadphase generics to CollisionDetection #1908

Merged
merged 1 commit into from
Sep 17, 2022

Conversation

spydon
Copy link
Member

@spydon spydon commented Sep 15, 2022

Description

This adds generics for the broadphase used in the collision detection.
This should simplify things a bit in #1894.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

@spydon spydon mentioned this pull request Sep 15, 2022
7 tasks
@spydon
Copy link
Member Author

spydon commented Sep 15, 2022

@ASGAlex

@ASGAlex
Copy link
Contributor

ASGAlex commented Sep 16, 2022

Since we're talking about breaking changes...

To be honest, all these generics in collision detection system looks overcomplicated. The <T extends Hitbox<T>> is specified everywhere, but in real practice only the ShapeHitbox makes sense. Why should't we to use this base class instead generic type? If to specify ShapeHitbox explicitly in the Broadphase and CollisionDetection classes, then we will not need to pass these generics everywhere. And it would allow to avoid unnecesary type conversions at many places.

As I can see, there are no hitboxes at the Flame which are not extended from ShapeHitbox. I also tried to replace all generics with basic types - ShapeHitbox and Broadphase - and everithing works well, at least in examples.

@spydon
Copy link
Member Author

spydon commented Sep 16, 2022

Since we're talking about breaking changes...

To be honest, all these generics in collision detection system looks overcomplicated. The <T extends Hitbox<T>> is specified everywhere, but in real practice only the ShapeHitbox makes sense. Why should't we to use this base class instead generic type? If to specify ShapeHitbox explicitly in the Broadphase and CollisionDetection classes, then we will not need to pass these generics everywhere. And it would allow to avoid unnecesary type conversions at many places.

As I can see, there are no hitboxes at the Flame which are not extended from ShapeHitbox. I also tried to replace all generics with basic types - ShapeHitbox and Broadphase - and everithing works well, at least in examples.

This is because the collision detection system is fairly new, if we would only be able to use ShapeHitbox then the system is narrowed down to only be able to be used within FCS, if someone would want to have collision detection and use for example Oxygen they can't have their hitboxes extend from ShapeHitbox. But there are changes coming to that in the future, where we'll use pure shapes as hitboxes (wrapped in a component where needed) instead of having them directly components as it is today, but that is a much larger feat and not related to this PR.

@spydon spydon requested review from st-pasha and a team September 16, 2022 07:46
@ASGAlex
Copy link
Contributor

ASGAlex commented Sep 16, 2022

But there are changes coming to that in the future

Nice! Hope someday it will be reworked because with adding more dependencies we aggravates this "hell of generics" :-)

As about this PR - i merged it into my branch, changes really helps to make life easier for end-user. Some previous type conversions become unnecesary. So I strongly confirm this PR usefulness 👍🏻

@spydon spydon merged commit f771412 into main Sep 17, 2022
@spydon spydon deleted the spydon/broadphase-generics branch September 17, 2022 03:32
ASGAlex added a commit to ASGAlex/flame that referenced this pull request Sep 17, 2022
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.

3 participants