Hi Jean-Jacques,
2017-05-31 19:08 GMT+02:00 Ceresa Jean-Jacques <
jean-jacques.cer...@orange.fr>:
> To give an example, this idea has been implemented partialy in a previous
> patch called "Poly/mono patch" (Ticket 160).
>
I was very happy when you posted that patch and I had a good look through
it. My instrument behaves very similar to a breath controller, so it would
probably benefit a great deal from the functionality your patch adds to
FluidSynth.
But I must admit that I found it quite hard to review and understand the
patch. Mainly because it is so large and adds and changes so many separate
parts of the codebase simultaneously: new shell commands, legato
functionality, instrument zones, breath mode, maybe more? Also the fact
that your patch adds yet another coding style to the codebase made me shy
away from it at first (but that is only a minor concern and easily fixed).
When you implemented all the changes for ticket 160, did you maybe make
smaller local commits for the individual features that you could provide as
separate patches?
Please don't get me wrong: I've tested the patch and the test cases you
provided all seem to work as intended. So I'm very grateful for your work!
I just think that splitting up the patch into smaller chunks might increase
the chances of it getting reviews and eventually making it into the
mainline codebase.
There is your very detailed PDF that describes your ideas and most of the
changes, of course. But its great detail also has it's downsides, at least
for me: I found it hard to keep track of everything. As an example, I
noticed the following change in your patch:
-------------- cut ---------------------
diff -Naur ./fluidsynth-1.1.6\src\rvoice\fluid_adsr_env.h
./fluid-polymono-0003\src\rvoice\fluid_adsr_env.h
--- ./fluidsynth-1.1.6\src\rvoice\fluid_adsr_env.h Tue May 19 12:27:02 2015
+++ ./fluid-polymono-0003\src\rvoice\fluid_adsr_env.h Wed Jun 22 13:37:59
2016
@@ -95,9 +95,11 @@
env->section++;
env->count = 0;
}
+ else env->count++;
env->val = x;
- env->count++;
+
+
}
/* This one cannot be inlined since it is referenced in
-------------- cut ---------------------
I wondered for a long time if you were fixing a bug here or adding new
functionality. Only just now I had another look through the PDF and
actually found the place where you mention that it is a bugfix. In my
opinion, this type of change should really be in a separate patch, so that
it can be fixed quickly even if your larger feature is delayed.
All the best,
Marcus
_______________________________________________
fluid-dev mailing list
fluid-dev@nongnu.org
https://lists.nongnu.org/mailman/listinfo/fluid-dev
