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

Areas don't detect static bodies #391

Closed
timshannon opened this issue May 23, 2023 · 15 comments · Fixed by #460
Closed

Areas don't detect static bodies #391

timshannon opened this issue May 23, 2023 · 15 comments · Fixed by #460
Labels
bug Something that isn't working as intended severity:minor Small impact on functionality or usability topic:runtime Concerning runtime behavior (or its source code)

Comments

@timshannon
Copy link

Areas don't detect static bodies. Areas work fine for character bodies, and rigid bodies, but not static bodies.

See the attached project, or simply create an area that overlaps with a static body and check get_overlapping_bodies or has_overlapping_bodies.

bug.zip

The attached project is C#, but I can probably throw together a GD script one if needed.

@mihe mihe added bug Something that isn't working as intended topic:runtime Concerning runtime behavior (or its source code) severity:minor Small impact on functionality or usability labels May 23, 2023
@mihe
Copy link
Contributor

mihe commented May 23, 2023

To quote Jolt's documentation on sensors, which are the thing backing Area3D in Godot Jolt:

Sensors are normal rigid bodies that report contacts with other Dynamic or Kinematic bodies through the ContactListener interface.

So them not detecting static bodies is by design. Judging by the reply here it's done for performance reasons.

I was under the impression that this was a limitation in Godot Physics as well, but when looking more closely it seems that was only an issue when the "Monitorable" property was disabled, as seen here. I'll make sure to document this discrepancy.

As for a workaround, I suppose you could do what was suggested in that Jolt discussion and just do an intersect_shape yourself using the area's shape(s), although you won't be able to do that from C# right now, due to #270. Alternatively you could maybe create some sort of "ghost" kinematic body, using a dedicated collision layer, child that to your static object and use it as a sort of proxy body. I'm sure there are other interesting ways of going about it as well.

@mihe mihe closed this as not planned Won't fix, can't repro, duplicate, stale May 23, 2023
@mihe mihe added the wontfix This will not be worked on label May 23, 2023
@Zireael07
Copy link

This should be pinned and made extra clear in documentation. (README doesn't make it clear that this is Jolt's by design and I came here to request a fix - I rely on Area3D to know whether I should fire off raycasts for more detailed collision detection or not)

@mihe
Copy link
Contributor

mihe commented May 30, 2023

and made extra clear in documentation

I bumped it up to the list of unsupported features instead.

I rely on Area3D to know whether I should fire off raycasts for more detailed collision detection or not

An initial intersect_shape sounds like it might serve as a reasonable workaround then, albeit not as convenient I'll admit.

While I haven't taken the time to explore this yet, this limitation in Jolt is potentially nothing more than an if-statement away from being supported, and could theoretically be forked to accommodate for this feature, if deemed too much of a footgun for upstream Jolt. In fact, we've already had a similar discussion and fix for allowing areas to detect other areas.

An important difference between these two is that Area3D has the ability to disable (on a per-area basis) whether they want other areas to be able to collide with them, through the monitorable property. So it's at least technically possible to opt out of that, or even make it opt-in by extending Area3D and having monitorable be disabled by default instead, if you find yourself in performance trouble.

Controlling collision between areas and static bodies would only be possible globally through some potential project setting, and seeing as how areas are likely put to use as triggers around levels, potentially in great numbers, and likely intersecting with the static level geometry, a global setting like this starts to feel like a dangerous rabbit hole that you can't reasonably climb out of once you've found yourself in trouble. Sufficiently clear documentation might be a counter to that though, assuming people read it.

Sorry to be lighting the bat-signal so soon again, but I'd love to hear your thoughts on this as well @jrouwe.

(It's probably relevant to point out that I run my sensors as kinematic, and with mUseManifoldReduction disabled.)

Without any numbers to back this up, and this being way outside my expertise, I suspect that even enabling this functionality globally without a setting to disable it would still put us in a better spot than Godot Physics in terms of performance, even if it's technically not making the most out of Jolt. I find this to be a difficult thing to balance though.

@mihe
Copy link
Contributor

mihe commented May 30, 2023

Controlling collision between areas and static bodies would only be possible globally through some potential project setting

Although, I guess collision layers/masks could help with this as well, now that I think about it. I was thinking more in terms of having this global setting changing the broadphase layer matrix, and that being the important factor, but maybe as far as bloating the contact cache is concerned it's equally as effective to cull at the object layer level?

@jrouwe
Copy link
Contributor

jrouwe commented May 31, 2023

Bat man responding, 😄

I took a quick look and made this: https://github.com/jrouwe/JoltPhysics/tree/feature/sensors_vs_static

If you start the SensorTest you see that the 'KinematicSensor' will start detecting the floor. As you can see it's quite a simple fix (I didn't test it very well though so it might be buggy) but I'm very worried about the performance implications. Sensors in general tend to be quite large so I think this will end up wasting a lot of CPU cycles and it's not easy to spot that you're burning all these cycles because the code will probably ignore all collisions that its not interested in. As suggested in the other ticket, I would be more in favor of solving this by doing explicit collision checks to avoid polluting the contact cache with this information (could this be done in Godot Jolt's Area3D implementation?).

@mihe
Copy link
Contributor

mihe commented May 31, 2023

could this be done in Godot Jolt's Area3D implementation?

Yes, I believe so. In fact, this is what the Bullet implementation in Godot 3 does. It would be a fairly sizeable refactoring, but a doable one I think.

I guess that would effectively remove all uses of Jolt's sensors in any practical sense, and instead they would be replaced with just narrow phase queries. They would still need to be bodies though, for things like queries to work on them, but they'd reside in their own broad phase layer with no interaction with any of the other layers.

How would that compare in performance overall? Is using sensors with a contact listener mostly a convenience thing, and not so much an optimization, over just doing your own queries?

@jrouwe
Copy link
Contributor

jrouwe commented Jun 1, 2023

How would that compare in performance overall? Is using sensors with a contact listener mostly a convenience thing, and not so much an optimization, over just doing your own queries?

Sensors are quite simple in Jolt, it is really just a parallel for running over all sensors and checking what they collide with. It will utilize the contact cache, which reduces the amount of actual collision checks that need to be done (easily implemented yourself) and the parallel for is executed together with other physics work so that it leaves less gaps in scheduling compared to when you would do it yourself outside the physics update.

I've been thinking about this a bit more and now I'm leaning towards adding an additional flag on a body next to the IsSensor flag that indicates if you want a sensor to collide with static objects. It can be off by default and have a big warning that things will be expensive if you turn it on...

@timshannon
Copy link
Author

I know I would definitely prefer to opt-in to this behavior rather than have it enabled globally by default.

I'm easily seeing the perf gains had by switching to Jolt, and it's worth a few minor workarounds needed for the change in behavior.

@mihe
Copy link
Contributor

mihe commented Jun 1, 2023

I know I would definitely prefer to opt-in to this behavior rather than have it enabled globally by default.

I must admit I'd be tempted to have it be opt-out just to make the migration path easier, because I do feel strongly about the drop-in aspect of this extension, but I also understand that a lot of people wouldn't know or care to disable such a setting, even when suffering the negative side effects from it. Having it be opt-in (with a prominent warning) is probably the wiser choice.

I'm leaning towards adding an additional flag on a body next to the IsSensor flag that indicates if you want a sensor to collide with static objects.

How would this pair up with disabling mUseManifoldReduction on the sensors? I had to disable that for contacts to be added/removed in a predictable manner. Are there any negative interactions there that need to be considered?

Also, just to be clear, if one were to set up their collision layers/masks (object layers) in such a way that the static bodies were completely culled from any collision with the sensors, then this performance concern mostly goes away, right? Assuming that you only have a select few sensors that need to actually detect static bodies.

@jrouwe
Copy link
Contributor

jrouwe commented Jun 2, 2023

How would this pair up with disabling mUseManifoldReduction on the sensors?

This should work for static too (with the same performance cost).

Also, just to be clear, if one were to set up their collision layers/masks (object layers) in such a way that the static bodies were completely culled from any collision with the sensors, then this performance concern mostly goes away, right?

Yes, if people carefully set up their collision layers so that sensors don't collide with static then there is no performance issue.

@mihe
Copy link
Contributor

mihe commented Jun 2, 2023

Yes, if people carefully set up their collision layers so that sensors don't collide with static then there is no performance issue.

Great! I'll have to figure out some way of communicating that as well.

Seeing as how there seems to be some path forward with this, either with this new body flag or by refactoring areas to instead use queries, I'll go ahead and reopen this.

@mihe mihe reopened this Jun 2, 2023
@mihe mihe removed the wontfix This will not be worked on label Jun 2, 2023
@jrouwe
Copy link
Contributor

jrouwe commented Jun 2, 2023

I'll let you know when there's some progress on this.

jrouwe added a commit to jrouwe/JoltPhysics that referenced this issue Jun 18, 2023
* Enable the BodyCreationSettings::mSensorDetectsStatic flag to have sensors interact with static objects (note the sensor must be kinematic and active for this to work)
* Added test case for sensor detecting static

Fixes #574 and allows godot-jolt/godot-jolt#391 to be fixed.
@jrouwe
Copy link
Contributor

jrouwe commented Jun 18, 2023

There's a new flag BodyCreationSettings::mSensorDetectsStatic to make this possible in jrouwe/JoltPhysics#581.

@mihe
Copy link
Contributor

mihe commented Jun 26, 2023

This is included in the newly released 0.3.0-rc1. I'd appreciate it if you could give it a try and report back how it works out for you, either here or in the release's dedicated discussion.

@timshannon
Copy link
Author

Yep, after flipping the option in the project settings, areas detect static bodies just like in GD physics.

Thanks,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working as intended severity:minor Small impact on functionality or usability topic:runtime Concerning runtime behavior (or its source code)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants