[Bf-codereview] UV Atlas Addon Review (issue 10183043)

[ideasman42 at gmail.com]

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py
File /src/blender/release/scripts/addons_contrib/uv_texture_atlas.py
(right):

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode46
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:46:
thisScene = context.scene
simply `scene` is conventional, also this isnt being used later on in
this function

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode55
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:55: if
len(context.scene.ms_lightmap_groups) > 0:
if context.scene.ms_lightmap_groups:
- works too

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode89
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:89: if
context.scene.objects.active != None:
`is not None` is more common in python (slightly faster)

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode92
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:92: if
group.bake == True and len(bpy.data.groups[group.name].objects) > 0:
again, if bpy.data.groups[group.name].objects: is faster

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode101
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:101:
layersNumber = [0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19]
layersNumber = range(20)

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode104
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:104:
isThisObjectVisible = True
looks like you could break after assigning.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode133
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:133: if
context.scene.objects.active != None:
is not None

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode136
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:136: if
group.bake == True and len(bpy.data.groups[group.name].objects) > 0:
no need for len()

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode144
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:144:
layersNumber = [0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19]
range(20)

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode177
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:177: if
group.bake == True and len(bpy.data.groups[group.name].objects) > 0:
Notice this code is being duplicated, you could try move into a function
and share it.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode224
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:224:
unwrap_type =
EnumProperty(name="unwrap_type",items=(('0','Smart_Unwrap',
'Smart_Unwrap'),('1','Lightmap', 'Lightmap'), ('2','No_Unwrap',
'No_Unwrap')))
nicer to give each item its own line, see examples of EnumProperty in
blender scripts.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode249
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:249: for
groupObj in bpy.data.groups:
you can do this instead...

think you could change this so it gets the group,

group = bpy.data.groups.get(group_name)
if group is None:
   group = bpy.data.groups.new(group_name)

Then no need to do group lookups within the loop below (which can get
slow)

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode261
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:261:
bpy.data.groups[group_name].objects.link(object)
this should be avoided in a loop: bpy.data.groups[group_name]

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode267
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:267:
bl_label = ""
Label shouldn't be blank

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode290
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:290: def
removeUV(self, mesh, name):
calling the object mesh is a bit confusing, perhaps ob_mesh instead?

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode294
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:294:
bpy.ops.mesh.uv_texture_remove()
General comment for the entire script using operators to add and remove
UV's isnt great.
You can do through the data api:

uv_layer = me.uv_textures.new()

... and ...

me.uv_textures.remove(uv_layer)

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode350
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:350: if
object.type == 'MESH' and bpy.data.groups[group_name] in
object.users_group:
bpy.data.groups[group_name] in object.users_group

Can be written better as...

if object in bpy.data.groups[group_name].objects

(Internally this uses less lookups)

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode360
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:360:
bpy.ops.mesh.uv_texture_remove()
again, no need to use operators.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode365
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:365:
context.area.type = old_context
if you dont use operators here you shouldn't have to change the area
type.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode384
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:384: if
len(context.selected_objects) > 0:
in this case there is no real advantage in checking the list is empty
an empty list wont loop anyway. simply remove the check is fine.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode386
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:386:
context.scene.objects.active = object
Quite sure setting active isnt needed.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode388
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:388:
bpy.data.groups[group.name].objects.link(object)
you should get the group once and then reuse it.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode410
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:410: if
groupObj.name == group_name:
one group lookup should be done here, rather then checking every groups
name.\

group = bpy.data.groups.get(group_name)
if group is not None:
  ...

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode444
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:444:
context.scene.objects.active = object
if you dont use operators below you wont need to set this object active.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode467
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:467:
bpy.ops.image.new(name=self.group_name,width=self.resolution,height=self.resolution)
you can use bpy.data.images.new(), then no need to read from the screen.

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode513
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:513:
OBJECTLIST = []
suggest to do this a bit different

OBJECTLIST = bpy.data.groups[self.group_name].objects[:]
for obj in OBJECTLIST:
     obj.select = True

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode525
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:525:
bpy.ops.object.select_all(action='DESELECT')
You are selecting all objects in OBJECTLIST  above, now deselecting all,
looks like selection above isnt needed?

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode593
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:593:
OBJECTLIST = [] #clear array
OBJECTLIST.clear()

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/addons_contrib/uv_texture_atlas.py#newcode691
/src/blender/release/scripts/addons_contrib/uv_texture_atlas.py:691:
bpy.utils.register_class(addLightmapGroup)
suggest to make a list of classes, then loop over and
register/unregister

classes = (
     delLightmapGroup,
    ....
     )

for cls in classes:
     bpy.utils.register_class(cls)

https://codereview.appspot.com/10183043/