vedderb / vesc_express

The source code for the VESC Express
GNU General Public License v3.0
39 stars 31 forks source link

Fix missing pixels in fill_circle #20

Closed r3n33 closed 6 months ago

r3n33 commented 6 months ago

When working with large filled circles I was noticing what looked like missing pixels in the lower right quadrant:

before

This PR replaces the default case for large circles without the unexpected results: after

vedderb commented 6 months ago

Nice work!

Did you compare it to the old version to see if it still gives the same size in pixels as before? I know that @laxsjo spent some time on making sure that circles are pixel perfect (although as you demonstrated they weren't pixel perfect :-) ).

Also, did you compare the performance to the old version? It is always a nice sanity check just to make sure that nothing unexpected is going on. You can do something like this to test the performance:

(def img (img-buffer 'indexed2 200 200))

(def iterations 100)
(def start (systime))
(looprange i 0 iterations {
        (img-circle img 100 100 80 1 '(filled))
})
(def ms (/ (* 1000.0 (secs-since start)) iterations))

(print (str-from-n ms "Time: %.1f ms"))
r3n33 commented 6 months ago

I will run some tests and see how this implementation performs in comparison, thanks!

r3n33 commented 6 months ago

This does not look good on my end :thinking: My code takes 12.9ms compared to 6.5ms with the original implementation. I will try timing an alternate implementation tomorrow as I have run out of time today.

r3n33 commented 6 months ago

^ the latest commit is considerably faster but still comes in at 8.2ms

vedderb commented 6 months ago

Nice work! Merged.