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

Bullet: Fix detection of concave shape in Area #33690

Merged
merged 1 commit into from Feb 26, 2020

Conversation

@Phischermen
Copy link
Contributor

Phischermen commented Nov 18, 2019

Added a new property to the Area class that can make Areas perceive concave shapes. It is useful for jumping/crouching systems.

Bugsquad edit: Fixes #35854.

@Phischermen Phischermen requested review from AndreaCatania and godotengine/documentation as code owners Nov 18, 2019
@Phischermen Phischermen mentioned this pull request Nov 18, 2019
@Phischermen Phischermen force-pushed the Phischermen:area-enhancement branch from 223d10f to 7a024ba Nov 18, 2019
@AndreaCatania

This comment has been minimized.

Copy link
Member

AndreaCatania commented Nov 18, 2019

In the previous PR I miss read the purpose of this PR, but hopefully now I reviewed the code and everything is more clear.

However, the if (other_body_shape->isConcave()) was inserted to fix this issue #25419. Now that I'm re-checking this issue I'm realizing that I solved that issue not in the best way.
I don't remember exactly what the problem was with concave that make me add that IF statement; however, I would remove that IF, and try to solve the issue #25419 correctly.

Do you want to do it, (in case I'm available for help)? Otherwise I can give it a check.

@Phischermen

This comment has been minimized.

Copy link
Contributor Author

Phischermen commented Nov 19, 2019

Today I looked at the issue you referenced and downloaded the minimal reproduction project. I ran it with "perceive_concave" on and off, and in both cases it worked as I expected it to. I modified the project a little so that I could transform the area (which was using a cube shape) into the static body (which was using a concave shape). With perceive_concave off, the "body_entered" signal was not emitted. With perceive_concave on, the "body_entered" was emitted.

I haven't run into any instances where I received or did not receive the signal when I expected. I'm still working on setting up a stress test, to see if using multiple areas with perceive_concave on has a big impact on performance. I'll let you know what kind of results I get.

@AndreaCatania

This comment has been minimized.

Copy link
Member

AndreaCatania commented Nov 19, 2019

I'm still working on setting up a stress test, to see if using multiple areas with perceive_concave on has a big impact on performance. I'll let you know what kind of results I get.

I prefer to not have this option at all and rather allows the volume to emit a signal when it detects a concave.

Is strange that you can't reproduce the issue reported here #25419, so I'll re-try it and in case we can completely remove the if.

@Phischermen

This comment has been minimized.

Copy link
Contributor Author

Phischermen commented Nov 20, 2019

I prefer to not have this option at all and rather allows the volume to emit a signal when it detects a concave.

Gotcha! I'd be happy to strip the option and just enable detecting concave shapes - after I find out if I fixed my bug :)

I was able to set up a simple stress test, and my conclusion is that the bottle neck for areas is the number of overlapping shapes in an area, not necessarily what kind of shape the area is overlapping with. I think we could safely enable the detection of concave shapes without worrying about a performance impact.

@Phischermen Phischermen force-pushed the Phischermen:area-enhancement branch 2 times, most recently from 0d93770 to 9095905 Nov 21, 2019
@Phischermen

This comment has been minimized.

Copy link
Contributor Author

Phischermen commented Nov 21, 2019

I removed the property, and it is working correctly when set to Bullet Physics. It does not work in Godot Physics, but Godot Physics is somewhat deprecated from what I understand. Let me know if there's anything else I can do.

Here's the project I used for testing by the way:
AreaStressTest.zip

@akien-mga akien-mga added this to the 4.0 milestone Nov 22, 2019
@AndreaCatania

This comment has been minimized.

Copy link
Member

AndreaCatania commented Nov 23, 2019

Can be merged, @Phischermen Thanks!

Volume optimization

From time to time this is a recurrent issue that I work on, this is the last test that I did: https://github.com/AndreaCatania/godot/commits/area_optimization_try2

In this version, the collision information are taken directly from bullet , but turns out that is equally bad and sometimes slower.

The main problem is related to the collision algorithm used. Currently it's using GJK that returns a lot of information not required by the areas.

This algorithm is really slow when it checks two overlapping bodies, and use this algorithm for the volumes (that are supposed to always overlap things) is not optimal.

My next iteration will be change this algorithm, I just need some free time.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Feb 20, 2020

Could you amend the commit message to be less verbose and more explicit on the functional change, instead of describing the code diff? I'm not familiar with the physics engine's internal but it would likely be something like "Bullet: Fix detection of concave shape in Area".

@akien-mga akien-mga changed the title AreaEnhancement Bullet: Fix detection of concave shape in Area Feb 20, 2020
@Phischermen Phischermen force-pushed the Phischermen:area-enhancement branch from 9095905 to 2c01cf5 Feb 24, 2020
@Phischermen

This comment has been minimized.

Copy link
Contributor Author

Phischermen commented Feb 26, 2020

I amended the commit message. How's this:
"Enabled concave collision detection with Areas in Bullet"

@akien-mga akien-mga merged commit fde1211 into godotengine:master Feb 26, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Feb 26, 2020

Thanks!

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Mar 4, 2020

Cherry-picked for 3.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.