[Bf-committers] Node Efficiency Tools - can it go to trunk?
Bastien Montagne
montagne29 at wanadoo.fr
Tue Mar 26 17:10:25 CET 2013
Yep. :)
On 26/03/2013 17:10, Bartek Skorupa (priv) wrote:
> I will ask Campbell for review.
>
> If he reviews and say that it's ok - can I treat this as "You have a permission to move it to trunk"?
>
> Thank you.
>
> Cheers
> Bartek Skorupa
>
> www.bartekskorupa.com
>
> On 26 mar 2013, at 17:01, Bastien Montagne<montagne29 at wanadoo.fr> wrote:
>
>> Yes, imho it's good for addons/ dir. Would be cool if you could get
>> Campbell (ideasman_42) to review it too in the next days, else just move
>> it! :)
>>
>> Bastien
>>
>> On 26/03/2013 16:54, Bartek Skorupa (priv) wrote:
>>> @ Bastien
>>> Changed the code according to your guidelines.
>>>
>>> Is the script now ok to go to trunk?
>>> Thank you for your help.
>>>
>>> Cheers,
>>> Bartek Skorupa
>>>
>>> www.bartekskorupa.com
>>>
>>> On 25 mar 2013, at 17:33, Bartek Skorupa (priv)<bartekskorupa at bartekskorupa.com> wrote:
>>>
>>>> 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
>
More information about the Bf-committers
mailing list