On Wed, Mar 27, 2013 at 10:49 PM, Bartek Skorupa (priv) <[email protected]> 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) > <[email protected]> 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) >> <[email protected]> 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) >>> <[email protected]> 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) >>>> <[email protected]> wrote: >>>> >>>>> Thank you very much. >>>>> >>>>> Bartek Skorupa >>>>> >>>>> www.bartekskorupa.com >>>>> >>>>> On 26 mar 2013, at 17:53, CoDEmanX <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>>>>>> 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<[email protected]> >>>>>>>>>>> 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 >>>>>>>>>>>>> [email protected] >>>>>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>>>>>> >>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>> Bf-committers mailing list >>>>>>>>>>>> [email protected] >>>>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> Bf-committers mailing list >>>>>>>>>>> [email protected] >>>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>>>> >>>>>>>>>> _______________________________________________ >>>>>>>>>> Bf-committers mailing list >>>>>>>>>> [email protected] >>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> Bf-committers mailing list >>>>>>>>> [email protected] >>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Bf-committers mailing list >>>>>>>> [email protected] >>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Bf-committers mailing list >>>>>>> [email protected] >>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>> >>>>>> _______________________________________________ >>>>>> Bf-committers mailing list >>>>>> [email protected] >>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>> >>>>> _______________________________________________ >>>>> Bf-committers mailing list >>>>> [email protected] >>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>> >>>> _______________________________________________ >>>> Bf-committers mailing list >>>> [email protected] >>>> http://lists.blender.org/mailman/listinfo/bf-committers >>> >>> _______________________________________________ >>> Bf-committers mailing list >>> [email protected] >>> http://lists.blender.org/mailman/listinfo/bf-committers >> >> _______________________________________________ >> Bf-committers mailing list >> [email protected] >> http://lists.blender.org/mailman/listinfo/bf-committers > > _______________________________________________ > Bf-committers mailing list > [email protected] > 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 [email protected] http://lists.blender.org/mailman/listinfo/bf-committers
