API Exposing of Bullet Collision Masks

Exposes Bullet's collision mask/group functionality through new UI elements in the Physics panel while the renderer is set to "Game Engine".

[z0r, 2012-05-19 11:58:09]

Very nice feature!

I have tested it, and it works well - except for one major regression: the Near
and Radar sensors no longer work. This should be fixed before committing.
Collision, Ray and Touch do work.

I left a number of comments inline. Many are about style, but some are not, in
particular my comments in object.c and properties_game.py.

I really look forward to having this in trunk.

http://codereview.appspot.com/6216047/diff/1/release/scripts/startup/bl_ui/pr...
File release/scripts/startup/bl_ui/properties_game.py (right):

http://codereview.appspot.com/6216047/diff/1/release/scripts/startup/bl_ui/pr...
release/scripts/startup/bl_ui/properties_game.py:178: layout.prop(ob,
"hide_render", text="Invisible")
Duplicate of previous elif statement

http://codereview.appspot.com/6216047/diff/1/release/scripts/startup/bl_ui/pr...
release/scripts/startup/bl_ui/properties_game.py:189: if physics_type not in
{'INVISIBLE', 'NO_COLLISION', 'OCCLUDE'}:
'INVISIBLE' is not a physics type; see body_type_items in rna_object.c. The elif
statement above should be changed too.

http://codereview.appspot.com/6216047/diff/1/release/scripts/startup/bl_ui/pr...
release/scripts/startup/bl_ui/properties_game.py:193: col.prop(game,
"collision_mask")
I'd prefer these to sit next to each other on the same row. There's plenty of
space for it.

http://codereview.appspot.com/6216047/diff/1/source/blender/blenkernel/intern...
File source/blender/blenkernel/intern/object.c (right):

http://codereview.appspot.com/6216047/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/object.c:838: ob->col_group = ob->col_mask = 1;
I think it's probably more useful to have this set to all masks (0xff), instead
of just the first one (0x01). Consider this use case:

 - Object X should collide with all objects except object Y;
 - Object Y should collide with all objects except object X.

As it is currently, I would have to:

 - Move Y to group 2
 - Set *all* objects' masks to 1 & 2
 - Except for X's mask, which should be set to 1 only.

However, if the mask was already 0xff, all I would have to do is:

 - Move Y to group 2
 - Unset X's 2nd mask

In summary, I think this should be changed to:

+	ob->col_group = 0x01;
+	ob->col_mask = 0xff;

http://codereview.appspot.com/6216047/diff/1/source/blender/blenloader/intern...
File source/blender/blenloader/intern/readfile.c (right):

http://codereview.appspot.com/6216047/diff/1/source/blender/blenloader/intern...
source/blender/blenloader/intern/readfile.c:7530: }
Style: should be:
for (ob = main->object.first; ob; ob = ob->id.next)

See also comment about the default mask in object.c

http://codereview.appspot.com/6216047/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_object.c (right):

http://codereview.appspot.com/6216047/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_object.c:1075: }
The memset is redundant here, isn't it?

http://codereview.appspot.com/6216047/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_object.c:1088: return;
Style: space after "if"

http://codereview.appspot.com/6216047/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_object.c:1104: }
as above

http://codereview.appspot.com/6216047/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_object.c:1117: return;
Style: as above

http://codereview.appspot.com/6216047/diff/1/source/gameengine/Ketsji/KX_Game...
File source/gameengine/Ketsji/KX_GameObject.cpp (right):

http://codereview.appspot.com/6216047/diff/1/source/gameengine/Ketsji/KX_Game...
source/gameengine/Ketsji/KX_GameObject.cpp:2975:
KX_RayCast::Callback<KX_GameObject> callback(this,spc, NULL, false, true);
Style: May as well add a space before "spc" here

[benoit.bolsee, 2012-05-20 16:40:25]

http://codereview.appspot.com/6216047/diff/7001/source/blender/blenloader/int...
File source/blender/blenloader/intern/readfile.c (right):

http://codereview.appspot.com/6216047/diff/7001/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:7528: }
default value should be set according to the static/dynamic status.
dyna: 0x01/0xFF
static: 0x02/0xFD
(see my other comment in KX_ConvertPhysicsObject.cpp

http://codereview.appspot.com/6216047/diff/7001/source/gameengine/Ketsji/KX_C...
File source/gameengine/Ketsji/KX_ConvertPhysicsObjects.cpp (right):

http://codereview.appspot.com/6216047/diff/7001/source/gameengine/Ketsji/KX_C...
source/gameengine/Ketsji/KX_ConvertPhysicsObjects.cpp:408:
ci.m_collisionFilterMask |= objprop->m_col_mask;
There seems to be a problem here: the distinction between static and dynamic
object no longer exists. Previously static objects were not allowed to collision
with other static objects. The new code requires that the appropriate
filter/mask is set in m_col_group and m_col_mask to reproduce this behavior,
which will cause big regression tests as these fields are set to the same value
for all objects by default. I think that static/dynamic object must receive a
different mask by default.
dynamic should have filter/mask=0x01/0xFF and static=0x02/0xFD or something like
that.

[Moguri, 2012-05-21 05:38:08]

Just a note, when this eventually makes it into trunk, we can probably close
this bug report:
http://projects.blender.org/tracker/index.php?func=detail&aid=19265&group_id=...

[z0r, 2012-05-21 22:44:04]

On 2012/05/21 05:38:08, Moguri wrote:
> Just a note, when this eventually makes it into trunk, we can probably close
> this bug report:
>
http://projects.blender.org/tracker/index.php?func=detail&aid=19265&group_id=...

Will the groups and masks override the old behaviour? Is that really desirable?
If anyone has one handy, I'd like to see a test file that illustrates exactly
what is going to change - this would be useful for the release notes, anyway.

[z0r, 2012-05-21 23:05:43]

Here's my basic test, FWIW:

http://www.pasteall.org/blend/14052

It doesn't have any logic bricks in it, though.

[z0r, 2012-05-21 23:53:55]

I have tested my big game with the latest version of the patch; I didn't find
any regressions.