asb2m10 / dexed

DX7 FM multi plaform/multi format plugin
GNU General Public License v3.0
2.86k stars 246 forks source link

reducing certain kind of distortion, implementation of dB-scaled VU meters and 200-300-400% GUI size selections #423

Closed FulopNandor closed 6 months ago

FulopNandor commented 6 months ago

Dear Pascal, do apply my current pull request, including my last 5 commits, please, which are related to the following topics:

asb2m10 commented 6 months ago

Except for the "version" insert, all of this should be merged. I will wait for the "final" PR.

Thanks for the dB scaling, this was on my TODO list.

FulopNandor commented 6 months ago

Thanks for the reply and explanation, I will close the current PR, remove the faulty version number update and send the new PR after it. Sorry in advance, but I probably won't be ready until tomorrow at the earliest.

asb2m10 commented 6 months ago

No worries!

You didn't have to close this PR, just revert the "AboutBox version" commit and let me test this on the 3 platforms afterwards, once everything is OK, I'll be happy to merge this.

Keep in mind that a PR should not change once it is requested unless there is a fix based on that PR, otherwise the validation restart once there is a new commit/feature.

FulopNandor commented 6 months ago

Dear Pascal, I have deleted the last commit, related to modification of the version number displayed in the AboutBox. Thank you for your patience.
And sorry for the troubles caused, I am totally new to GitHub and I have no former experience, your Dexed project is the first one, which I have tried to contribute to.

FulopNandor commented 6 months ago

Dear Pascal,

Should I wait for a little bit more, or should I do something else?

I’m afraid I might have messed up or forgotten something. If so, please help - I am detailing here what I have done until now: After writing my response last week , I took the following steps:

1) I closed the Pull Request.

Then, from the Command Prompt in my local repository, I executed the steps below:

2) I navigated to the subdirectory of my local repository: cd to-my-local-repo

3) I verified that I was in the correct location: git remote -v

The output was:

origin  https://github.com/FulopNandor/dexed.git (fetch)
origin  https://github.com/FulopNandor/dexed.git (push)
upstream        https://github.com/asb2m10/dexed.git (fetch)
upstream        https://github.com/asb2m10/dexed.git (push)

4) I confirmed that I was on the correct branch: git branch -a

The output was:

* master
  remotes/origin/HEAD -> origin/master
  remotes/origin/master
  remotes/upstream/HEAD -> upstream/master
  remotes/upstream/gh-pages
  remotes/upstream/master

5) I reverted back to the last commit, before the faulty commit that contained the incorrect version number update: git reset --hard HEAD~1

The output was:

HEAD is now at 755544f maintenance of source code by identification of certain unused parts``

6) I initiated a forced update to my remote repository: git push origin +master

Oops, I made another mistake here: I forgot to record the response of this command. However, I do remember that:

Unfortunately, I had already completed the steps above, including closing the PR, before I read your message.

Yesterday, I checked again to see if the Pull Request had been successful, but it seemed to still be in "Changes requested" status. I reread the messages above and concluded that I might have misunderstood the last one. I thought that

. . . once it is requested unless there is a fix based on that PR . . .

could mean that another commit is needed to meet the “Changes requested” condition. So, I uploaded a new commit update the settings of relative volumes of the engines. But I am afraid this might have been a newer mistake...

How should I fulfill the red "1 change requested" requirement now?

In short: I am waiting for your response on what to do next, I promise not to change anything until it arrives.

asb2m10 commented 6 months ago

Hi, this is normal since you asked to merge your fulopnandor/master on asb2m10/master. Each time you commit on your repo on master, the PR will reflect the changes.

The correct way to handle is to create a specific branch on your side (eg fulopnandor/guiscaling for example) and PR it on asb2m10/master. This way fulopnandor/master is independent on the original PR request.

By the way, we might have a problem with the (reducing certain kind of distortion) commit. I see that some engine are less loud than the previous version. This is a no go since 0.9.6 user will have to adjust Dexed volume on all tracks once they upgrade to the potential 0.9.7 version. Also the "distortion" is how DX FM synths are making noise, it should not be softened.

image

That said, I'm happy to merge to "dB-scaled VU and scaling gui values" suggestion. Create a branch on your side based on asb2m10/master and insert those changes, I'll be happy to merge them.

FulopNandor commented 6 months ago

Dear Pascal,

Thank you very much for your answer explaining my problem. It has brought me a big step closer to understanding how the RPs and branches are related in the practice.

Thank you also for the notification about the differences in the output volume of the engines. I also realized this in the meantime, unfortunately after I initiated my recent PR. The mistake was that I used a single correction factor for each engine to achieve equal loudness for the given note velocities. (Even though my initial idea was that since there are different maximal amplitudes, different values should be needed for balancing the loudness.) So, I created my last commit containing three different correction factors - but it was a little late. :-)

Returning to the issue of distortion, of course, I do not doubt that a certain type of distortion occurs in the case of DX FM synthesizers. However, I would like to send a brief summary of why I came to the conclusion that instead of the currently applied ‘per note’ based threshold cutting in DEXED for all three engines, I recommend another implementation according to the already uploaded commit.

So, I would like to ask for your patience while I prepare this summary and also prepare a separate branch for the commit containing the VU meter.

Thank you again!