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

Doppler effect #115

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Doppler effect #115

wants to merge 13 commits into from

Conversation

Roms1383
Copy link
Contributor

Here's my attempt at implementing Doppler effect. I'm by no means a mathematics person, but it sounds right when (briefly) tested. I'm gonna let it as a draft for now, as there's some cool additions I'd like to make.

@Roms1383
Copy link
Contributor Author

I realized 2 points that I'm gonna address:

first the speed_of_sound is better left as a parameter because its calculation depends on the materials the sound travels through (instead of temperature, index and mass which are mostly for gas, but do not apply to wood, concrete, etc).

Second, being not familiar with audio I applied the result to the Frame while it appears that doppler effect is meant to alter the pitch. I could apply it to playback_rate, but playback rate alters both volume and pitch. If you have some heads up do not hesitate to share, otherwise I'll keep looking into it and test another approach.

As a side note I added an asset for tests that I'm not sure is free-to-use, so eventually I'll rebase and force-push to remove it if needed: it's just that it's way easier to test with the sound of a motor engine than the current assets found in the examples.

@BrianWiz
Copy link

BrianWiz commented Jan 14, 2025

@Roms1383 Dammit, I was going to take this feature :P

I too am not a super math audio nerd. I also have not looked at your code in detail yet (and I'm new to Kira).

However, having worked on a couple shooters, the doppler effect is best when you can tune it/amplify it per sound emitter. If you look at FMOD they just straight up have a multiplier on it, which isn't all that realistic but I find the effect is very much a stylistic choice more than anything. For some things you will want it toned down, for other things you want it cranked up (usually on projectiles), for things like modern vehicles you'll want it somewhat realistic because we're just so used to hearing it in real life, so there's a certain level of expectation there. Point being, having that control is super important.

I will check out the PR tonight.

@tesselode
Copy link
Owner

@Roms1383 Yes, doppler effect is definitely meant to affect pitch, not volume. I'm not sure what you mean by playback rate affecting volume and pitch...did you mean speed and pitch? If so, I believe a doppler effect should affect both.

@Roms1383
Copy link
Contributor Author

Overall I think the maths are correct, but I didn't apply it to the right parameters (it shouldn't affect the Frame directly, but more like the playback rate currently does). I briefly looked into the playback rate implementation this morning and I'm not sure yet how to proceed because the playback rate is defined on the sounds which are (rightfully) decoupled from Effect. I need more time, and I definitely appreciate your help.

@tesselode
Copy link
Owner

Maybe look into how oddio does it?

@Roms1383
Copy link
Contributor Author

Hi everybody, sorry for the delay.

I took a brief look, but not sure exactly where to do the resampling.
Effect does seem like where it belongs, but since it doesn't have access to existing resampler (which afaict lives in StaticSound), I wonder if I'm supposed to duplicate the machinery into Effect.

@tesselode I didn't get enough time to dig deep into oddio, but at first sight their overall implementation seems quite different (got surprised that they actually take into account ears position for doppler, and handle potential artifact when e.g. player teleports).

I'm quite overworked atm so @BrianWiz if you want to take over (even reuse ideas / snippets of code) feel free to do so I won't take it personally. Otherwise, I'll keep looking into it at a later point.

@BrianWiz
Copy link

BrianWiz commented Mar 4, 2025

Hey! I'm ready to take a look at this now. @tesselode if I make progress, should I start another PR?

Or maybe @Roms1383 would be willing to give me permission to your fork, then I can commit directly onto this branch

EDIT:

Okay I'm not actually seeing how it's currently affecting the playback rate if at all right now. I've been struggling to actually hear the effect. So I think that's going to be the trick, is getting the playback rate set correctly.

EDIT 2:

Okay I see that's what you already mentioned

@Roms1383
Copy link
Contributor Author

Roms1383 commented Mar 4, 2025

Or maybe @Roms1383 would be willing to give me permission to your fork, then I can commit directly onto this branch

I totally be willing to ! I just don't know where to set it, could you point me where ?

@BrianWiz
Copy link

BrianWiz commented Mar 5, 2025

@Roms1383 yeah you need to go to your fork in GitHub, then you'll see Settings at the top, and then on the left sidebar, Collaborators. Then you can send an invite from there.

@Roms1383
Copy link
Contributor Author

Roms1383 commented Mar 5, 2025

Done @BrianWiz 👌

@BrianWiz
Copy link

BrianWiz commented Mar 5, 2025

So I managed to get something working

  • I simplified the code a bit, but definitely used Rom's code as a guide among other resources
  • It doesn't use an effect. It's just an extension of the spatial settings, which honestly might be more correct? I wasn't able to figure out an elegant way to get the playback speed into the effect's process function. I was thinking of just passing in a reference to a value and then use that value to affect the playback speed but I thought that would be a bit weird seeing as it's probably the only effect that would ever need it.
  • The other issue I came across was the velocity computation was wrong, it needed to be divided by delta time. So then I thought of using the dt value that's passed around in Kira, but it's the delta time for the audio thread which is the wrong value in this case. I needed the delta time of the game loop itself. So the less-than-ideal part about this feature is you need to also set the game_loop_delta_time on the spatial track & listener.
  • I used a different sound, for two reasons, I know it's free to use, and it's a more constant sound for better hearing

Example of what I mean:

let mut spatial_track = audio_manager
  .add_spatial_sub_track(
	  &listener,
	  SPATIAL_TRACK_POSITION,
	  SpatialTrackBuilder::new()
		  .distances(SpatialTrackDistances {
			  min_distance: 1.0,
			  max_distance: 400.0,
		  })
		  // NOTE: Even though the doppler effect is enabled, the sound will not be affected by it
		  // until the listener and the spatial track have set the game loop delta time. See below!
		  .doppler_effect(true)
  )
  .unwrap();
  
  loop {
    let delta_time = get_frame_time();  // Macroquad's delta time
    
    // this will track the previous position, but not enough, need to divide by delta time to get the correct velocity
    listener.set_position(camera_controller.position, Tween::default());
    spatial_track.set_position(some_position, Tween::default());
    
    // need to set this every frame unless you're dealing with a fixed timestep
    listener.set_game_loop_delta_time(delta_time);
    spatial_track.set_game_loop_delta_time(delta_time);
  }

The other option would be the user has to set the velocity directly, but I figured it's easier for them to set the delta time. Any other way would be a lot more involved I think.

The following video uses a speed of sound at 343 m/s

2025-03-04.19-23-07.mov

edit: @Roms1383 noice! Thank you, code pushed

Comment on lines +341 to +344
// "Sonic boom"
// It may sonic boom at the start of the emitter's life because the previous position
// is 0,0,0 not sure how to get around this, or if it even matters.
// But it would be kind of cool to emit an event when this happens.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think what you mention here is what is described in oddio over POSITION_SMOOTHING_PERIOD and further in set_motion. Hope this helps :)

Fantastic job by the way, I'm super hyped!

Copy link

@BrianWiz BrianWiz Mar 6, 2025

Choose a reason for hiding this comment

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

Another thing that could solve this particular scenario is on the first time we set position, the previous position is the same. That would make the velocity 0,0,0 on spawn and not cause a sonic boom (I like to pretend it's really causing a sonic boom)

@BrianWiz
Copy link

@Roms1383 I think this can probably be moved out of draft and into review

@Roms1383 Roms1383 marked this pull request as ready for review March 12, 2025 06:25
@tesselode
Copy link
Owner

@BrianWiz Thanks for your work on this! This works pretty well, but I do have a couple thoughts:

  • Could this work with proximity voice chats? Not that Kira has anything like that built-in, but I'd like people to be able to implement their own Sounds for voice chats and have everything work as expected. You're getting the pitch modulation by changing the delta time, which works fine for finite sounds, but wouldn't work for live streaming sounds that need to run in real time. I believe oddio saves the incoming sound into a buffer and then uses the distance of the sound emitter from the listener as an offset for reading from the buffer. As the sound gets closer, the offset decreases, which naturally gives you the velocity change you want. It may incur some additional latency, though, I'm not sure.

  • Do we really need a game loop delta time argument? I was hoping the velocity could be derived from the tween duration.

  • We should have some way to signal whether a track position change is a continuous movement or a teleport.

@BrianWiz
Copy link

I'll look into using tweens this weekend!

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.

3 participants