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

Refactor and clean up FlxSound #92

Merged
merged 5 commits into from
Sep 20, 2012

Conversation

IQAndreas
Copy link
Member

The point was to make FlxSound more clear, and to avoid repetitious code (which if I may say so myself, was a great success). See the commits for full changelog.

Several spots used 4 spaces rather than a tab to indent lines of code.
This has been fixed in FlxSound (TODO: Check elsewhere for same occurrence)
Removed repetitious code for stopping and cleaning up the old channel,
and moved it all to a new function `cleanup()`.
According to [the documentation](http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/media/SoundChannel.html#position):

> If the sound is looped, position is reset to 0 at the beginning of each loop.

So I removed a block of code that was simply unecessary.
- Removed oodles of duplicate code.
- Changed the way pausing sounds works.
Seriously, Adam? Naming variables the same name as classes is a big no-no!
@IQAndreas
Copy link
Member Author

NOTE! I don't have any project to test this on, so it still needs to be tested.

FDT allowed me to compile it, so there weren't any syntax errors, but I'm not sure if any functionality has stopped. Though, it should work in theory.

@Dovyski
Copy link
Member

Dovyski commented Sep 18, 2012

I've tested your code and found some problems:

  • After fadeOut() finishes its processing, the sound stops playing and cannot be resumed (even if PauseInstead == true). It is not possible to increase/decrease the volume using the volume property and getActualVolume() always return 0. The only way to make it sound again is calling play().
  • If the sound is paused, you resume it and less than a second before you resume it again, the sound plays twice (one for each resume). It works for more than two quickly resumes (three, four, etc).
  • If you pause a sound twice, it stops playing and cannot be resumed anymore;
  • proximity() seems to be inverted (the closer you are, the lower is the volume). There is already an open issue related to that problem (FlxSound.proximity is broken. #4).

I've tested all those problems using Adam's branch and I got the same results. Those bugs were not introduced by your refactoring.

Since the objective of this pull request is to refactor FlxSound, I think it was achieved and it's good to be merged. We should fix those bugs using a new issue for each one or a single issue for them all.

@IQAndreas
Copy link
Member Author

I did not take fadeOut() into consideration when making these changes. I believe I know why they are happening and how to fix them. Let me address those issues while I'm mucking around in FlxSprite anyway (I'll add another commit to this pull request tonight, or I can add it to a separate one if you prefer).

If you pause a sound twice, it stops playing and cannot be resumed anymore;

I actually noticed that bug in Adam's code, but this version should have fixed that. I don't see how pausing twice can affect anything in this version of the code.

If the sound is paused, you resume it and less than a second before you resume it again, the sound plays twice (one for each resume).

I'm unclear of how this could be happening. Do you think you could share the test project you are using so I can use it to debug this code?

@Dovyski
Copy link
Member

Dovyski commented Sep 18, 2012

Yes, I can share it. As soon as I have access to the computer where I tested it, I will upload the code to a temp repo.

@Dovyski
Copy link
Member

Dovyski commented Sep 18, 2012

Here it is, @IQAndreas https://github.com/FlixelCommunity/FlixelSandbox . Knock yourself out :)

moly added a commit that referenced this pull request Sep 20, 2012
Refactor and clean up FlxSound
@moly moly merged commit a558c44 into FlixelCommunity:dev Sep 20, 2012
@IQAndreas
Copy link
Member Author

If you pause a sound twice, it stops playing and cannot be resumed anymore;

I tested this one, and it seems to work just fine in the current dev branch (v2.55-50-g6486760, which includes this pull request as well as the "proximity" issues). Perhaps in your case, Flash Develop hadn't realize that you had switched branches and were using a different version of the Flixel library (does it keep a cache to reduce compile times?)

After fadeOut() finishes its processing, the sound stops playing and cannot be resumed (even if PauseInstead == true). It is not possible to increase/decrease the volume using the volume property and getActualVolume() always return 0. The only way to make it sound again is calling play().

This actually has to do with the above issue and has also been fixed in the current version. Using resume() after having faded out will now work just fine.

getActualVolume() still sometimes returns 0 if the sound is not playing (since it is updated inside of update() which doesn't run unless the sound is playing), but once it starts up again the volume resets itself. I'll add some code to fix this.

If the sound is paused, you resume it and less than a second before you resume it again, the sound plays twice (one for each resume). It works for more than two quickly resumes (three, four, etc).

I haven't been able to reproduce this one either.

@IQAndreas
Copy link
Member Author

I updated the Flixel Sandbox with the latest SWC if you want to test these yourself.
https://github.com/FlixelCommunity/FlixelSandbox

NOTE: The SWC matches the current version of dev (v2.55-50-g6486760), which means it does not include the pull request I just submitted that resets "fade" problems.

@Dovyski
Copy link
Member

Dovyski commented Sep 26, 2012

I've tested this pull request using the last version of Flixel Sandbox and it is working fine. All bugs I reported were gone. Maybe it was some cache problem related to FlashDevelop.

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.

3 participants