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

Bartek Skorupa (priv) bartekskorupa at bartekskorupa.com
Wed Mar 27 12:49:16 CET 2013


Final code clean up before moving to trunk:
revision 4436.
All suggested changes have been made.

I'd like to ask for final permission before I commit.
Thank you

With Respect

Bartek Skorupa

www.bartekskorupa.com

On 27 mar 2013, at 11:35, Bartek Skorupa (priv) <bartekskorupa at bartekskorupa.com> wrote:

> @Campbell
> Thank you for cleaning up my code of node_efficiency_tools.py (revision 4435)
> I assume that now it's ready to be moved to trunk, is it?
> Do I have your permission to do it?
> 
> Thank you for your time.
> 
> With Respect
> Bartek Skorupa
> 
> 
> Revision: 4435
>         http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-extensions&revision=4435
> Author:   campbellbarton
> Date:     2013-03-27 10:17:08 +0000 (Wed, 27 Mar 2013)
> Log Message:
> -----------
> minor code cleanup
> 
> Modified Paths:
> --------------
>   contrib/py/scripts/addons/node_efficiency_tools.py
> 
> 
> 
> 
> On 27 mar 2013, at 09:14, Bartek Skorupa (priv) <bartekskorupa at bartekskorupa.com> wrote:
> 
>> Note about "Select Parent/Children":
>> This operator allows to select 'FRAME'  that wraps selected nodes or select all of the nodes that are "children" of selected 'FRAME'.
>> It's done using '[' and ']' keys.
>> Campbell said that it could be implemented on a higher level, but before it happens, I'd leave it in my code.
>> When this option is implemented I'll immediately remove this class from node_efficiency_tools.py
>> 
>> Bartek Skorupa
>> 
>> www.bartekskorupa.com
>> 
>> On 27 mar 2013, at 09:06, Bartek Skorupa (priv) <bartekskorupa at bartekskorupa.com> wrote:
>> 
>>> 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
>>> 
>>> _______________________________________________
>>> 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