Code review - Issue 7235078: Nodehttps://codereview.appspot.com/2013-02-02T12:52:21+00:00rietveld
Message from unknown
2013-02-01T13:35:57+00:00ideasman42urn:md5:8337d229cc25d4b31a15a55e21f155ea
Message from ideasman42@gmail.com
2013-02-01T13:35:59+00:00ideasman42urn:md5:d23d418c9216311fd7466c0b44417345
Message from ideasman42@gmail.com
2013-02-01T13:55:11+00:00ideasman42urn:md5:1d9c01325732e056447c977685047758
Generally fine to merge but suggest changes,
- Unless theres a reason to use lists, use tuples.
- Dont use globals (Looks like theres no need to anyway).
- Keymap removal can be handled nicer (see link inline)
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py
File node_efficiency_tools.py (right):
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode42
node_efficiency_tools.py:42: rl_outputs = [
if this never changes, suggest to use tuples. Same for other global lists.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode73
node_efficiency_tools.py:73: blend_types = ['MIX', 'ADD', 'MULTIPLY', 'SUBTRACT', 'SCREEN', 'DIVIDE', 'DIFFERENCE', 'DARKEN', 'LIGHTEN', 'OVERLAY', 'DODGE', 'BURN', 'HUE', 'SATURATION', 'VALUE', 'COLOR', 'SOFT_LIGHT', 'LINEAR_LIGHT']
prefer splitting at 120 length max.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode125
node_efficiency_tools.py:125: global blend_types
None of these need to be global, unless you want to re-assign (which looks not to be the case - this goes for all other `global` use in this file)
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode136
node_efficiency_tools.py:136: if node.select and len(node.outputs) > 0:
'len(node.outputs) > 0'
should be replaced with
'node.outputs'
This is more efficient to test since it only checks the list isnt empty.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode138
node_efficiency_tools.py:138: for [type, the_list, dst] in [
again, suggest to use typles.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode313
node_efficiency_tools.py:313: if change in [0.0, 1.0]:
with python3.x checking contains is nice to use sets.
'in {0.0, 1.0}'
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode803
node_efficiency_tools.py:803: bpy.ops.transform.resize(value = (0.0, 0.0, 0.0))
would prefer changing X/Y's explicitly, calling transform op isnt totally reliable.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode1289
node_efficiency_tools.py:1289: def unregister():
Suggest to use this method of removing keymaps. more direct.
http://www.blender.org/documentation/blender_python_api_2_65_9/info_tutorial_addon.html#keymap
Message from bartekskorupa.com@gmail.com
2013-02-01T14:27:10+00:00bartekskorupaurn:md5:28bb68dfefc75d22329f94d94033d3bd
I'll try to address most of the issues, but I have problems with one of them:
line 803: boy.ops.transform.resize(value = (0.0, 0.0, 0.0))
I use this operator as it's a hack to calculate nodes' dimensions.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py
File node_efficiency_tools.py (right):
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode803
node_efficiency_tools.py:803: bpy.ops.transform.resize(value = (0.0, 0.0, 0.0))
What I do here is a hack. Changing X/Y locations won't do what I want. nodes' locations are based on upper/left corners. I use resize operator because this brigs them into same position basing on "median point". Then I can calculate nodes' dimensions. I need to do it this way, because 'width' or 'height' properties of nodes have issues. They don't give me what I expect.
Message from bartekskorupa.com@gmail.com
2013-02-01T18:40:36+00:00bartekskorupaurn:md5:4166a5329f18f1d221654949c31e4ad2
Most of issues have been addressed. Only 2 left because I have doubts I'll need to discuss.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py
File node_efficiency_tools.py (right):
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode42
node_efficiency_tools.py:42: rl_outputs = [
On 2013/02/01 13:55:11, ideasman42 wrote:
> if this never changes, suggest to use tuples. Same for other global lists.
Done.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode73
node_efficiency_tools.py:73: blend_types = ['MIX', 'ADD', 'MULTIPLY', 'SUBTRACT', 'SCREEN', 'DIVIDE', 'DIFFERENCE', 'DARKEN', 'LIGHTEN', 'OVERLAY', 'DODGE', 'BURN', 'HUE', 'SATURATION', 'VALUE', 'COLOR', 'SOFT_LIGHT', 'LINEAR_LIGHT']
On 2013/02/01 13:55:11, ideasman42 wrote:
> prefer splitting at 120 length max.
Done.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode125
node_efficiency_tools.py:125: global blend_types
On 2013/02/01 13:55:11, ideasman42 wrote:
> None of these need to be global, unless you want to re-assign (which looks not
> to be the case - this goes for all other `global` use in this file)
Done.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode136
node_efficiency_tools.py:136: if node.select and len(node.outputs) > 0:
On 2013/02/01 13:55:11, ideasman42 wrote:
> 'len(node.outputs) > 0'
> should be replaced with
> 'node.outputs'
> This is more efficient to test since it only checks the list isnt empty.
Done.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode138
node_efficiency_tools.py:138: for [type, the_list, dst] in [
On 2013/02/01 13:55:11, ideasman42 wrote:
> again, suggest to use typles.
Done.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode313
node_efficiency_tools.py:313: if change in [0.0, 1.0]:
On 2013/02/01 13:55:11, ideasman42 wrote:
> with python3.x checking contains is nice to use sets.
> 'in {0.0, 1.0}'
Done.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode1289
node_efficiency_tools.py:1289: def unregister():
Here I have doubts. Doesn't it remove the whole keymap? What if I have several add ons using the same keymap? I'm afraid items for those other addons will be removed once I de-activate this addon. Am I totally wrong here?
Message from bartekskorupa.com@gmail.com
2013-02-02T12:52:21+00:00bartekskorupaurn:md5:ad8c0549bf9fc26d9d2b464669153c9a
Kemap items handling corrected. It didn't at first work because of my mistakes.
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py
File node_efficiency_tools.py (right):
https://codereview.appspot.com/7235078/diff/1/node_efficiency_tools.py#newcode1289
node_efficiency_tools.py:1289: def unregister():
On 2013/02/01 13:55:11, ideasman42 wrote:
> Suggest to use this method of removing keymaps. more direct.
>
> http://www.blender.org/documentation/blender_python_api_2_65_9/info_tutorial_addon.html#keymap
Done.
In my previous tests I must have made some mistakes, so I wrongly assumed that there must be some issue with your solution. I analyzed everything and now I have no doubts whatsoever that your way is THE way :-). Sorry for that. I have modified my script according to suggestions