search

SketchUp / sketchup-stl

A SketchUp Ruby Extension that adds STL (STereoLithography) file format import and export.
http://extensions.sketchup.com/content/sketchup-stl
MIT License
248 stars 68 forks source link

Change variable to a constant #82

Closed ChrisFullmer closed 11 years ago

ChrisFullmer commented 11 years ago

In importer.rb line 433:

offset_reverse could be a CONSTANT (initialized at the top of the class with the other constants) if it is always (0,0,-1) to help speed up its use on line 440.

jimfoltz commented 11 years ago

Can we link to the line?

https://github.com/SketchUp/sketchup-stl/blob/master/src/sketchup-stl/importer.rb#L433

Why yes. Yes we can.

ChrisFullmer commented 11 years ago

I see it being used on line 440

https://github.com/SketchUp/sketchup-stl/blob/master/src/sketchup-stl/importer.rb#L440

jimfoltz commented 11 years ago

I'm now wondering why a constant would be faster? Both the current local variable and a constant would just be a reference to an Array of one Geom::Vector3d object in memory. What makes the constant faster if both are references?

ChrisFullmer commented 11 years ago

hmm, I think I ran a faulty speed test earlier on this. In my testing it was about 5 times faster. Over 10,000,000 iterations, it save 2 seconds :)

I just re-created it and realized my logic was flawed. In my earlier test, I had put the "offset_reverse = [Z_AXIS.reverse]" part INSIDE my loop, so it recalculated it each time it looped.

Looking at it again, I see no speed benefit in making this change.

thomthom commented 11 years ago

So this is a closed case?