fluid-dev
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[fluid-dev] Poly/mono patch - ticket 160 (was: Re: New patch: polyphonic


From: Marcus Weseloh
Subject: [fluid-dev] Poly/mono patch - ticket 160 (was: Re: New patch: polyphonic key pressure (aftertouch))
Date: Fri, 2 Jun 2017 11:01:06 +0200

Hi Jean-Jacques,

2017-05-31 19:08 GMT+02:00 Ceresa Jean-Jacques <address@hidden>:
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

  
 

reply via email to

[Prev in Thread] Current Thread [Next in Thread]