[Bf-codereview] API Exposing of Bullet Collision Masks (issue 6216047)

[alex.d.fraser at gmail.com]

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/properties_game.py
File release/scripts/startup/bl_ui/properties_game.py (right):

http://codereview.appspot.com/6216047/diff/1/release/scripts/startup/bl_ui/properties_game.py#newcode178
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/properties_game.py#newcode189
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/properties_game.py#newcode193
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/object.c
File source/blender/blenkernel/intern/object.c (right):

http://codereview.appspot.com/6216047/diff/1/source/blender/blenkernel/intern/object.c#newcode838
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/readfile.c
File source/blender/blenloader/intern/readfile.c (right):

http://codereview.appspot.com/6216047/diff/1/source/blender/blenloader/intern/readfile.c#newcode7530
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/rna_object.c
File source/blender/makesrna/intern/rna_object.c (right):

http://codereview.appspot.com/6216047/diff/1/source/blender/makesrna/intern/rna_object.c#newcode1075
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/rna_object.c#newcode1088
source/blender/makesrna/intern/rna_object.c:1088: return;
Style: space after "if"

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

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

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

http://codereview.appspot.com/6216047/diff/1/source/gameengine/Ketsji/KX_GameObject.cpp#newcode2975
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

http://codereview.appspot.com/6216047/