-
Notifications
You must be signed in to change notification settings - Fork 20
C2LC-317: Adjusted sound panning to match new scene dimensions. #140
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
C2LC-317: Adjusted sound panning to match new scene dimensions. #140
Conversation
src/AudioManagerImpl.js
Outdated
@@ -161,7 +162,7 @@ export default class AudioManagerImpl implements AudioManager { | |||
// As we use a single Sampler grade, our best option for panning is | |||
// to pan all sounds. We can discuss adjusting this once we have | |||
// multiple sound-producing elements in the environment. | |||
const panningLevel = Math.min(1, Math.max(-1, (0.1 * characterState.xPos))); | |||
const panningLevel = ((characterState.xPos / sceneDimensions.getWidth()) * 2) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The panningLevel
calculation isn't quite symmetrical. The xPos
varies from 1-26 inclusive.
- At
xPos
of 1,panningLevel
is -0.92 - At
xPos
of 26,panningLevel
is 1
We'd want a calculation like:
const panningLevel = ((characterState.xPos - minX) / (maxX - minX)) * 2 - 1;
Where minX
and maxX
are from the SceneDimensions
. Unfortunately, right now SceneDimensions
will not give us the right min X and max X. It may be too risky to rework SceneDimensions
at this point in the 0.7 timeline. I have another thought on panning, which I will leave as a separate comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the new coordinate system, the SceneDimensions
constructor should really now be:
constructor(minX: number, maxX: number, minY: number, maxY: number)
So we can construct with:
new SceneDimensions(1, 26, 1, 16);
But as I mentioned above, it may be too risky to rework at this stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hard-coded the current minimum in an adjusted calculation of the midpoint, so that the panning is symmetrical.
I also left a TODO in the relevant area about using minX and maxX.
I asked @chosww to help me test this PR as he initially reported the issue. And he was initially still not hearing sound at the last column Z. As we talked, we figured out that it was because he was testing with only 1 earbud in: the left. Which made me think that hard panning from full left to full right might not be quite the best thing. As it will create a worse experience for people only hearing one of the speakers/ears. Some ideas:
|
I asked Sepideh for her thoughts on panning in the C2LC General room on Thursday April 1 and she would like to keep the panning for 0.7 as she would like to try it out with the workshop co-designers. So, we either keep the full panning from full left to full right, or we maybe try a slightly reduced range, so that some audio is always present in each ear. |
Thanks for all the feedback, I will work on improving the panning so that it never drops below 25% in either ear, and so that it's properly symmetrical. |
I added a limiter on the panning range so that there is always at least some sound in each ear, but so that the panning still clearly indicates which "side" of the median the character is on. I also ensured that the panning is symmetrical. |
See C2LC-317 for details.