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: quad tree broadphase support #1894

Merged
merged 75 commits into from
Sep 27, 2022
Merged

Conversation

ASGAlex
Copy link
Contributor

@ASGAlex ASGAlex commented Sep 9, 2022

Description

Quad tree broadphase support.

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.

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Very good job!
Tests are missing, but I guess that might have been intentional so that you could show the API first.

doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Show resolved Hide resolved
doc/flame/collision_detection.md Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
packages/flame/lib/src/collisions/quadtree/quadtree.dart Outdated Show resolved Hide resolved
packages/flame/lib/src/collisions/quadtree/quadtree.dart Outdated Show resolved Hide resolved
packages/flame/lib/src/collisions/quadtree/quadtree.dart Outdated Show resolved Hide resolved
packages/flame/lib/src/collisions/quadtree/quadtree.dart Outdated Show resolved Hide resolved
@ASGAlex
Copy link
Contributor Author

ASGAlex commented Sep 22, 2022

I have done with tests - looks like old tests are reused correctly 🎉

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Some very minor last comments.
You've done an amazing job! 🎉

doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
doc/flame/collision_detection.md Outdated Show resolved Hide resolved
packages/flame/test/collisions/collision_test_helpers.dart Outdated Show resolved Hide resolved
@spydon spydon requested review from st-pasha and a team September 23, 2022 15:19
@spydon spydon requested a review from a team September 25, 2022 12:26
@spydon
Copy link
Member

spydon commented Sep 27, 2022

@ASGAlex if you can just fix the merge conflict this PR is ready to be merged! 😍

@ASGAlex
Copy link
Contributor Author

ASGAlex commented Sep 27, 2022

15 minutes 👌

# Conflicts:
#	packages/flame/test/collisions/collision_callback_test.dart
@spydon spydon enabled auto-merge (squash) September 27, 2022 11:34
auto-merge was automatically disabled September 27, 2022 11:39

Head branch was pushed to by a user without write access

@ASGAlex
Copy link
Contributor Author

ASGAlex commented Sep 27, 2022

@spydon looks like done =^_^=

@spydon spydon merged commit e33d541 into flame-engine:main Sep 27, 2022
@spydon
Copy link
Member

spydon commented Sep 27, 2022

@spydon looks like done =^_^=

Great job! I think it ended up quite nice :)

@max-zamkovoy
Copy link

@ASGAlex congrats! You did great job! :)

spydon pushed a commit that referenced this pull request Sep 27, 2022
…1943)

A variant of refactoring mentioned here #1894 (comment)
Sweep broadphase have no any additional specific classes, it's based only on core classes. So it is discussion question, do we need to keep int in separate folder (to avoid mixing with core broadphase.dart file) or just leave it in the root folder 'broadphase'.
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.

4 participants