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

FlxSound: add endTime, closes #1906 #1943

Merged
merged 10 commits into from
Sep 11, 2016
Merged

FlxSound: add endTime, closes #1906 #1943

merged 10 commits into from
Sep 11, 2016

Conversation

Beeblerox
Copy link
Member

See issue #1906

private function set_loopTime(value:Float):Float
{
loopTime = value;
if (_sound != null)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we use the length property to avoid the null checks?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, of course. i've missed this new property)

Copy link
Member

Choose a reason for hiding this comment

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

You forgot to remove the null checks. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. removed them

@Gama11 Gama11 added this to the 4.2.0 milestone Sep 10, 2016

private function set_loopEndTime(value:Float):Float
{
return loopEndTime = (value <= loopTime) ? length : value;
Copy link
Member

Choose a reason for hiding this comment

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

So, you prevent invalid loopEndTime values here... However, what these values are depends on loopTime. What happens if loopTime is set after loopEndTime?

@Gama11
Copy link
Member

Gama11 commented Sep 11, 2016

I'm not sure how much is gained by having setters for loopTime and endTime. As my last two comments show, it's getting quite complex...

Maybe it's better to just trust the user to supply correct values? That way there are no bugs caused by Flixel / bounding the values at least.

@Beeblerox
Copy link
Member Author

yeah, i think it would be better.
sorry for such messed pull request for such simple feature

@Gama11
Copy link
Member

Gama11 commented Sep 11, 2016

It's really not as simple as it looks, there's a lot of things to take into account. :)

*/
public function play(ForceRestart:Bool = false, StartTime:Float = 0.0):FlxSound
public function play(ForceRestart:Bool = false, StartTime:Float = 0.0, EndTime:Float = 0.0):FlxSound
Copy link
Member

Choose a reason for hiding this comment

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

EndTime == 0 here used to mean "set it to length", but now the behavior of set_endTime() is different.

Maybe it should be ?EndTime:Float, and if (EndTime == null), we set it to length?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@Gama11 Gama11 changed the title Allow setting sound playback/looping end point FlxSound: add endTime Sep 11, 2016
@Gama11
Copy link
Member

Gama11 commented Sep 11, 2016

Looks good now, thanks! 👍

@Gama11 Gama11 changed the title FlxSound: add endTime FlxSound: add endTime, closes #1906 Sep 11, 2016
@Gama11 Gama11 merged commit f8b43f6 into dev Sep 11, 2016
@Gama11 Gama11 deleted the Beeblerox-patch-1 branch September 11, 2016 10:07
Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this pull request Apr 18, 2018
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