Skip to content

Fix #771 by replacing collision-tests with shapely #788

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

Merged

Conversation

akapkotel
Copy link
Contributor

  1. Replaced all geometry collision test with shapely functions.
  2. Replaced math.sqrt function with math.hypot in get_distance function because hypot is faster - see math module documentation.

Additionally:
3. Removed some redundant local assignments in sprite_list.py functions with straightforward returns.

Enhancement request:

Replacement current collision-test functions with shapely library geometry functions.

What would it help with?

Faster collision tests than solution proposed in #776 but still solves issue in #771 since shapely library works fine with concave polygons.
I've already tested both solutions - mine and @LiorAvrahami - with a concave polygons and time-performance of shapely is up tu 3 times better (but, unfortuunatelly still slower than original polygons-collisions test, still the original algorithm did not work for concave poly's, so it's speed was artificial).

Where is it located?

arcade.geometry.py, arcade.sprite_list,py

…y_to_find_line_of_sight

1. Replaced all geometry collision test with shapely functions.
2. Replaced math.sqrt function with math.hypot in get_distance function because hypot is faster - see math module documentation.
3. Removed some redundant local assignments in sprite_list.py functions with straightforward returns.
@SirGnip
Copy link
Contributor

SirGnip commented Nov 10, 2020

I don't have a lot of context on this issue and I know nothing about shapely, but wanted to as a question. It is obviously good to have a solution that handles concave poly collisions. But, this means that convex polygon collisions are now slower? Would it be worth having two different collision algorithms available? One that handles convex and concave polys (using shapely library) and another that handles only convex polys (will be faster for this specific case). That way, if someone knows they only need convex collision testing, they can use the original algorithm and not pay the speed penalty for their simpler use case. The shapely algorithm could be the "default". Then, for those users who need the optimizations, they could do something different (call a different function, pass an argument, etc.) to use the original algorithm? Just brainstorming...

@akapkotel
Copy link
Contributor Author

@SirGnip your idea sounds great. Not-loosing old-concave algorithm speed seems being worth of additional work. We only need a cheap method of finding if a polygon is convex to fast-detect which case are we dealing with. It could be a counter-clockwise points order check maybe?

@pvcraven
Copy link
Member

I need to redo benchmarks, but my initial tests (not and all thorough) was shapely was faster than anything in Arcade currently.

@akapkotel
Copy link
Contributor Author

@pvcraven how do you benchmark? It seems that your results are always different than mine, so I am curious about how do you profile code execution.
I use my own custom function to decorate profiled functions and enclose them with time.perf_counter calls.

@pvcraven
Copy link
Member

I have some custom code here I like to use:

https://github.com/pythonarcade/performance_tests

@pvcraven pvcraven merged commit 3051413 into pythonarcade:development Nov 16, 2020
@pvcraven
Copy link
Member

I carelessly merged this in, now the unit tests are getting some errors around collision and line-of-sight. Hoping I'll be getting some time to hack this out and clean up unit tests.

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