search

sfstoolbox / sfs-matlab

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

zero-padding of HRIRs to conf.N before delayline() #89

Closed VeraE closed 8 years ago

hagenw commented 8 years ago

It works nicely.

There is only one drawback of the current solution. As ir_correct_distance() now always returns an impulse response with the length conf.N, there is no way to get the original impulse response from the SOFA file with get_ir(). This was possible before, if you have specified conf.ir.useoriglength = true and chosen the same distance for the desired impulse response as the measured impulse response provided.

Do you think this is ok, and we should abandon that feature as you can simply set conf.N after the length you can lookup in the SOFA file. If so, would it also make sense to abandon the conf.ir.useoriglength setting altogether?

/cc @fietew

VeraE commented 8 years ago

Hm, I'm not sure when you need this inside the SFS Toolbox to load the impulse responses in their original length. Do you have an example? If it is necessary to have this we could move the zero-padding of the HRIRs to conf.N inside if ~useoriglength and adapt the new if condition for the length check. By the way, this question would not arise if we didn't use conf.N... :)

Regarding the question of keeping conf.ir.useoriglength, I'm also not sure what it is used for...?

hagenw commented 8 years ago

You are probably right, we don't need this inside the Toolbox.

But as there are no other functions to work with SOFA files, I have also used get_ir() in order to work with SOFA files. And in this case I used conf.ir.useoriglength in order to get exactly the signal that is stored inside the SOFA file.

But I agree, we should remove this functionality.

Regarding conf.N, I have to think about this and would propose to not include changes to it into this pull request.

hagenw commented 8 years ago

Maybe we can also remove this from ir_correct_distance()?

% Stop extrapolation for distances larger than 10m
if any(ir_distance>10)
    ir_distance = min(ir_distance,10);
    warning(['%s: Your desired radius is larger than 10m, but we will ', ...
        'only extrapolate up to 10m. All larger radii will be set to ', ...
        '10m.'],upper(mfilename));
end
VeraE commented 8 years ago

I wouldn't mind if you want to hold on to the possibilty of loading SOFA files in their original condition via get_ir(). It could be done as described above. But it would be cleaner without that.

The distance restriction to 10 m can be removed in my opinion. It would even be disturbing if you wanted to calculate something like an auralization of an image source model.

hagenw commented 8 years ago

The problem of conf.N is continued in #90, the rest can be merged.