[Bf-committers] Node Efficiency Tools - can it go to trunk?

Bartek Skorupa (priv) bartekskorupa at bartekskorupa.com
Wed Mar 27 09:06:10 CET 2013


Thank you Campbell, Bastien and CoDEmanX for reviewing my code.
I have implemented all of your suggestions.
All of the issues pointed in this review: https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode103 have been addressed.
The review has been done using not the latest version of the script, so some of the suggestions have been implemented earlier.
The script with all of those changes is version 2..1.4 and has been committed as revision: 4434.

Is it now ok to move it to trunk?

Thank you.

With Respect
Bartek Skorupa

www.bartekskorupa.com

On 26 mar 2013, at 17:55, Bartek Skorupa (priv) <bartekskorupa at bartekskorupa.com> wrote:

> Thank you very much.
> 
> Bartek Skorupa
> 
> www.bartekskorupa.com
> 
> On 26 mar 2013, at 17:53, CoDEmanX <codemanx at gmx.de> wrote:
> 
>> if the number of properties you pass to an operator differ, e.g. one 
>> prop per Custom Property of the active object, then you shouldn't do 
>> something like
>> 
>> prop1 = ...
>> prop2 = ...
>> ...
>> prop20 = ...
>> 
>> to allow for up to 20 properties be passed to the operator. Why? Cause 
>> you don't need 20 properties if you mostly need to pass just one or two, 
>> and there may be situations in which you need more than 20. So a fixed 
>> amount is really bad.
>> 
>> Instead, you can use a CollectionProperty and add an item for every 
>> element you need to pass.
>> 
>> Here's an example:
>> 
>> http://www.pasteall.org/40829/python
>> 
>> 
>> If ideasman is fine with your code, then it's ok to move to trunk!
>> 
>> 
>> Am 25.03.2013 17:33, schrieb Bartek Skorupa (priv):
>>> Hey,
>>> @CoDEmanX:
>>> I don't quite get what you mean by:
>>>> But please don't do this if the amount of props varies (prop1-propN)!
>>> Could you elaborate, please?
>>> 
>>> 
>>> Bartek Skorupa
>>> 
>>> www.bartekskorupa.com
>>> 
>>> On 25 mar 2013, at 17:19, CoDEmanX <codemanx at gmx.de> wrote:
>>> 
>>>> Proper formatting:
>>>> 
>>>> props = layout.operator(NodesLinkActiveToSelected.bl_idname,
>>>> text="Replace Links")
>>>> 
>>>> props.prop1 = True
>>>> props.prop2 = True
>>>> props.prop3 = False
>>>> 
>>>> But please don't do this if the amount of props varies (prop1-propN)!
>>>> Use a CollectionProperty instead. I think i did this in the Game
>>>> Property Visulizer addon to replace the evil eval().
>>>> 
>>>> 
>>>> Am 25.03.2013 16:08, schrieb Bartek Skorupa (priv):
>>>>> Thank you for the explanation.
>>>>>> props = layout.operator(NodesLinkActiveToSelected.bl_idname, text="Replace Links").prop1 = True
>>>>>>    props.prop2 = True
>>>>>>    props.prop3 = False
>>>>> Oops… I didn't know you can do it :-) Really…
>>>>> 
>>>>> Every day I learn something.
>>>>> I will change all those properties accordingly.
>>>>> 
>>>>> Thank you once again
>>>>> 
>>>>> Regards
>>>>> 
>>>>> Bartek Skorupa
>>>>> 
>>>>> www.bartekskorupa.com
>>>>> 
>>>>> On 25 mar 2013, at 16:02, Bastien Montagne <montagne29 at wanadoo.fr> wrote:
>>>>> 
>>>>>> Hi,
>>>>>> 
>>>>>> On 25/03/2013 15:51, Bartek Skorupa (priv) wrote:
>>>>>>> Thank you for this quick review.
>>>>>>> Yes you're right about my understanding of bl_label. I did mismatch this and I can change it, np.
>>>>>>> 
>>>>>>> About my 'option' properties:
>>>>>>> In many cases I can change it to enums, but sometimes StringProperty is used because I need to pass more than one property in one go.
>>>>>>> Say I need to set 3 properties:
>>>>>>> prop1 = True
>>>>>>> prop2 = True
>>>>>>> prop3 = False
>>>>>>> 
>>>>>>> I wrap it in one StringProperty that gets the value of 'True True False' and then in execute I use string split( ), and eval to get 3 booleans.
>>>>>>> 
>>>>>>> Please take a look at line 1325 for example:
>>>>>>> layout.operator(NodesLinkActiveToSelected.bl_idname, text="Replace Links").option = 'True True False'
>>>>>>> 
>>>>>>> option then is passed to execute of NodesLinkActiveToSelected and split.
>>>>>>> see lines: 795 to 798:
>>>>>>>        option_split = self.option.split( )
>>>>>>>        replace = eval(option_split[0])
>>>>>>>        use_node_name = eval(option_split[1])
>>>>>>>        use_outputs_names = eval(option_split[2])
>>>>>>> 
>>>>>>> This way I get 3 variables out of one property 'option'
>>>>>>> 
>>>>>>> Is there a better way of doing it?
>>>>>> Yes there is! :)
>>>>>> 
>>>>>> In such case, you should do that:
>>>>>> 
>>>>>>    props = layout.operator(NodesLinkActiveToSelected.bl_idname, text="Replace Links").prop1 = True
>>>>>>    props.prop2 = True
>>>>>>    props.prop3 = False
>>>>>> 
>>>>>>> Another question:
>>>>>>> Could you please explain me why using StringProperty instead of proper EnumProperty is wrong?
>>>>>>> Is it a convention, speed issues or anything else?
>>>>>> First of all, it is a convention. True/false options should be booleans,
>>>>>> options with a well defined set of choices should be enums, etc.
>>>>>> 
>>>>>> This also helps on documentation level (as each enum items has its own
>>>>>> label/description, you do not need comments in code, and doc can be
>>>>>> retrieved easily by multiple ways).
>>>>>> 
>>>>>> And there is also an UI interest, as using enums you can get a nice
>>>>>> drop-down list as a control, or you can even auto-generate a menu where
>>>>>> each entry will execute the operator with one of the options of the enum, …
>>>>>> 
>>>>>> By the way, note that if you have a set of related booleans options that
>>>>>> are not mutually exclusive, you can use an enum with 'ENUM_FLAG' option
>>>>>> (in that case you are just limited to at most 32 different flags - the
>>>>>> length of a classical integer ;) ).
>>>>>> 
>>>>>> Kind regards,
>>>>>> Bastien
>>>>>> 
>>>>>>> Thank you in advance
>>>>>>> 
>>>>>>> Regards
>>>>>>> 
>>>>>>> Bartek Skorupa
>>>>>>> 
>>>>>>> www.bartekskorupa.com
>>>>>>> 
>>>>>>> On 25 mar 2013, at 15:24, Bastien Montagne<montagne29 at wanadoo.fr>  wrote:
>>>>>>> 
>>>>>>>> Hi Bartek,
>>>>>>>> 
>>>>>>>> Just did a (very quick) skim review of your addon, and I agree that
>>>>>>>> there are valuable features in it, worth moving it to trunk. However, I
>>>>>>>> noted at least two points that imho should be addressed before the move:
>>>>>>>> 
>>>>>>>> First, you seems to mismatch labels and tips of operators! E.g. instead
>>>>>>>> of this line:
>>>>>>>> 
>>>>>>>>    bl_label = "Copy Settings of Active Node to Selected Nodes"
>>>>>>>> 
>>>>>>>> I would rather see those:
>>>>>>>> 
>>>>>>>>    bl_label = "Copy Node Settings"
>>>>>>>>    bl_description = "Copy the settings of the active node to selected
>>>>>>>> ones"
>>>>>>>> 
>>>>>>>> The second point is, I think, more important. You should replace your
>>>>>>>> “multipurpose” "option" StringProperty by relevant properties. E.g.
>>>>>>>> 
>>>>>>>>    # option: 'from active', 'from node', 'from socket'
>>>>>>>>    option = StringProperty()
>>>>>>>> 
>>>>>>>> Should be replaced by:
>>>>>>>> 
>>>>>>>>    source = EnumProperty(name="Source", description="A relevant
>>>>>>>> description…", default='FROM_ACTIVE',
>>>>>>>>                          items=(('FROM_ACTIVE', "From Active", "A
>>>>>>>> relevant description…", 'ICON_NONE', 1),
>>>>>>>>                                 ('FROM_NODE', "From Node", "A relevant
>>>>>>>> description…", 'ICON_NONE', 2),
>>>>>>>>                                 ('FROM_SOCKET', "From Socket", "A
>>>>>>>> relevant description…", 'ICON_NONE', 3),
>>>>>>>>                                )
>>>>>>>>                         )
>>>>>>>> 
>>>>>>>> (with relevant changes in other parts of the code).
>>>>>>>> 
>>>>>>>> Best regards,
>>>>>>>> Bastien
>>>>>>>> 
>>>>>>>> On 25/03/2013 14:00, Bartek Skorupa (priv) wrote:
>>>>>>>>> Hey,
>>>>>>>>> 
>>>>>>>>> After recent commits of node_efficiency_tools.py I as an author call this add-on ready.
>>>>>>>>> All of the features that I had in mind have been included, documented on wiki and in video tutorial.
>>>>>>>>> Recent API changes have taken into account.
>>>>>>>>> It certainly will develop further, but at this stage it's IMHO ready to go trunk.
>>>>>>>>> 
>>>>>>>>> I'd like to ask for reviewing the code and hopefully permission to move this add-on to trunk.
>>>>>>>>> https://svn.blender.org/svnroot/bf-extensions/contrib/py/scripts/addons/node_efficiency_tools.py
>>>>>>>>> 
>>>>>>>>> Here's the wiki page:
>>>>>>>>> http://wiki.blender.org/index.php/Extensions:2.6/Py/Scripts/Nodes/Nodes_Efficiency_Tools
>>>>>>>>> 
>>>>>>>>> Tracker:
>>>>>>>>> http://projects.blender.org/tracker/?func=detail&atid=468&aid=33543&group_id=153
>>>>>>>>> 
>>>>>>>>> Thread on blenderartists:
>>>>>>>>> http://blenderartists.org/forum/showthread.php?274755-ADDON-Nodes-Efficiency-Tools
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Many users appreciate this add-on and wish to have it in official Blender releases.
>>>>>>>>> I declare to maintain the code.
>>>>>>>>> 
>>>>>>>>> Please let me know if you feel that it's worth including or not.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> With Respect,
>>>>>>>>> 
>>>>>>>>> Bartek Skorupa
>>>>>>>>> 
>>>>>>>>> www.bartekskorupa.com
>>>>>>>>> 
>>>>>>>>> _______________________________________________
>>>>>>>>> Bf-committers mailing list
>>>>>>>>> Bf-committers at blender.org
>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>>>>>>> 
>>>>>>>> _______________________________________________
>>>>>>>> Bf-committers mailing list
>>>>>>>> Bf-committers at blender.org
>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>>>>> _______________________________________________
>>>>>>> Bf-committers mailing list
>>>>>>> Bf-committers at blender.org
>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>>>>> 
>>>>>> _______________________________________________
>>>>>> Bf-committers mailing list
>>>>>> Bf-committers at blender.org
>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>>> 
>>>>> _______________________________________________
>>>>> Bf-committers mailing list
>>>>> Bf-committers at blender.org
>>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>>> 
>>>> _______________________________________________
>>>> Bf-committers mailing list
>>>> Bf-committers at blender.org
>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>> 
>>> _______________________________________________
>>> Bf-committers mailing list
>>> Bf-committers at blender.org
>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>> 
>> _______________________________________________
>> Bf-committers mailing list
>> Bf-committers at blender.org
>> http://lists.blender.org/mailman/listinfo/bf-committers
> 
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers



More information about the Bf-committers mailing list