sfstoolbox / sfs-matlab

SFS Toolbox for Matlab/Octave
https://sfs-matlab.readthedocs.io
MIT License
97 stars 39 forks source link

Bug: Inconsistent delay_offset computation in time domain wfs driving functions #108

Closed fietew closed 8 years ago

fietew commented 8 years ago

The delay_offset return value of the delayline is given in samples, while the calculations in driving_function_imp_wfs are assuming seconds. Which convention is the correct one?

hagenw commented 8 years ago

Both are correct. The whole calculation in delayline() is done in samples as I found this to be more intuitive when working with integer delays and the whole calculation for the WFS driving functions is performed in seconds. But you are correct, there is a bug then in line 150 of driving_function_imp_wfs which reads

delay_offset = delay_offset + delayline_delay;

at the moment and should read

delay_offset = delay_offset + delayline_delay/fs;

instead.

Maybe we should avoid this by restricting us to use always t / s as input and output arguments in the whole Toolbox? At the moment sound_field_imp() uses also t / samples.

fietew commented 8 years ago

Yeah, as the purpose of delay_offset is to somehow incorporate its value into sound_field_imp() in order to get the correct time snapshot, it is quite complicated that one has to convert it from seconds back to samples. However, I don't know how many functions would be effected from such a change and if it's worth the effort.

fietew commented 8 years ago

Maybe, we should merge the fix and open another issue/pull request to discuss the seconds/samples convention?

hagenw commented 8 years ago

OK, I merged it and created a new branch time_in_sec where we can start work and discuss a change in the general time behavior (no pull request yet as this is not possible without a change in code).