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

My FlxPreloader changes #987

Merged
merged 10 commits into from Apr 9, 2014
Merged

My FlxPreloader changes #987

merged 10 commits into from Apr 9, 2014

Conversation

SeiferTim
Copy link
Member

These changes seem to make the FlxPreloader work with Chrome again...

I was looking at the way NMEPreloader works (since it works on Chrome) vs the FlxPreloader, which started getting 'stuck' once my swf was > 5MB.

It looked like, at some point, someone was worried about the super.onLoaded() function being called more than once, and decided that the way to fix it was to override onLoaded to not do anything, and then call destroy once percent >= 1 and I think this seemed to work okay when the swf was small enough, but once the swf gets to a certain size this logic breaks and it was getting stuck - because EnterFrame still gets called more than once after the swf has finished loading...

Instead, I stopped overriding onLoaded, and put a flag (_loaded) that will stop EnterFrame's logic from happening after the onLoaded function is called for the first time...
In my project, if I revert to the current version of FlxPreloader, it gets stuck every time - and when tracing I can tell that EnterFrame is being called about 10 times after the swf has finished loading.
When I use my changes, it has never gotten stuck and only goes through the EnterFrame logic until it's loaded and then stops.

@gamedevsam
Copy link
Contributor

The FlxGamepad and FlxKeyboard changes seem good, but they shouldn't be part of this pull request. @Gama11 won't be happy about this.

@SeiferTim
Copy link
Member Author

Sorry... I'm still very new to the whole Git thing... I don't understand how to keep these things separate...

@gamedevsam
Copy link
Contributor

@SeiferTim before making a pull request, you should create a branch that is specific for that fix, so for example, for this issue you should create a branch called FlxPreloader Chrome Fix, so that way you can commit only the relevant change to that branch, and switch back to your dev branch where you can continue committing your work normally.

@SeiferTim
Copy link
Member Author

I tried that, but that still has all my FlxKeyboard changes in it....

override public function onUpdate(bytesLoaded:Int, bytesTotal:Int):Void
{
#if !(desktop || mobile)
//in case there is a problem with reading the bytesTotal (Gzipped swf)
if (root.loaderInfo.bytesTotal == 0)
{
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird white space change here.

@gamedevsam
Copy link
Contributor

I'm OK merging your gamepad / keyboard changes. They're good changes.

@gamedevsam
Copy link
Contributor

Looks good 👍

gamedevsam added a commit that referenced this pull request Apr 9, 2014
My `FlxPreloader` changes
@gamedevsam gamedevsam merged commit de925de into HaxeFlixel:dev Apr 9, 2014
@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

You broke the preloader! 0:
also, @SeiferTim, just wondering, did you encounter the "stuckness" while on the git branches of both OpenFL and Flixel?

@SeiferTim
Copy link
Member Author

@sruloart how is it broken now?
I'm only using dev branches of Flixel, Flixel-addons, and Flixel-ui....

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

Edit: Good to know! I was just wondering how did you manage to use this preloader, as it's not functional without this (it will just jump to the game):
override public function onLoaded():Void
{
// Do nothing here
}

@Gama11
Copy link
Member

Gama11 commented Apr 9, 2014

Could you please explain your gamepad changes? Why do we need to check if the button exists?

Also, I'm not happy with the implementation of the firstPressed() functions. They should not return a FlxKey, but the string for the key. That's way more convenient - all you ever need the key for is the name anyway, and that way you won't have to guard against null.

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

Someone could be eating his controller, you never know :)

@SeiferTim
Copy link
Member Author

@Gama11 I don't remember the details now only that I was getting errors in cpp about null values until I added that in there.

@sruloart This current code is working perfectly for me.... it seems to be that overriding onLoaded to not do anything, instead of doing what NMEPreloader has it doing, was causing it to get stuck. I'm not really following your issue... I'm not sure what you're seeing...

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

It's not my issue, I think you don't use the git branch of OpenFL...Please tell me that I'm right...

@Gama11
Copy link
Member

Gama11 commented Apr 9, 2014

Just looked over it again. The gamepad changes make sense, and I adjusted the keyboard functions a bit 8dd5509.

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

If you do use OpenFL git branch, please try to run a swf < 5MB...

@SeiferTim
Copy link
Member Author

@sruloart I'm not saying that there isn't a problem, but I don't know if it's ideal to gear changes in HaxeFlixel on dev branches of other things like OpenFL - it might be a bug in that version that will be fixed or will never be released, etc, or this dev version of HaxeFlixel could get released as 'live' long before OpenFL's dev version does... and I think it's more likely that someone would be using all the 'currently released' versions of libraries and no dev versions...
I'm not sure if I'm making any sense here lol

The other issue was that I hadn't really expected it to get merged so quickly, I made the pull request in an effort to get some other eyes on the issue and some more feedback - like I said, it fixed the issue for me on more than one project (several demo projects which are really, really small, as well as my big 8Mb beast of a project)...

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

@SeiferTim I understand everything, this is why I was asking you about the branch. I'm all over the damn preloader for months now.

You see, the last time the preloader got stuck (the second or third time) I contacted Joshua directly (via Twitter) and he made a new ApplicationMain! This is not some dev feature for OpenFL, it just wasn't big enough to re-release OpenFL (I think it's 1.3 now) because of it.

The new ApplicationMain get stuck because of your change. Now, you may be totally right about the all "stuckness" process and you'll be forever glorified on the Halls of Preloadment, but this requires some testing, and a possible adjustment to the FlxPreloader and/or the new ApplicationMain (This may affect many other users on many other platforms, as I (sadly) didn't test the new ApplicationMain with big swfs).

I think that releases should work with other releases, and dev branches should work with other dev branches. This way the problems get located and solved before they get released. Plus, This way you can quickly add and play with new external branches' new dev features...

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

@SeiferTim Wait! I spoke too soon, OpenFL 1.3.0 is updated with the latest changes (as the git branch), So can I assume you have even earlier version of OpenFL? Maybe 1.2.3? I really need to know before starting testing...

@SeiferTim
Copy link
Member Author

I am definitely using 1.3.0

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

BRRR...more testing is required, brain is getting more and more agitated (-:
I'll get back to you when I can understand what's going on...it's definitely not "everything"...

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

@SeiferTim OpenFL 1.3.0 + Flixel dev = OpenFL git + Flixel dev = The preloader moves directly to the game on the second frame without actually reloading (Adobe Scout says so).

I have a problem. And let me tell you, I rarely do this to solve any of my problems, but let us, for this one time and only, use LOGIC:

  1. Something with my testing is wrong.
  2. Something with your testing is wrong.

Now, this is easily settled: @Gama11 / @gamedevsam - would you mind checking if you see the preloader loading (on a clean, fresh flixel project) with the Flash standalone Player / dragging the swf into a clean browser window? (tim, would you mind doing that as well? I know your server doesn't Gzip so it's very important to know how you test this).

In order to prove me right or wrong, please also Change the "minDisplayTime" on the FlxPreloader to, let's say, 10. Otherwise it will be too fast to notice...(and if this change doesn't do anything at all it proves my point).

@SeiferTim
Copy link
Member Author

So, I don't really care who fixes it or how or if I get any 'glory' for fixing it or not - I needed to have a working build released, I saw a problem, and I found a solution that worked for me and was able to release. I guessed that my solution might be THE solution and shared it in a Pull Request to get more discussion going so that a viable, permanent solution can be found.

I think we can all agree that it SHOULD be fixed ASAP so people can use HaxeFlixel.
I also think - at some point - it should be easier for a user to be able to access and use FlxPreloader - it's not ideal to have to change the code inside FlxPreloader to be able to do things like set the minTime or set URL Locking, etc.

Okay, so back to the problem at hand:
I made a new Flixel Template Project. I added a flxText to my Menu (just to verify that it's getting there), and I changed my SWF_VERSION tag to 12.0 (to prevent the FlxGamepad warning from coming up).

Using my FlxPreloader it's now jumping straight past the loading screen.
This was not happening before... but it seems to be an issue if it takes less time to load than min time would allow....

I added some traces:
the INSTANT the game starts (the very first time onEnterFrame is called), it tells me that it's at 1 percent loaded, time = 115 (!?), but, even though it SHOULDN'T be calling onLoaded, it still is anyway... anytime the game is actually loaded, regardless of what minTime is...

SO, something is causing onLoaded to be called whether or not time > minTime the instant that the project is loaded completely, and that gave me an idea on how to fix it.

This is my new attempt, and this (so far) fixes all the scenarios I can think of: Small filesize and minDisplayTime > time it takes to actually load, small filesize and smaller minDisplayTime, and Huge filesize.

PLEASE take a look, give it a try and give me some feedback and we can see if this truly a fix to turn into a pull request... THANKS.

https://github.com/SeiferTim/HaxeFlixel/blob/473ac7690db087a04fb043e204ea475ceea795cd/flixel/system/FlxPreloader.hx

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

So, I like this a lot, but I do have two comments:

  • Keep minDisplayTime>= 1, not lower, otherwise the preloader will get stuck, it says so on the comment :) (seariously, I've doubled check this right now). Keep the comment.
  • Unlike the others platforms, Chrome will get stuck if you don't listen to the comment and place a value of bytesTotal > Actual file size in bytes. We don't have anything to do here but alerting the user. The comment needs to be a bit stronger, be creative, use lots of !!!.
    • I thought about adding a macro that get the actual file size for the users, but then I thought, nah, this is complicated, let's just write 50K~ and let it be.

The Halls of Eternal Preloadment awaits your pull request! oh wait, that means they're stuck, right?

@SeiferTim
Copy link
Member Author

@sruloart for each of these, they should be more easily accessible outside of changing FlxPreloader.hx - I was told yesterday that FlxPreloader is not really meant to be extended... so there should be something that a user can do to interact with those values...

@sruloart
Copy link
Contributor

sruloart commented Apr 9, 2014

I agree entirely, did you have any specific idea?
This inspires me a little: https://github.com/benlowry/openfl-fglads-preloader

@SeiferTim
Copy link
Member Author

No, I don't have any ideas... hopefully @Gama11 or @gamedevsam or someone might?

@Gama11
Copy link
Member

Gama11 commented Apr 10, 2014

Pulic static variables for FlxPreloader?

@SeiferTim
Copy link
Member Author

Isn't it loaded before you have any access to that stuff in your code?

@Gama11
Copy link
Member

Gama11 commented Apr 10, 2014

I guess... :/

@SeiferTim
Copy link
Member Author

Could there be a way to put values in project.xml that the preloader will use?

@Gama11
Copy link
Member

Gama11 commented Apr 10, 2014

Not easily I think... You'd need a macro to parse the xml.

@sruloart
Copy link
Contributor

I think Reg should work...All the classes are loaded on the same frame as the preloader, so it's there...

@Gama11
Copy link
Member

Gama11 commented Apr 10, 2014

What do you mean? If it's the template Reg.hx - we can't have a dependency in the core preloader for a class that is totally optional.

@sruloart
Copy link
Contributor

Can't we have a core class that stores special values for the core classes for easy access?

@Gama11
Copy link
Member

Gama11 commented Apr 10, 2014

For which core classes? The only use case is FlxPreloader as far as I can tell.

@sruloart
Copy link
Contributor

This is a very good point, but surely there must be some added functionality to this somewhere? it at least sounds useful...

@Gama11
Copy link
Member

Gama11 commented Apr 10, 2014

This is simply not possible by adding a class - at least it would be no different from the "go-into-FlxPreloader-and-change-stuff"-approach.

Either you add a FlxPreloaderVars.hx or whatever it's called to the engine itself making sure every game has one. But then it still requires changing engine files. Or you require such a file to exist on the user side somwhere - which is a very, very bad idea.

@sruloart
Copy link
Contributor

Agreed [Added to my tothinkabout list...]...

@SeiferTim
Copy link
Member Author

We could make it 'easier' to extend FlxPreloader, including a lot of documentation about the things that the user may want to change (URL Locking settings, Min Time, etc) vs things they should probably leave alone...

@sruloart
Copy link
Contributor

👍

@Gama11
Copy link
Member

Gama11 commented Apr 10, 2014

Could always override the constructor in a FlxPreloaderExt, use that in the xml and and set the values there?

@sruloart
Copy link
Contributor

Great idea, this gives me another idea: Move the non-core functions of the preloader to FlxPreloaderExt, thus make extending the preloader much easier (and maybe make a specific preloader class for HaxeFlixel? The logo creation process is not easy on the eyes).

@sruloart
Copy link
Contributor

@SeiferTim can you pull request (remember minDisplayTime = 1) and decide later? The preloader is broken anyhow...

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.

None yet

4 participants