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.

Sign up for GitHub

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

Jump to bottom

Fix volume interpolation in positional audio nodes #37279

Merged
merged 1 commit into from
Mar 26, 2020
Merged

Fix volume interpolation in positional audio nodes #37279

merged 1 commit into from
Mar 26, 2020

Conversation

Waridley
Copy link
Contributor

Should fix #22016
Volume was interpolating from either 0 or the current volume, instead of the previous volume.

The issue I was having was with long sounds moving around the player in 3D, not with the rapid, short sounds mentioned in the linked issue. Perhaps someone should verify that this fix actually fixes that issue before closing it, but this definitely fixes my issue, and it appears to me to be the cause of the short-sound issue as well.

@Waridley Waridley changed the title Use correct initial volume level Fix #22016 Mar 24, 2020
@Calinou Calinou added this to the 4.0 milestone Mar 24, 2020
@Waridley
Copy link
Contributor Author

Just downloaded Jitspoe's minimal reproduction project, and it does fix it for that as well.

@akien-mga
Copy link
Member

Thanks for the contribution!

There are a few things that need to be changed/improved:

Fix volume interpolation in AudioStreamPlayer

Fixes #37279.

@Waridley
Copy link
Contributor Author

You're right, it needed fixed in AudioStreamPlayer2D as well.

The basic AudioStreamPlayer seems to only support fading out at the moment, not fading in, and the logic for that seems correct. Should adding fade-in support be a separate PR?

Also... Is it possible to change which branch this PR is made against without making a separate PR?

@akien-mga
Copy link
Member

The basic AudioStreamPlayer seems to only support fading out at the moment, not fading in, and the logic for that seems correct. Should adding fade-in support be a separate PR?

That should likely be a separate PR (though we probably need to assess if it's missing by design or because it was forgotten.

Also... Is it possible to change which branch this PR is made against without making a separate PR?

Yes, though it's a bit tricky. You'd have to:

  • Change the target branch by editing the PR:
    Screenshot_20200324_213349
  • Cherry-pick your commit on a new branch locally, based on master
  • Force push that local branch to your remote fix_22016 branch

@Waridley Waridley changed the base branch from 3.2 to master March 25, 2020 01:08
@Waridley
Copy link
Contributor Author

Waridley commented Mar 25, 2020 •
edited
Loading

I think it tried to run the CI in between me force-pushing the new branch and me changing the base branch because I did them in the wrong order, but it should be correct now.

@akien-mga
Copy link
Member

Indeed, the CI doesn't seem to properly support PRs changing base branch it seems. Could I ask you to do another (noop) force push to force CI to restart?

@akien-mga akien-mga changed the title Fix #22016 Fix volume interpolation in positional audio nodes Mar 25, 2020
@Waridley
Copy link
Contributor Author

Sure. Rebased onto a more recent commit anyway.

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 25, 2020
@akien-mga akien-mga merged commit 4507d0c into godotengine:master Mar 26, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.2.2.

@akien-mga akien-mga removed cherrypick:3.x Considered for cherry-picking into a future 3.x release needs testing labels Apr 16, 2020
@Waridley Waridley deleted the fix_22016 branch February 2, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga Awaiting requested review from akien-mga

@reduz reduz Awaiting requested review from reduz

Assignees
No one assigned
Labels
bug topic:audio
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

Audio clipping / static / interference on rapid intervals of sound (fixed in master)
3 participants
@Waridley @akien-mga @Calinou