First of all, let me say that I really appreciate all the help with my (somewhat silly) plugin. I really hoped that I would get some feedback on my general coding practices, and I got more than I expected!
@thomthom said:
- You haven't wrapped your code in a module - meaning all the methods you added are added to the core Object and therefore in every object in the Ruby environment. Please wrap all your code in your own namespace. See this article for more information: http://www.thomthom.net/thoughts/2012/01/golden-rules-of-sketchup-plugin-development/
I didn't even know this was a thing! I'll implement that in v1.1 as well as my future plugins, definitely.
@thomthom said:
- You are extending the Float class with some methods to round number, this is the same as adding methods without a namespace, you are very likely to clash with other plugins. Some other plugin might try to add the same method to
Float, that behave slightly differently which will cause unexpected results in either plugin. Please avoid such extension and use helper methods within your own namespace. This is also explain in the first link I posted.
In your code it's also redundant, as you use it like so: redness.round_to(0).to_i which is the same as redness.round.
Interesting! In another thread I asked about Ruby's round() function, but I was told (correctly) that it was not supported. I didn't understand that there is still a round function. That clears up a lot! I was able to dump that whole class thing and just use round.
@thomthom said:
- You are using
typename to determine the type of entity - this is very slow. Use .is_a? instead. See this article for more information: http://www.thomthom.net/thoughts/2011/12/never-ever-use-typename/
Oh man, my bad! I totally saw this in the optimization thread... then promptly forgot it. Fixed!
@thomthom said:
- You test against version 8 of SketchUp, but I see nothing in the code that requires version 8. You're not even using the
disable_ui flag of model.start_operation - which is a SU 7 feature (and recommended to give you a speed boost). As far as I can tell, it'd work with at least SU6, maybe even older. Also, avoid diaplying message boxes upon loading a plugin, it makes the startup experience of SketchUp unpleasant and might cause the toolbars to shuffle about.
Good point! I copypasta'd that from another plugin, as I thought it would prevent issues with earlier versions of plugins, but I don't know what the major differences between SketchUp versions even are. In any case, I've enabled disable_ui and changed version check to look for 7.0.
@thomthom said:
- I'm wondering what this is about:
<span class="syntaxdefault"></span><span class="syntaxcomment"># Setting VERBOSE to nil is a simple speed boost tip from Dan Rathbun on the Sketchucation forums.<br /></span><span class="syntaxdefault">$VERBOSE </span><span class="syntaxkeyword">=</span><span class="syntaxdefault"> nil</span>
Can you link to the thread where this is talked about?
Since it's a shared environment I stay away from adjusting global settings like that. But I'm interesting in knowing what speed improvement there is.
Sure! It's here in the optimization thread. It's possible I totally did not understand how to properly use that information.
@thomthom said:
selection.each{ |f| selection.remove f if f.typename != "Face" }
If you are removing lots of entities from the selection it's best to do it in bulk, otherwise the operation can be very slow.
<span class="syntaxdefault"><br />not_faces </span><span class="syntaxkeyword">=</span><span class="syntaxdefault"> selection</span><span class="syntaxkeyword">.</span><span class="syntaxdefault">reject </span><span class="syntaxkeyword">{</span><span class="syntaxdefault"> </span><span class="syntaxkeyword">|</span><span class="syntaxdefault">e</span><span class="syntaxkeyword">|</span><span class="syntaxdefault"> e</span><span class="syntaxkeyword">.</span><span class="syntaxdefault">is_a</span><span class="syntaxkeyword">?(</span><span class="syntaxdefault">Sketchup</span><span class="syntaxkeyword">;;</span><span class="syntaxdefault">Face</span><span class="syntaxkeyword">)</span><span class="syntaxdefault"> </span><span class="syntaxkeyword">}<br /></span><span class="syntaxdefault">selection</span><span class="syntaxkeyword">.</span><span class="syntaxdefault">remove</span><span class="syntaxkeyword">(</span><span class="syntaxdefault"> not_faces </span><span class="syntaxkeyword">)<br /></span><span class="syntaxdefault"> </span>
However, there is no need for you to modify the user's selection, maybe the user wants to keep the selection. Instead of modifying the selection just inspect it for what you are interested in. In your case the faces.
<span class="syntaxdefault"><br />faces </span><span class="syntaxkeyword">=</span><span class="syntaxdefault"> selection</span><span class="syntaxkeyword">.</span><span class="syntaxdefault">select</span><span class="syntaxkeyword">{</span><span class="syntaxdefault"> </span><span class="syntaxkeyword">|</span><span class="syntaxdefault">e</span><span class="syntaxkeyword">|</span><span class="syntaxdefault"> e</span><span class="syntaxkeyword">.</span><span class="syntaxdefault">is_a</span><span class="syntaxkeyword">?(</span><span class="syntaxdefault">Sketchup</span><span class="syntaxkeyword">;;</span><span class="syntaxdefault">Face</span><span class="syntaxkeyword">)</span><span class="syntaxdefault"> </span><span class="syntaxkeyword">}<br /></span><span class="syntaxdefault"> </span>
Now you can just refer to the faces collection instead of selection.
I figured there was an easier way to do that! I just didn't know how... thanks for the tip!
@thomthom said:
Is the plugin creating a colour for each selected face? What happens if you run this multiple times? I think you can easily end up with thousands of materials, which can really slow down the model - material list.
It's true... running this multiple times floods your "In Model" materials. Is there a way to apply a color without affecting the list? Perhaps that's a consequence of this tool, regardless of implementation.
@sdmitch said:
Using face.bounds.center failed in the sort but using ORIGIN.distance(face.bounds.center) did
faces=selection.select{|f| f.is_a?(Sketchup;;Face)}
> stack = {}
> faces.each{|face| stack[face]=ORIGIN.distance(face.bounds.center)}
> sorted = stack.sort { |a,b| a[1] <=> b[1] }.map { |n| n[0] }
> redness=0.0;greenness=255.0;blueness=0.0;colorincrement=(510/faces.length.to_f);
> sorted.each{|f|
> f.material = [redness.round,greenness.round,blueness.round];
> redness += colorincrement
> if redness >= 255.0
> redness = 255.0
> greenness -= colorincrement
> if greenness <= 0.0
> greenness = 0.0
> end
> end
> }
>
This is incredibly helpful! I had been trying today to figure out how to get thomthom's method to work, and I kept getting errors. Does anyone feel like explaining this line?
sorted = stack.sort { |a,b| a[1] <=> b[1] }.map { |n| n[0] }
I really can't make heads or tails of it. If not, that's okay, I'll probably investigate it on my own when I can.
Again, thanks to everyone who offered tips!
