UV Atlas Addon Review

UV Atlas

[ideasman42, 2013-06-11 08:51:48]

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

https://codereview.appspot.com/10183043/diff/1//src/blender/release/scripts/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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/a...
/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)