Add reverb and chorus settings #385
Conversation
as suggested by #49
should now be fetched through a settings instance
test/test_synth_chorus_reverb.c
Outdated
| TEST_ASSERT(settings != NULL); | ||
|
|
||
| TEST_SUCCESS(fluid_settings_setnum(settings, "synth.reverb.roomsize", 1.0)); | ||
| TEST_SUCCESS(fluid_settings_setnum(settings, "synth.reverb.damp", 1.0)); |
There was a problem hiding this comment.
If you would use different values for the parameters, the tests could actually check that the correct value is returned/set for a named parameter.
There was a problem hiding this comment.
the tests could actually check that the correct value is returned/set for a named parameter.
This is done below by asking the synth to return these values (which he must have read from the settings object to be correct). For the correct saving of the settings, relying on the return value of the settings API is sufficient here.
There was a problem hiding this comment.
Sorry, I mean by settings damp to 1.0, roomsize to 0.9, etc we could test that fluid_synth_get_reverb_damp does actually return the value set by synth.reverb.damp, and not the one set via synth.reverb.roomsize.
There was a problem hiding this comment.
Oh ok. Yeah makes sense. Will do that + test the realtime settings functionality to update properly as well.
test/test_synth_chorus_reverb.c
Outdated
|
|
||
| TEST_ASSERT(fluid_synth_get_chorus_nr(synth) == 99); | ||
| TEST_ASSERT(fluid_synth_get_chorus_level(synth) == 1.0); | ||
| TEST_ASSERT(fluid_synth_get_chorus_speed_Hz(synth) == 1.0); |
There was a problem hiding this comment.
I just noticed that there is this capital "H" in fluid_synth_get_chorus_speed_Hz. Maybe we should clean that up as well before the 2.0 release?
There was a problem hiding this comment.
Not sure. This function is already in use for 1.1.x for long. I dont like it as well, if you'd asked me I would simply call it *_speed(). However one might consider this as an unnecessary API change, motivated by personal taste.
There was a problem hiding this comment.
I think it's more than personal taste... it's about consistency in the API. All assuming this is the only function that uses a capital letter in the API. If it is, then I would consider this a bug. But then again, how much you value consistency is definitely personal taste... :-)
There was a problem hiding this comment.
Alright, consistency is a good argument. Shall we call it fluid_synth_get_chorus_speed() then?
There was a problem hiding this comment.
Yes, sounds good. Especially as the setter is called fluid_synth_set_chorus_speed. Which leads to another possible cleanup, I guess: we currently have fluid_synth_set_chrous_depth and fluid_synth_get_chrous_depth_ms.
src/synth/fluid_synth.c
Outdated
| @@ -3367,6 +3428,53 @@ fluid_synth_render_blocks(fluid_synth_t* synth, int blockcount) | |||
| return blockcount; | |||
| } | |||
|
|
|||
| /* | |||
| * Handler for synth.reverb.* settings. | |||
There was a problem hiding this comment.
Maybe also mention synth.chorus.* settings and explain the _num (floating point) and _int (int) distinction?
|
|
room-size would be more consistent with the other settings, I think. We already have things like media-role, period-size, sample-format, cpu-cores, min-note-length. |
fluid_synth_get_chorus_speed() and fluid_synth_get_chorus_depth()
|
Ok, should be good now. |
|
Yes, looks good! Just wondering if we should have a deprecation path for the renamed API functions? |
|
Dont think that's necessary, it's mentioned in the overall 2.0 changes. |
This introduces reverb and chorus realtime settings as suggested by #49. It also deprecates reverb and chorus shell commands and removes the
FLUID_*_DEFAULT_*macros from the public API, as they can now be retrieved via the settings object.Closes #49.