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

fix can't change volume after it fadein #34

Merged
merged 2 commits into from
Feb 17, 2017
Merged

fix can't change volume after it fadein #34

merged 2 commits into from
Feb 17, 2017

Conversation

krmbn0576
Copy link
Collaborator

@krmbn0576 krmbn0576 commented Feb 14, 2017

RMMV's WebAudio contains several bugs.

  • Can't change BGM volume after playing ME
  • Can't change BGM volume after replaying BGM
  • Can't change BGM and BGS volume after battle end

The cause of all these bugs is "Can't change volume after it fadein".

This is caused by the strange specification of WebAudio AudioParam. (For details, see WebAudio/web-audio-api#128)
Changes to value are ignored after setValueAtTime or linearRampToValueAtTime is called.
Therefore, I decided to cancel these schedules when changing value.

@ghost
Copy link

ghost commented Feb 14, 2017

It works correctly?

Anxious comment is found : WebAudio/web-audio-api#128 (comment)

Maybe replace setting values directly to calling setValueAtTime() if it works.

@krmbn0576
Copy link
Collaborator Author

Hmm, In my Environment, it works correctly.

I think he/she misunderstood. setValueAtTime() is scheduling method, so value won't work. Only cancelScheduledValues() can cancel schedules and enable value.

@ghost
Copy link

ghost commented Feb 14, 2017

I read the spec. setting value means calling setValueAtTime() at context's currentTime.

You'd better write cancelScheduledValues() just before automation methods being called.
There are some points to insert it.

@krmbn0576 krmbn0576 added this to the 1.1 milestone Feb 14, 2017
@krmbn0576
Copy link
Collaborator Author

Hm, is it profitable?
I think this commit works correctly now.

@ghost
Copy link

ghost commented Feb 15, 2017

Same problem is found some mehtods. :-<

ex. WebAudio._fadeIn
It's rare to be called. But when called, it causes same problem, so You'd better fix it.

@krmbn0576
Copy link
Collaborator Author

Hmm, it never causes the problem because _masterGainNode.gain.value is assigned only when created.
Do you think that any method should not cause bugs under any circumstances?
I think, not necessary.

@ghost
Copy link

ghost commented Feb 15, 2017

Confusing, I tested current branch. (not this branch)
By the specification, value property and setValueAtTime is exactly same.
I can't understand why masterGainNode works and others not.

I found that volume works after using fadeIn. (Chrome 56.0.2924.87 Windows 10)
Is the bug from specific browser?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I cannot reproduce the problem, but LGTM if it works. 🍣

@krmbn0576
Copy link
Collaborator Author

krmbn0576 commented Feb 15, 2017

I found that volume works after using fadeIn. (Chrome 56.0.2924.87 Windows 10)

Me too! However, the bug occurs in Firefox and Nwjs...

Hmm, AudioParam#value is very confusing.
I replaced them to AudioParam#setValueAtTime at context's currentTime.

But... oh no the situation got worse!
this commit works correctly in Nwjs, but Firefox51 still have the same bug...

@ghost
Copy link

ghost commented Feb 15, 2017

Then, can you insert cancelScheduledValues() and test it by FF?

@krmbn0576
Copy link
Collaborator Author

I inserted this._gainNode.gain.cancelScheduledValues(0); at 270,
but FF still has the bug...

@ghost
Copy link

ghost commented Feb 15, 2017

I see...

Call cancelScheduledValues() then set value and setValueAtTime().
If these work, please comment these are workaround for FF.

@krmbn0576
Copy link
Collaborator Author

OK, I completely made a mistake.
The real problem is Firefox load old (before modified) scripts until I push F5 key explicitly!!!(including XMLHTTPRequest)
Chrome always load latest ones, so I made a very easy mistake. 😂

This commit works correctly!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

okay, LGTM 😇

@krmbn0576 krmbn0576 merged commit 905e8ae into master Feb 17, 2017
@krmbn0576 krmbn0576 deleted the fix_fadein branch February 17, 2017 07:49
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.

1 participant