<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 21, 2016 at 7:41 PM, Søren Sandmann <span dir="ltr"><<a href="mailto:soren.sandmann@gmail.com" target="_blank">soren.sandmann@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Mon, Mar 21, 2016 at 12:29 PM, Bill Spitzak <span dir="ltr"><<a href="mailto:spitzak@gmail.com" target="_blank">spitzak@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span><div>On Sun, Mar 20, 2016 at 11:36 PM, Søren Sandmann <span dir="ltr"><<a href="mailto:soren.sandmann@gmail.com" target="_blank">soren.sandmann@gmail.com</a>></span> wrote:<br></div></span><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span>On Sun, Mar 6, 2016 at 8:06 PM,  <span dir="ltr"><<a href="mailto:spitzak@gmail.com" target="_blank">spitzak@gmail.com</a>></span> wrote:<br></span><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Bill Spitzak <<a href="mailto:spitzak@gmail.com" target="_blank">spitzak@gmail.com</a>><br>
<br>
The IMPULSE special-cases did not sample the center of the of the region. This<br>
caused it to sample the filters outside their range, and produce assymetric<br>
filters and other errors. Fixing this required changing the arguments to<br>
integral() so the correct point could be determined.<br></blockquote><div><br></div></span><div>I don't understand what is wrong and why this patch fixes it. Which region precisely did not have its center sampled? When IMPULSE filters are involved the width of the integral is 0 so there isn't really any "region" to sample.<br><br>Can you give a concrete example where the previous code produced asymmetric filters? Also, what "other errors" was produced? I think these examples should be added to the commit log.</div></div></div></div></blockquote><div><br></div></span><div>It sampled the *other* filter (the one that is not impulse) at the left edge of the region being passed, rather than at the location of the center of the impulse filter. This was detected by putting asserts in the filter functions to see if they were being called outside their width.</div></div></div></div></blockquote><div><br></div></span><div>I tried adding such asserts and I couldn't make them trigger with the scale demo. It would be helpful if you could give a specific pair of filters and scale factor where a filter is sampled outside its width.<br><br></div><div>And it really doesn't make sense to talk about the "region being passed" when one of the filters is IMPULSE. In that case, the width parameter is always 0 so there is no "region".</div></div></div></div></blockquote><div><br></div><div>Your version relies on width==0 when either filter is impulse. This is how it is being called right now, but I think it a very good idea to avoid this. This patch series does not include fixes I attempted to make the picture not vanish when impulse+impulse was selected. Those changes made the impulse filter act like it had a width of 1 and easily triggered the asserts.</div><div><br></div><div>I think my version is a lot clearer. The center of the sampling filter is at pos, an argument to the function, and the range for the integral is separated from the alignment and scale. I think it is obvious that this reduces the size of both the implementation and the calling code, in particular the sampling only needs a single argument rather than two.</div><div><br></div></div></div></div>