SirRamEsq / SmartShape2D

A 2D Terrain Tool for Godot
MIT License
1.27k stars 63 forks source link

Optimize collision generation performance #159

Closed mphe closed 2 months ago

mphe commented 3 months ago

The time needed to generate collision shapes depends on two factors: edge generation and the convex decomposition of the polygon by CollisionPolygon2D. Especially the latter is very expensive on large shapes that are difficult to decompose. Additionally the amount of points the collision polygon consists of depends on the tesselation settings.

This patch tweaks the default tesselation settings to provide pretty much the same visual quality at less computation time. I think they can be even lowered more to tesselation_tolerance = 7.0 and maybe tesselation_stages = 3. I think it's better to provide faster defaults and let users increase them if necessary. Need feedback on this. This also speeds up regular edge generation respectively.

Edit: Settled on tesselation_stages = 3 but kept tesselation_tolerance = 6.0.

The PR also adds two new properties to shape.gd: collision_generation_method and collision_update_mode. See also property doc comments. I set the defaults to Fast (curve based generation instead of edge based) and Editor (only update in editor), as these settings probably fit most projects and improve performance and load times immensely.

To mention some numbers, consider the included test_large_curvy.tscn scene.

Setting Before After
tesselation_tolerance 4.0 6.0
tesselation_stages 5 4
collision_generation_method Precise Fast
collision_update_mode EditorAndRuntime Editor
Metric Before After
Num points 1366 918
Generate 38 ms 0.007 ms
Convex decompose 64 ms 32 ms

With the new settings, this scene updates around 60 ms faster in editor and loads 102 ms faster during runtime (disregarding improved edge generation time through tesselation settings). Since generation time became negligible with Fast collision generation and convex decomposition is not in our hands, this is as fast as collision generation will get. For even more performance, users can tweak tesselation settings according to their project needs.

With this patch merged, I'd also close #119, as the load times are absolutely fine by now.

mphe commented 3 months ago

I think they can be even lowered more to tesselation_tolerance = 7.0 and maybe tesselation_stages = 3.

What do you think about lowering the settings more?

limbonaut commented 3 months ago

What do you think about lowering the settings more?

In my tests, increasing tolerance to 7.0 produced some drastic changes on some large segments. Personally, I wouldn't like it as a default if it's prone to such errors.

image ↓↓↓↓↓↓↓ TURNS FLAT ↓↓↓↓↓↓↓ image

Maybe we could turn it into a project setting?

mphe commented 3 months ago

What about tesselation_stages = 3?

Project setting sounds good, but I'd also keep the properties for overriding the project settings when needed, e.g. when having a shape that requires more detail. This will be a breaking change though, as the existing properties should be renamed so not all existing shapes automatically override the project settings. But I think people rarely touched the setting in the first place.

limbonaut commented 3 months ago

What about tesselation_stages = 3?

Feels like a good compromise. In some cases, even looks better for whatever reason.

Project setting sounds good, but I'd also keep the properties for overriding the project settings when needed, e.g. when having a shape that requires more detail. This will be a breaking change though, as the existing properties should be renamed so not all existing shapes automatically override the project settings. But I think people rarely touched the setting in the first place.

Hmm, it could be done with _get_property_revert. I wrote a similar thing for BlackboardPlan in limboai (in a context of a dynamic default value). Not sure if it's worth the effort, if people rarely touch these settings.

limbonaut commented 3 months ago

There is probably no reason to have different defaults on different project, is there? The more I think about it, the less I like this idea. It's enough to just have sane defaults.

mphe commented 2 months ago

I can think of users having lots of shapes in their game but want to tweak tesselation options globally without having to go through all shapes manually. But I'm also not sure whether it's actually worth it. Personally, I have my shapes in a separate scene so I just need to tweak the scene to update all shapes. Not sure how other people do it.

mphe commented 2 months ago

I changed the default tesselation_stages to 3. Also fixed a unit test which now requires a slightly greater epsilon.

I think we can hold the idea with the project settings in mind and maybe implement it in the future when there is a demand for it or when there are more settings that would benefit from being project-wide configurable.

In that regard, this PR should be finished.

limbonaut commented 2 months ago

Besides that scene, LGTM. Unless you want to do something else with this PR.

mphe commented 2 months ago

What do you mean besides the scene? Remove the test scene?

limbonaut commented 2 months ago

Oh, I probably forgot to finish my review :facepalm: That's why you didn't see that comment.

limbonaut commented 2 months ago

Thanks!