Adding pitch support #2564
Adding pitch support #2564
Conversation
|
I'll work on this, it's just that it works when you play and change the pitch, but when you play it with the pitch already given, it doesn't for some reason? |
|
I think the failing on cpp is a false alarm, cos I have everything ok (testing on desktop at least), and if I check on the cpp builds, there seem to have an error somewhere in |
|
I also addressed the concerns that you have about the code I made, when you can tell me if you have any questions about what I replied or adding some suggestion of my code |
I don't think it's a false alarm, I'm seeing the unit tests failing at: Have you ran the unit tests on cpp? Have you tried using this feature for sounds in a soundGroup? try using this feature with |
|
I hope you don't mind re-re-requesting another review about this @Geokureli lmao |
| #else | ||
| @:privateAccess | ||
| if (_channel.__source != null) | ||
| _channel.__source.pitch = v; |
There was a problem hiding this comment.
getting errors here:
.../FlxSound.hx:752: characters 17-25 : flash.media.SoundChannel has no field __source
.../FlxSound.hx:753: characters 14-22 : flash.media.SoundChannel has no field __source
There must be a problem with the FLX_PITCH definition
flixel/system/macros/FlxDefines.hx
Outdated
| @@ -129,6 +131,9 @@ class FlxDefines | |||
| defineInversion(FLX_NO_FOCUS_LOST_SCREEN, FLX_FOCUS_LOST_SCREEN); | |||
| defineInversion(FLX_NO_DEBUG, FLX_DEBUG); | |||
| defineInversion(FLX_NO_POINT_POOL, FLX_POINT_POOL); | |||
| #if (openfl_legacy && sys || lime >= "8.0.0" && !flash) | |||
| defineInversion(FLX_NO_PITCH, FLX_PITCH); | |||
| #end | |||
There was a problem hiding this comment.
the #if shouldn't be here, FLX_PITCH and FLX_NO_PITCH will always be inversions of each other, you need to define FLX_PITCH here:
There was a problem hiding this comment.
why it shouldn't be there?
afaik, defineInversion is for stuff that has 2 sides, the yes and the no version, which, what defineInversions() does afaik.
defineHelperDefines() only adds defines depending if it can help the first thing
unless I'm overlooking things
There was a problem hiding this comment.
None of the other inversion are wrapped in #ifs, take a look at what
defineInversion() actually does. I believe this is the reason flash is failing
|
also do you have a little tiny project that uses this feature you could share? |
I have one little project that I used for testing back in the lime pr. |
Either that, or send me a relevant snippet, or zip it on discord |
Sure! here it is! |
|
and I believe this should wrap it up @Geokureli? |
|
something is odd about this, I feel I was wrong about the order here. FLX_NO_PITCH needs to be set to true on flash and openfl legacy before defineInversions is called. Right now it's still failing on flash |
I believe it was still failing on flash cos I forgot a semicolon when setting the FLX_NO_PITCH thing. |
|
oh no, it was failing on flash because I was testing with your demo, which should fail on flash, Im dumb |
Oh LMAO |
|
a/w I think that rn it's somewhat acceptable to merge, unless there's another change that should be made @Geokureli? |
|
I think it would be nice / accessible to have a demo, or at the very least a snippet that nicely demos the new pitch support, just suggestion tho !!! |
I don't mind if geokureli wants to use this as a pitch sample. |
|
thanks cheems, I'll probably use that as a starting point, if not, as is |
I know there's already a PR that's been merged in lime right here
but that build doesn't seem to be public yet, sooo I decided to make this support as local as possible (also that it may work on more than one version while using lime idk)