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

Positional Audio: Do not assume Y up for small errors #4172

Merged
merged 1 commit into from May 25, 2020

Conversation

vimpostor
Copy link
Contributor

@vimpostor vimpostor commented May 14, 2020

If front and top are not perpendicular by a small margin, it is better
to find a perpendicular vector that differs the least from top.

Only for large errors, i.e. if front and top are not perpendicular at
all, it makes sense to create a totally new top vector by assuming Y is
up.

This fixes distorted audio if the positional audio plugin would
sometimes (but only rarely) deliver errors larger than 0.01, in which
case Mumble would rapidly switch between Y up and the real up vector.

Fixes #4169

See the attached screenshot for old behaviour vs new behaviour (old corrected top is a totally new perpendicular vector, while new corrected top is the closest perpendicular vector (closest to top, perpendicular to front):
Screenshot_20200514_160106

@vimpostor
Copy link
Contributor Author

I might fix this with the GTA V plugin directly as well (because surely I can find a memory address with a top vector that is always exactly perpendicular, instead of approximately perpendicular), but this is definetely bad behaviour from Mumble as well, as it does not make sense to create a totally new vector, if it is only not perpendicular with a small error.

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

LGTM but I never really understood the applied logic in this code area. I'm lacking an understanding of what the different directions are meant to represent. That being said though my vector geometry understanding is enough to say that it at least looks senseful from a mathematical point of view :D

@vimpostor
Copy link
Contributor Author

Just pushed a small correction (forgot to normalize top again, but wasn't really an issue since top is changed only slightly after it was already normalized).

I might create a separate PR to refactor the math in this entire file a bit. Stuff like normalizing, calculating length, dot product etc are used multiple times in this file, but are always done from scratch. It would be better if we would create a Util/math file or something, where we can reuse this stuff.

@Krzmbrzl
Copy link
Member

Krzmbrzl commented May 15, 2020

The refactoring is actually already done in my Plugin framework PR ☝️
(at least the dot product and normalizing stuff)

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

I was just about to merge this PR when I noticed that the commit message style doesn't quite fit our conventions. Could you please change it to e.g.

src/mumble/AudioOutput.cpp: Do not assume Y up for small errors

<The rest of your commit message>

If front and top are not perpendicular by a small margin, it is better
to find a perpendicular vector that differs the least from top.

Only for large errors, i.e. if front and top are not perpendicular at
all, it makes sense to create a totally new top vector by assuming Y is
up.

This fixes distorted audio if the positional audio plugin would
sometimes (but only rarely) deliver errors larger than 0.1, in which
case Mumble would rapidly switch between Y up and the real up vector.

Fixes mumble-voip#4169
@vimpostor
Copy link
Contributor Author

Done.

Though I can't resist to comment on this that I absolutely despise this commit style:

  • It is literally against the official git commit guidelines. The first line of a commit message should not exceed 50 or so characters and the first line should be left concise and short. Prepending the entire file path does not leave enough characters
  • The same information can be retreived via git show --name-only --pretty=format: 052d2d89
  • What happens if you change multiple files in the same commit? Should you add all paths?

Overall this does not really work with git nicely, but that's just my 2 cents

@Krzmbrzl
Copy link
Member

I agree that it's a bit dubious style but consistency has priority here. But in fact that something that'll probably discussed at our next team meeting ;)

@Krzmbrzl Krzmbrzl merged commit 40f2abf into mumble-voip:master May 25, 2020
@Krzmbrzl
Copy link
Member

Thank your for your contribution! :)

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.

Positional Audio distorted when top vector is moving
3 participants