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

Add gate sound effect #86104

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marc-weber1
Copy link

@marc-weber1 marc-weber1 commented Dec 13, 2023

Gate seemed like it was one of the effects obviously missing from the sfx that come with Godot, so I made a very basic one with only a threshold parameter.

This will hopefully have an attack/hold/release envelope.

@marc-weber1
Copy link
Author

Gate Test Project.zip

my test example if it saves anyone time

@fire
Copy link
Member

fire commented Dec 14, 2023

@YuriSizov what are the review rules for the less active Godot Engine teams?

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I am not that familiar with audio gates, but I tried to review code style. Sorry for not being substantial!

servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
ClassDB::bind_method(D_METHOD("get_threshold_db"), &AudioEffectGate::get_threshold_db);

ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "threshold_db", PROPERTY_HINT_RANGE, "-100,0,0.01,suffix:dB"), "set_threshold_db", "get_threshold_db");
}
Copy link
Member

Choose a reason for hiding this comment

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

Add new line at the end file.

doc/classes/AudioEffectGate.xml Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.h Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.h Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.h Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.cpp Show resolved Hide resolved
servers/audio/effects/audio_effect_gate.h Outdated Show resolved Hide resolved
doc/classes/AudioEffectGate.xml Outdated Show resolved Hide resolved
@marc-weber1
Copy link
Author

image
i got confused and made a state diagram,,,

@marc-weber1
Copy link
Author

The extra parameters seem to work completely fine now, freezing commits here unless there are any more review changes or system-specific issues that my computer isn't running into

@fire
Copy link
Member

fire commented Dec 18, 2023

Please merge into one commit as we review. Note that it'll require manual approval of cicd for first time contributors.

@marc-weber1
Copy link
Author

not sure how to rebase on the gate-sound-effect-rebase branch, i've just been making a new branch every time (using commandline git)

@YuriSizov
Copy link
Contributor

See this documentation, if you need help with squashing.

@marc-weber1 marc-weber1 force-pushed the gate-sound-effect-rebase branch from 75b657c to 40a61df Compare December 23, 2023 02:21
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 811ce36), it works as expected. Code looks good to me.

My only concern is about the default values, where it feels the noise gate is barely doing anything by default due to its very low threshold. With my microphone (Blue Yeti 2014), I need to change the threshold from -50 dB to -30 dB to completely eliminate background noise (-35 dB occasionally lets noise pass through). While the threshold is something users are expected to adjust individually for their microphone, we should try to provide a good default that has an immediately noticeable effect (so people don't think the filter is not working).

Additionally, the release window of 50 ms feels too aggressive.1 If I say something in the microphone, the last sound will often be cut off quite a bit. I've found that a release window of 500 ms works well here, although it might be better to increase both the hold and release windows to lower values like 250 ms each. I suggest testing the defaults on various microphones (e.g. studio microphone and gaming headset) to make sure it has a reasonable effect out of the box.

Footnotes

  1. This is one of my top pet peeves in a lot of VoIP apps. I get why it's done as a way to save a bit of bandwidth (which is debatable in 2024), but it distracts from the experience quite a bit.

@@ -0,0 +1,26 @@
<?xml version="1.0" encoding="UTF-8" ?>
<class name="AudioEffectGate" inherits="AudioEffect" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd">
Copy link
Member

@Calinou Calinou Jun 28, 2024

Choose a reason for hiding this comment

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

I'm curious whether this should be called AudioEffectNoiseGate, so that searching for "noise" in the class reference lets you find this effect.

In this case, I'd make it clear in the description that the effect does not perform noise removal (in the sense of RNNoise reduction), but just cuts off audio if it's lower than a threshold.

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

Successfully merging this pull request may close these issues.

5 participants