@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) <bartekskor...@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) > <bartekskor...@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) >> <bartekskor...@bartekskorupa.com> wrote: >> >>> Thank you very much. >>> >>> Bartek Skorupa >>> >>> www.bartekskorupa.com >>> >>> On 26 mar 2013, at 17:53, CoDEmanX <codem...@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 <codem...@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 <montagn...@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<montagn...@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@blender.org >>>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>>>> >>>>>>>>>> _______________________________________________ >>>>>>>>>> Bf-committers mailing list >>>>>>>>>> Bf-committers@blender.org >>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>> _______________________________________________ >>>>>>>>> Bf-committers mailing list >>>>>>>>> Bf-committers@blender.org >>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Bf-committers mailing list >>>>>>>> Bf-committers@blender.org >>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Bf-committers mailing list >>>>>>> Bf-committers@blender.org >>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>> >>>>>> _______________________________________________ >>>>>> Bf-committers mailing list >>>>>> Bf-committers@blender.org >>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>> >>>>> _______________________________________________ >>>>> Bf-committers mailing list >>>>> Bf-committers@blender.org >>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>> >>>> _______________________________________________ >>>> Bf-committers mailing list >>>> Bf-committers@blender.org >>>> http://lists.blender.org/mailman/listinfo/bf-committers >>> >>> _______________________________________________ >>> Bf-committers mailing list >>> Bf-committers@blender.org >>> http://lists.blender.org/mailman/listinfo/bf-committers >> >> _______________________________________________ >> Bf-committers mailing list >> Bf-committers@blender.org >> http://lists.blender.org/mailman/listinfo/bf-committers > > _______________________________________________ > Bf-committers mailing list > Bf-committers@blender.org > http://lists.blender.org/mailman/listinfo/bf-committers _______________________________________________ Bf-committers mailing list Bf-committers@blender.org http://lists.blender.org/mailman/listinfo/bf-committers