Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Реализация FixCollisions, работающая за O(n log n) #43

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

danilasar
Copy link

Предложенная в описании задачи реализация с for-each симпатична внешне, но, на мой взгляд, не является наиболее оптимальной. Я написал перебор двумя указателями. Чтобы избежать необходимости перебирать все obj2, для тех obj2, что имеют динамические коллайдеры, я дополнительно выполняю ещё один SolveCollision.

Если можно, я могу ещё попробовать сделать циклы асинхронными с помощью std::async.

@mchernigin
Copy link
Member

mchernigin commented Jun 1, 2024

Тот факт, что внутренний цикл начинает ход не с начала, а со следующего элемента, не говорит о том что асимптотика становится $nlog(n)$ — это всё ещё $n^2$. Иначе ты утверждаешь, что сортировка пузырьком работает за $nlog(n)$ :)

user.cpp Outdated Show resolved Hide resolved
Copy link
Member

@vasthecat vasthecat left a comment

Choose a reason for hiding this comment

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

Оптимизация проверки коллизий достигается полным изменением алгоритма, а не изменением условий циклов и добавлением async.

Изменение условий цикла делает такое решение некорректным. Добавление async может сказаться негативно на производительности решения. Для добавления асинхронности необходимо сначала протестировать, что оверхед действительно не замедляет решение.

Для действительного ускорения определения коллизий придётся реализовать подход sweep and prune, что не входит в предлагаемое решение и не даст достаточного ускорения. Насколько подходит данный метод для этого проекта также придётся тестировать на производительность.

Я предполагаю, что подходы кроме полного перебора в данном проекте либо дают некорректный результат, либо излишни.

user.cpp Outdated Show resolved Hide resolved
user.cpp Outdated Show resolved Hide resolved
user.cpp Outdated Show resolved Hide resolved
user.cpp Outdated Show resolved Hide resolved
@danilasar
Copy link
Author

Спасибо за замечания и указания на ошибки. Коммит #879a567 исправляет поведение в реализации с итераторами, когда статический объект идёт прежде динамического.

Как заставить for-each работать с итераторами, я, к сожалению, не разобрался. Но, если нужно, могу полностью отказаться от итераторов и переписать на for-each.

@danilasar danilasar requested review from mchernigin and vasthecat June 2, 2024 05:13
Copy link
Member

@vasthecat vasthecat left a comment

Choose a reason for hiding this comment

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

LGTM

@mchernigin mchernigin requested a review from vasthecat June 4, 2024 19:04
Copy link
Member

@mchernigin mchernigin left a comment

Choose a reason for hiding this comment

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

Посмотри комментарий над функцией ещё раз. Там очень хорошо и достаточно подробно описано всё решение. Текущая реализация попросту не работает.

@@ -121,6 +121,26 @@ void SolveCollision(Object &obj, Collision c, float dt)
//
void FixCollisions(Scene &scene, float dt)
{
bool (*checker)(Object&) { [](Object &obj) {
Copy link
Member

Choose a reason for hiding this comment

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

Давай избавимся от checker, из-за него логика становится очень запутанная и, как оказалась, неправильная.

|| obj.collider.of_type(ColliderType::STATIC));
} };
for(auto &obj1 : scene) {
if(!checker(obj1)) {
Copy link
Member

Choose a reason for hiding this comment

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

В отличии от checker, здесь obj1 должен быть строго DYNMIC.

continue;
}
for(auto &obj2 : scene) {
if(!checker(obj2) || (obj2.collider.of_type(ColliderType::STATIC) == obj1.collider.of_type(ColliderType::STATIC))) {
Copy link
Member

Choose a reason for hiding this comment

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

Тут получается очень запутанное условие. Должны проверяться 3 вещи:

  1. obj2 имеет коллайдер.
  2. obj2 это не obj1.
  3. obj2 это STATIC или DYNAMIC, то есть объект, в который можно врезаться.

}
auto collision = CheckCollision(obj1, obj2);
SolveCollision(obj1, collision, dt);
if(obj2.collider.of_type(ColliderType::DYNAMIC)) {
Copy link
Member

Choose a reason for hiding this comment

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

Так как сейчас ты проходишься по всем парам, нет необходимости в вызове SolveCollision дважды и наличии этого условия вообще. По крайней мере, если рассматривать условия, которые я описал в комментариях выше.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(FixCollisions) Решение всех коллизий в сцене
3 participants