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

Added linear gain filter #499

Merged
merged 44 commits into from
Aug 9, 2024
Merged

Added linear gain filter #499

merged 44 commits into from
Aug 9, 2024

Conversation

iluvcapra
Copy link
Contributor

Inspired by a comment in #217 I've added a generalized linear gain ramp filter that accepts a beginning and end gain factor and duration.

Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

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

If I understand this is a more general version of #217. That is clearly something Rodio needs, nicely done! I've got some small suggestions here and there.

The main two main issues for me are:

  • The behavior after the filter is done
  • The name

src/source/mod.rs Show resolved Hide resolved
src/source/linear_ramp.rs Show resolved Hide resolved
src/source/linear_ramp.rs Outdated Show resolved Hide resolved
src/source/linear_ramp.rs Outdated Show resolved Hide resolved
src/source/linear_ramp.rs Outdated Show resolved Hide resolved
src/source/linear_ramp.rs Outdated Show resolved Hide resolved
@iluvcapra
Copy link
Contributor Author

Hey I'm sorry I've missed these replies, I've been swamped irl but let me take a look in the next week or two.

@dvdsk
Copy link
Collaborator

dvdsk commented Jul 12, 2024

Hey I'm sorry I've missed these replies, I've been swamped irl but let me take a look in the next week or two.

no worries, its also fine if it takes another few months. Take your time, I do that too.

@iluvcapra
Copy link
Contributor Author

I think this is ready to go.

Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

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

looking good! just a few nitpicks here and there and we can get this merged!

src/source/linear_ramp.rs Outdated Show resolved Hide resolved
src/source/linear_ramp.rs Outdated Show resolved Hide resolved
src/source/linear_ramp.rs Outdated Show resolved Hide resolved
src/source/linear_ramp.rs Show resolved Hide resolved
src/source/linear_ramp.rs Outdated Show resolved Hide resolved
@iluvcapra
Copy link
Contributor Author

I've done the review comments, one note: I've added a dependency on the "assert_float_eq" package to allow soft comparisons of floats in the case float rounding causes results to be off within the system epsilon.

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 4, 2024

love the new tests btw, very readable, very clear!

and not the current position.
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

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

Good idea to add an example!

It might however be best to keep this one simple. Lots of user have trouble understanding how to use Rodio. Mixing multiple concepts (the new linear gain ramp and the mixing controller) could confuse them.

Maybe split off the linear gain ramp to a separate example?

@iluvcapra
Copy link
Contributor Author

How about I just make a fadeout source?

I'd like to do an example of a linear gain ramp but I was going to wait until this was accepted and then just write an ADSR envelope source and build a new example around that.

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 5, 2024

I think we had a slight miscommunication. The example examples/mix_multiple_sources.rs changed. It was relatively simple but now contains an iterator, for_each and a queue with tx and rx. To you those are known and simple, but that is not going to be true for everyone.

It is useful to have more difficult examples in there but an example called mix_multiple_sources should in my opinion only do one thing (mix multiple sources).

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 5, 2024

fadeout source

good addition! we got fadein so why not fade out 👍

It seems implementation is trivial, and FadeIn and FadeOut are more specific and thus more readable for when someone only wants a fadein or fadeout.

@iluvcapra
Copy link
Contributor Author

I see, I'll revert the example.

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 5, 2024

oh and you have the honor of editing the changelog 🎉 since these are two nice new api's!

(you can add them under unreleased)

Copy link
Contributor Author

@iluvcapra iluvcapra left a comment

Choose a reason for hiding this comment

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

All looks good to me.

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 9, 2024

Lets get this merged!

@dvdsk dvdsk merged commit 3c18b53 into RustAudio:master Aug 9, 2024
11 checks passed
@iluvcapra
Copy link
Contributor Author

Thanks I'll try to do it in fewer commits next time!

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 9, 2024

Thanks I'll try to do it in fewer commits next time!

no worries! again thanks for the contribution!

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.

2 participants