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

Simpler default values for AudioStreamRandomizer #80171

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

RedMser
Copy link
Contributor

@RedMser RedMser commented Aug 2, 2023

AudioStreamRandomizer has two purposes:

  • select from a list of audio streams
  • randomize parameters like volume and pitch

Using this class for the first use-case means you will always have to reset the arbitrary default values of pitch and volume variation to 1 and 0 respectively.
And for the latter use-case, the defaults are chosen very arbitrary and will likely need to be adjusted anyway. So no time is saved by having values that aren't just 1 and 0 for them either.

@MewPurPur
Copy link
Contributor

MewPurPur commented Aug 2, 2023

I totally agree with this in theory, but idk if it should be merged.

– Compatibility-breaking
+ The compatibility break is small and it's not going to break break any projects, just change their audio a tiny bit
+ The new default is better
– The old default may be suboptimal, but it's fairly reasonable.

@RedMser
Copy link
Contributor Author

RedMser commented Aug 2, 2023

– Compatibility-breaking

I assumed unchanged default values of resources are still saved into the .res files. Is this not the case?
Even the extension API validator does not complain about this change.

@YuriSizov
Copy link
Contributor

I assumed unchanged default values of resources are still saved into the .res files. Is this not the case?

Default values are not saved, no. That would bloat resources quite a lot, for every inherited type that they have.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 3, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Makes sense to me, those values were indeed quite arbitrary.

Needs to be documented clearly in the "Changed" section of the changelog.

@akien-mga akien-mga merged commit 12a9ed0 into godotengine:master Aug 17, 2023
14 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

5 participants