[Bf-committers] Node Efficiency Tools - moved to trunk
Bartek Skorupa (priv)
bartekskorupa at bartekskorupa.com
Wed Mar 27 23:22:47 CET 2013
Thank you very much Campbell for your changes.
I was so excited about possibility to move the Add On to trunk that I missed those other mixed cases that you corrected. Sorry for that and once again - thank you.
With Respect
Bartek Skorupa
www.bartekskorupa.com
On 27 mar 2013, at 19:34, Campbell Barton <ideasman42 at gmail.com> wrote:
> On Wed, Mar 27, 2013 at 10:49 PM, Bartek Skorupa (priv)
> <bartekskorupa at bartekskorupa.com> wrote:
>> 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
>>
>> _______________________________________________
>> Bf-committers mailing list
>> Bf-committers at blender.org
>> http://lists.blender.org/mailman/listinfo/bf-committers
>
> Thinks this is fine to move to trunk
>
> Command to move script...
> svn mv https://svn.blender.org/svnroot/bf-extensions/contrib/py/scripts/addons/node_efficiency_tools.py
> https://svn.blender.org/svnroot/bf-extensions/trunk/py/scripts/addons/
> (Must add this info to our wiki docs!)
>
> --
> - Campbell
> _______________________________________________
> 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