godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.43k stars 20.25k forks source link

Instancing of a scene does not instance a new ConcavePolygonShape in a collision shape #40887

Open goatchurchprime opened 4 years ago

goatchurchprime commented 4 years ago

Godot version: 3.2.2.stable

OS/device including version: ubuntu 18.04

Issue description: If you use a scene to procedurally generate static bodies using the SurfaceTool and the shape.set_faces() function, then it comes as a surprise to learn that each time you instance a new one it over-writes the triangles that are assigned to the concave collision shape

This is a very difficult bug to discover, particularly when your procedural shapes are reasonably similar and there's something about the physics that doesn't look right.

The work-around is easy. Put $CollisionShape.shape = ConcavePolygonShape.new() in the _ready() function of the static body. The current behavior of not applying the deep-copy to this Collision object when the scene is instanced is not functionally useful. This might be a special case, however. Maybe the other pre-configured shapes (box, cylinder) are sufficiently constrained that they can be shared, because it is not their purpose to have procedural geometry inserted into them.

Animated gif shows two procedurally generated ramp static bodies with cubes rolling down them, where the collision shape of the steeper ramp has overwritten the collision shape of the shallow ramp. out

Steps to reproduce: See below.

Minimal reproduction project: collisionclonebug.zip

hoontee commented 4 years ago

The Shape is a shared resource. Modifying it directly will apply the changes to all nodes that reference it.

You must use $CollisionShape.shape = $CollisionShape.shape.duplicate() or $CollisionShape.shape = ConcavePolygonShape.new().

Set resource_local_to_scene to true.

goatchurchprime commented 4 years ago

The problem is that the ConvexPolygonShape is not like a normal resource. It is used as a container that always starts out as empty. The shape only gets created by feeding it a list of triangles. https://docs.godotengine.org/en/stable/classes/class_concavepolygonshape.html

When you instance a scene, all the container nodes are cloned, so when I add new elements to a subnode in one instance those elements do not appear in the subnode of another instance. The same principle should apply to this node.

With the current implementation, anyone who does not write $CollisionShape.shape = ConcavePolygonShape.new() for the second instantiation of a scene containing one of these "resources" has created a bug in their code that is very difficult to spot. It would be better if every instantiation of ConcavePolygonShape from a scene returned an error if used or was null so that people knew that they always had to write $CollisionShape.shape = ConcavePolygonShape.new().

hoontee commented 4 years ago

I'm sorry, I actually gave you bad information due to me being blatantly unaware of resource_local_to_scene's purpose... Please confirm that setting resource_local_to_scene to true fixes your issue: image

goatchurchprime commented 4 years ago

Yes, that works perfectly.

Can that flag definitely be set to true by default? It's proven to be difficult for both of us to find, and I can't think of a use-case for to be set to false for these procedurally generated objects.

Zireael07 commented 4 years ago

Shapes are shared by default for performance reasons.