bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Optional override for global spatial scale

Open rparrett opened this issue 1 year ago • 8 comments

Objective

Fixes #10414.

That issue and its comments do a great job of laying out the case for this.

Solution

Added an optional spatial_scale field to PlaybackSettings, which overrides the default value set on AudioPlugin.

Changelog

  • AudioPlugin::spatial_scale has been renamed to default_spatial_scale.
  • SpatialScale is no longer a resource and is wrapped by DefaultSpatialScale.
  • Added an optional spatial_scale to PlaybackSettings.

Migration Guide

AudioPlugin::spatial_scale has been renamed to default_spatial_scale and the default spatial scale can now be overridden on individual audio sources with PlaybackSettings::spatial_scale.

If you were modifying or reading SpatialScale at run time, use DefaultSpatialScale instead.

// before
app.add_plugins(DefaultPlugins.set(AudioPlugin {
    spatial_scale: SpatialScale::new(AUDIO_SCALE),
    ..default()
}));

// after
app.add_plugins(DefaultPlugins.set(AudioPlugin {
    default_spatial_scale: SpatialScale::new(AUDIO_SCALE),
    ..default()
}));

rparrett avatar Nov 06 '23 19:11 rparrett

I will try to rebase this soon.

rparrett avatar Jan 14 '24 14:01 rparrett

Maybe we also want to add a DefaultSpatialScale setting? I felt like doing this would be sort of ugly -- maybe requiring spatial_scale to be an Option<SpatialScale>, so I didn't do that.

Three months later, I feel like this might be worth the effort. Anyone have an opinion?

rparrett avatar Jan 14 '24 14:01 rparrett

Maybe we also want to add a DefaultSpatialScale setting? I felt like doing this would be sort of ugly -- maybe requiring spatial_scale to be an Option, so I didn't do that.

Three months later, I feel like this might be worth the effort. Anyone have an opinion?

DefaultSpatialScale sounds like a good idea. Setting a ton of entities to the same value would be painful. Would it just be a Resource who's value would be used if .with_spatial(true) is used without .with_spatial_scale()? Where does the Option come in?

Ixentus avatar Jan 14 '24 16:01 Ixentus

Yeah, I guess things would work more or less like before this PR for configuring the default scale, but PlaybackSettings would need to use an Option<SpatialScale> to make

used if .with_spatial(true) is used without .with_spatial_scale()

doable. But users wouldn't really have to deal with it, I guess. with_spatial_scale could still take a SpatialScale.

rparrett avatar Jan 15 '24 14:01 rparrett

It was easier to just re-implement the changes than it was to fix merge conflicts, so I did that and added DefaultSpatialScale. That wasn't a super small change, so I'd appreciate a re-review.

rparrett avatar Jan 16 '24 01:01 rparrett

Much more convenient. Went through it again and it looks great. Let's get this merged!

You might want to update the opening post, especially the changelog and migration guide since they get included in the release info.

Ixentus avatar Jan 16 '24 16:01 Ixentus

Thanks for the reminder. The PR description should be up-to-date now. While updating the migration guide I found an opportunity to make this slightly less breaking by taking a SpatialScale instead of DefaultSpatialScale in AudioPlugin.

I wonder if the From<SpatialScale> impl is still useful?

rparrett avatar Jan 16 '24 17:01 rparrett

Thanks for the reminder. The PR description should be up-to-date now. While updating the migration guide I found an opportunity to make this slightly less breaking by taking a SpatialScale instead of DefaultSpatialScale in AudioPlugin.

Very clean!

I wonder if the From<SpatialScale> impl is still useful?

Since users never need to construct a DefaultSpatialScale it doesn't seem useful anymore.

Ixentus avatar Jan 17 '24 18:01 Ixentus