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

Audio pitch change implementation #1510

Merged
merged 11 commits into from
May 12, 2022
Merged

Conversation

ninjamuffin99
Copy link
Contributor

@ninjamuffin99 ninjamuffin99 commented Dec 18, 2021

Finally got hands dirty and figured out some of the backend stuff to get pitch changing stuff available.

I've tested Windows and HTML5, I'm just assuming most of the NativeBackend stuff works in a generally similar way.
NOT implemented Flash, I'm assuming it'd give you errors as getPitch() and setPitch() and the FlashAudioBackend aren't implemented.

I believe this closes #871

@Cheemsandfriends
Copy link

This seems to crash an hl build, or at least my results with hl were that.
Idk if it crashes more targets but yeah.

@Cheemsandfriends
Copy link

The two options go sweet in terms of pitch changing.
What it gives me a nerve is the other one, the native backend audio.
It seems that it works based and depending on a timer. Maybe there should be a new way to make the timer running better.

@player-03
Copy link
Contributor

player-03 commented Dec 29, 2021

Hope you don't mind, but I copied @Cheemsandfriends's work over from #1512. (With a few changes.)

Looks like it fixed the CI error.

@Cheemsandfriends
Copy link

I'm kinda flattered that you used some of my work even tho it was some bad code lol

@ninjamuffin99
Copy link
Contributor Author

ninjamuffin99 commented Dec 30, 2021

nice work, CI seems to be fixed, was there anything else that may need to be implemented for this? Was HL crashing?
Would it be worth to do a little bit of documentation as well? Just some standard code block comments I guess. Feel like it might be worth to put SOMEWHERE that Flash pitch changing isn't really supported

@ninjamuffin99 ninjamuffin99 marked this pull request as ready for review December 30, 2021 05:13
@player-03
Copy link
Contributor

Yeah, comments would be nice. You can refer to Application.hx for formatting examples. (Yeah the parameter descriptions don't all line up, but I guess that's just how we do things here.)

Other than that, I guess it's a matter of testing. Cheemsandfriends, does HL still crash for you?

@Cheemsandfriends
Copy link

nice work, CI seems to be fixed, was there anything else that may need to be implemented for this? Was HL crashing? Would it be worth to do a little bit of documentation as well? Just some standard code block comments I guess. Feel like it might be worth to put SOMEWHERE that Flash pitch changing isn't really supported

It isn't really supported but, you wouldn't trace UNSUPPORTED right?

And I don't know pretty much anything about Flash, tried to reproduce sounds but I couldn't hear them so I just gave up

@Cheemsandfriends
Copy link

Yeah, comments would be nice. You can refer to Application.hx for formatting examples. (Yeah the parameter descriptions don't all line up, but I guess that's just how we do things here.)

Other than that, I guess it's a matter of testing. Cheemsandfriends, does HL still crash for you?

I should check up the testing right now and tell if it correctly works or not wait a sec

@Cheemsandfriends
Copy link

Cheemsandfriends commented Dec 30, 2021

Ok, I tested what @player-03 made and have some opinions about it.

  1. It seems that in Flx, If you use it on Flixel, the timer will go nuts, so better use the pitch variable to only if the pitch is changed?
  2. Seems a nice job, even though it was removed some functions that make the functions work good like setLength or setCurrentTime

@Cheemsandfriends
Copy link

Cheemsandfriends commented Dec 30, 2021

I would want to make a commit here but idk if it's really possible lol

(Nvm, Just made a review comment on the pitch lol)

@player-03
Copy link
Contributor

Oh, oops, I never got around to answering these questions. I think the correct way to add changes to this pull request is first to check out the branch. It'd look something like this:

> git remote add ninjamuffin99 https://github.com/ninjamuffin99/lime.git
> git fetch ninjamuffin99
> git switch -c pitch ninjamuffin99/pitch

That'll get you set up to make changes locally. You can push these changes to your own repo, at which point I think you can use this url to start a pull request.

It seems that in Flx, If you use it on Flixel, the timer will go nuts, so better use the pitch variable to only if the pitch is changed?

It should be possible to make this work, but it might be that the formula is off. I'm afraid I haven't yet tested it myself, as awful as that sounds. Just kind of reused your formula and hoped everything would work out.

Seems a nice job, even though it was removed some functions that make the functions work good like setLength or setCurrentTime

But... those are still there. Or are you saying they no longer work?

@Cheemsandfriends
Copy link

Cheemsandfriends commented Jan 7, 2022

It should be possible to make this work, but it might be that the formula is off. I'm afraid I haven't yet tested it myself, as awful as that sounds. Just kind of reused your formula and hoped everything would work out.

It might be, I don't pretty know about coding since I just really started just a few months but the formula I made was like a play-and-test thing with my programming acknowledges. So if it is very off, i'm sorry lol

But... those are still there. Or are you saying they no longer work?

You removed the value option to getLength() That's why I said that it no longer works as intended lol
Either way I added the review comments on the things you made so you can check it out lol

@player-03
Copy link
Contributor

Either way I added the review comments on the things you made so you can check it out lol

I still don't see those. Link?

@Cheemsandfriends
Copy link

Cheemsandfriends commented Jan 7, 2022

I still don't see those. Link?

In the commits you made
If you can't see those, then I can post some photos to the reviews to check it out

@player-03
Copy link
Contributor

Odd. I have "show comments" checked, and I still can't see them.

@Cheemsandfriends
Copy link

Cheemsandfriends commented Jan 7, 2022

Odd. I have "show comments" checked, and I still can't see them.

Weird then I should post the images with the review then.
This is the review post where you changed the setCurrentTime Thing:

imagen
This is the review post where it was about the setLength thing

imagen

And this is the setPitch thing to make the setPitch something nice and not fucky when you set it when you're setting it on each frame

imagen

Idk why you can't see the review comments tho, It's a bug or something like that?

@player-03
Copy link
Contributor

player-03 commented Jan 7, 2022

Ah, I see your points.

Interesting that your reviews are labeled "pending." That's probably why they aren't publicly visible. I didn't see a button to approve them, so perhaps only ninjamuffin99 can (being the owner of the fork).

@Cheemsandfriends
Copy link

I have a button to like approve the things, maybe I should approve it lol

Copy link

@Cheemsandfriends Cheemsandfriends left a comment

Choose a reason for hiding this comment

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

Good but needs some more robust code to not lose the track of the thing

src/lime/media/AudioSource.hx Show resolved Hide resolved
src/lime/media/AudioSource.hx Show resolved Hide resolved
src/lime/media/AudioSource.hx Outdated Show resolved Hide resolved
src/lime/_internal/backend/native/NativeAudioSource.hx Outdated Show resolved Hide resolved
src/lime/_internal/backend/native/NativeAudioSource.hx Outdated Show resolved Hide resolved
src/lime/_internal/backend/native/NativeAudioSource.hx Outdated Show resolved Hide resolved
@Cheemsandfriends
Copy link

Oh heck, it was many review post XD

These setters now: (1) start by checking if anything changed, (2) use the new value in calculations, and (3) perform the assignment only at the end.
@Cheemsandfriends
Copy link

Seems good to me and to the CI

Using `getGain()` and `setGain()` as a template. Like gain, you won't be able to set pitch before calling `init()`.
@Cheemsandfriends
Copy link

Cheemsandfriends commented Jan 8, 2022

About the last commit, I have to check if it's correct or not cos maybe it was simply the bad positioning of the pitch thing
If I have results, I'll post it here.

@player-03
Copy link
Contributor

CI seems to be failing on the download step. @joshtynjala, how did you fix this one?

@joshtynjala
Copy link
Member

@player-03 I clicked the "Re-run all jobs" button in the summary page for the actions run.

It's probably just a temporary network issue today.

@Cheemsandfriends
Copy link

make a random commit, like add a space and remove it or some shit like that.
should restart the thing idk

@player-03
Copy link
Contributor

No commit required; there's a re-run button. I was just giving it time in hopes the network would fix itself.

@Cheemsandfriends
Copy link

No commit required; there's a re-run button. I was just giving it time in hopes the network would fix itself.

Oh, nvm.
I just don't know how github works

@Geokureli
Copy link
Contributor

What is the status of this?

@Cheemsandfriends
Copy link

What is the status of this?

tbh, Idk.
I'm kinda curious too tho

@player-03
Copy link
Contributor

I don't like to be the one to merge my own changes - what if I missed something? So I leave it in hopes someone else will check it, though it seems this is what happens instead.

Well, since it's been approved for four months and no one spotted any issues, let's go for it.

@player-03 player-03 merged commit e6674ba into openfl:develop May 12, 2022
@SomeGuyWhoLovesCoding
Copy link

Yooo ninjamuffin99 the creator of fnf 😱

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.

Add support for pitch shifting in AudioSource
6 participants