Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from

Conversation

FulopNandor
Copy link
Contributor

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

  • elimination of the per-note based threshold limiting from the main loop of DexedAudioProcessor::pprocessBlock(), which sometime causes some distortion of the output sound (see more details here)
  • implementation a tricolor VU meter for the main output (see more details here and here)
  • implementation of 200%, 300% and 400% size scaling factor for the GUI of Dexed, which was asked by kind user trancybabe recently, who suddenly transmuted into a not so kind user ghost... However, the possibility of the enlargement to 200% was originated in issue titled Improve scaling for HighDPI displays #308; (see more details here)
  • dB based scaling of VU meters of operators and the main output (see more details here)
    Thank you in advance!

Copy link
Owner

@asb2m10 asb2m10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forcing < 1.0 version on plugin version will break DAW compatibility on existing user projects. Once we hit 1.0.0 "github" version, we can sync cmake project (1.0.0) version with "github" (for now 0.9.7) release version.

Also, let the pipeline insert the version; it is already based on tag name; the version should not be in the source code but based on pipeline context. This is already taken care of.

@asb2m10
Copy link
Owner

asb2m10 commented May 23, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

asb2m10 commented May 24, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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:

  1. I navigated to the subdirectory of my local repository:
    cd to-my-local-repo

  2. 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)
  1. 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
  1. 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``
  1. 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:

  • the response was something along the lines of "there is nothing to commit",
  • the number of commits ahead of asb2m10/dexed:master decreased to 6 from 7 when I checked my remote repository in a web browser.
    Therefore, I concluded that everything was in order.

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
Copy link
Owner

asb2m10 commented May 30, 2024

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.

@asb2m10 asb2m10 closed this May 30, 2024
@FulopNandor
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants