Cleanup part1

[simonvizzini]

I took the liberty and did some bare minimum cleanup to get the separation started. I did what I proposed in #1, and so I extracted all JS to it's own directory, added webpack and all dependencies as npm modules. I also had to convert most things to ES classes, because the this scope was totally messed up, it never pointed to the correct object somehow and so this took me way longer than expected.

To get started:

cd src/FactorioBlueprintRenderer
npm install
npm run build

This will produce a fbpviewer.bundle.js which is copied to src/AppBundle/Resources/public/js (I have not checked in the bundle for now, you need to build it)

Everything works mostly, except that I had to disable clampZoom and clampPosition in handleKeyboardPanning, because it was constantly throwing exceptions, and I couldn't figure out what is wrong. Also, the JSON info dialogs don't open anymore, not sure what's wrong here, no JS error is thrown either. I haven't investigated this further yet.

npm run build produces a debug bundle including source maps. npm run build:prod will build a minified production bundle (but currently also includes sourcemaps, so it's rather big. I'll change that tomorrow). npm run watch will start webpack in watch mode, this will re-build the bundle on every file change.

The initial npm install and build should be somehow done automatically before docker-compose I guess, but I wasn't sure how to do it.

Let me know what you think and if you want me to continue. From this point on I'd continue as I suggested in #1, which means conversion to TS and then decoupling of the renderer. Conversion to TS should be a piece of cake now, since almost everything is classes already, just a a bunch of types need to be added.

[demipixel]

Have you tested atlasgen (located in /bin/fbpvatlasgen)? The spritesheet generator uses eval() on some of the js files, so its target location needs to be converted to the new JS location and use require() instead.

Anyway, although not strictly necessary, this PR would make it much easier to move the JS to its own library (and update this repo to use the library once the library is up and running). Fantastic work.

[simonvizzini]

Ah no, haven't tested the atlasgen yet, will look into it. Although I'm not sure if I should continue, @trakos unfortunately hasn't shown up yet and so I don't know what his plans and vision are. I'd have the whole day today to help out with this (not so much this weekend), and it looks like I'll move on to my other projects for today.

[simonvizzini]

@demipixel, I adapted the atlasgen, haven't tested but I think it should work fine. No eval necessary anymore.

[demipixel]

haven't tested but I think it should work fine

BAHAHAHAHA famous last words.

[simonvizzini]

If you have the chance to apply this pull request locally please do and let me know if it works :) I don't feel like setting up all the images right now.

Edit: of course I screwed up, walk is not needed anymore

[simonvizzini]

Of course it was not as simple as that... jquery was causing troubles, because it doesn't work outside a browser environment and so I replaced the $.each, $.extend and $.inArray utilities with lodash equivalents.

Unfortunately I still can't make the atlasgen work somehow. First the atlasgen.sh script with docker-compose didn't work for me. The npm install that is executed in docker was somehow missing the platform-command module (which is a dependency of spritesheet.js), running npm install outside docker worked. I also executed node atlasgen.js directly, without docker.

Next I tried to find all factorio images, but there is a reference to a "background.png" which is nowhere to be found in the factorio folder, so I used a white 1x1 pixel png. After that I had all the required images, and I was finally able to execute atlasgen.js. But then it errors out at the end:

.....
495/498
496/498
497/498
498/498
done
internal/child_process.js:317
    throw errnoException(err, 'spawn');
    ^

Error: spawn E2BIG

E2BIG apparently means argument list is too long. Would be nice if someone could verify if it works for them. Could be just my linux machine...

[trakos]

Thanks for your hard work! It definetely organizes code nicer.

Atlasgen works for me, though I had to remove package-lock because I had the error you mentioned.

Regarding the background, I won't commit factorio's images, and background is simply slightly edited concrete. You can use concrete for simple tests.

I think I had this E2BIG error once when I accidentally tried to generate spritesheet for twice as many images. We could consider splitting the textures in to few separate spritesheets later on to avoid it. It works for now though.

I've noticed a few issues:

• in ZoomAndPanHandler events vmousedown, vmouseup, vmouseout and vmousemove won't work without jquery-mobile (those are simply unified handlers for both mouse and touch events). Previously I've used build generated through http://jquerymobile.com/download-builder/ because full build destroys website (it adds some ajax loader that tries modifying html, requires css that messes navbar up etc). I think we have to restore the jquery.custom file, at least for now?
• because of that panning doesn't work for me, neither does clicking entities
• setting FBR_DEV to 1 should use normal images directly instead of spritesheet. It allows for quicker development when testing new entities, which might be needed if we consider adding some mods. Currently setting FBR_DEV to 1 causes error
• I'll check the errors with clampPosition and clampZoom later

Sorry for getting back to this so late, I wasn't around yesterday. I'll try to check those issues tomorrow, and we'll merge it as soon as we fix those.

As for the typescript, I don't mind it, I don't know about @demipixel?

[demipixel]

I'd prefer we implement typescript after it's been moved over to its own library.

[trakos]

Yeah, sure, priority should be moving to the different repo.

[simonvizzini]

Nice to hear back from you @trakos! And thanks for the docker clarifications. I'll check it again.

Regarding jquery.mobile: I wasn't sure where it's used. I added it as npm dependency but it's currently nowhere required so that's why panning and clicking is broken.

I didn't cleanup all the global config variables yet, so it's possible that FBR_DEV is not set correctly anymore. That's something I wanted to do in the next steps.

I wouldn't move to a separate repo just yet. Let me try to explain again:
All the JS is tightly coupled to the website. If we move to a different repo now we will have a harder time decoupling this, and we'll not have the website where we can verify our changes. Otherwise we'd have to setup yet another website for testing, but why bother right now if there is one already?

Additionally, I won't be able to help with the decoupling of the renderer if this is not TS. I've wasted countless years refactoring JS without TS, and I'll never do that again :) Refactoring a JS code base (especially if it's not written by yourself) is very hard and error prone, almost all errors show up during runtime only.

Yesterday I didn't do much, but I branched off of this pull request and started the TS conversion. Only file left is ui.js. Other than that everything is converted to TS, including proper types. The TS compiler already revealed some potential bugs that I fixed along the way. And now it will be almost impossible to break stuff.

If you want to see what it looks like, here is my TS branch: https://github.com/simonvizzini/fbpviewer/tree/refactor_part2

It won't compile at the moment, because I haven't setup webpack to compile TS yet, but you could use a TS enabled editor and browse around and try some of the TS features. Hovering over stuff you will see that almost everything has a type, so you'll always know what you're dealing with and the compiler will cry like a baby if you mess up :) And you will notice that the code is still pretty much the same as JS, just some type annotations here and there so the compiler knows what is what.

I need like 1 more hour to finish the TS conversion. After that it will be much simple to decouple the renderer from the website (this shouldn't bee too hard from what I've seen so far). Only after that it will become feasible to move the renderer to a new repo, in my opinion.

Unfortunately I'm out of home most of the day today, but I'll address all the points you mentioned plus finish the TS conversion when I get home later tonight.

[simonvizzini]

One last thing before I have to run: separating this into a new repo also has the highest priority for me. You'll just have to trust me that the way I'm doing this is the fastest and most hassle free way. My plan was to have this separated by tomorrow, so that other people can start to contribute as hassle free as possible. And TS will help with that, just believe me :) I don't know much about PIXI, Blueprints and Factorio in general (still new to Factorio), so I don't have the knowledge to turn this thing into an awesome Blueprint viewer/editor. I'm counting on other people to do that :D I'm just here to help everyone get started, and to make it as easy as possible to contribute (again, TS rocks in this regard!) Once I'm done with this I'll still be around to help with questions of course (I admit TS compile errors can be sometimes cryptic, but it's possible to get comfortable with it after a day or two, I'll help with that)

[trakos]

Well, if you're already almost done with the conversion, I suppose we can wait with the split until you're done. We wouldn't want you to have to redo the conversion in a new repo :)

[simonvizzini]

I finished the TS conversion. Everything works like before, except that I was unable to get jQuery mobile to work, so panning and clicking still doesn't work... problem is that jQuery mobile is only compatible with jQuery 1.8+ or 2+, but not 3+... and I didn't want to downgrade everything right now. This can be fixed later (personally I'd completely drop jQuery and all the plugins for something better, it's a mess, but that's not really relevant right now for the renderer separation)

FBR_DEV = 1 still doesn't quite work, I get this error:

Uncaught Error: The frameId "/images/factorio/entity/splitter/splitter-east.png.0" does not exist in the texture cache

I think it's just a minor thing missing somewhere, but I wasn't able to figure it out yet.

@trakos, how would you like to proceed? Should I merge my other TS branch with this pull request, so that it becomes one big pull request? Or do you want to merge this one first, and I'll create a 2nd pull request with the TS changes?

Also it would be good to start some form of real time communication soon, like slack, discord, irc or something, to discuss things further, once the initial cleanup is done, and to coordinate with other potential contributors. Has anyone any ideas and knows how to set it up?

[trakos]

Hey, thanks a lot for your hard work!

I've fixed the minor typo in loader (FBR_DEV related error), although I had a different error. Yours was probably caused by the lack of image, I'd guess.

I've downgraded jquery for now and restored custom jquery.mobile. I'm not going to accept merge requests if something's simply not working ;) Plus, if we're going to drop jquery altogether, its version doesn't matter. I care much more about things working than version numbers.

So, everything seems to be working now, I'm merging.

[trakos]

@simonvizzini For the part2 of the refactor, can you move it to https://github.com/trakos/fbpviewer-js ? I've decided that it was a good moment to split the code.

[trakos]

Hey, I've tried your refactor_part2 branch, but I had some js errors after the build.

I hate to say it, but I'm kinda lost in that typescript code. With how many changes the javascript still requires, I think I'd rather skip the conversion for now, and maybe do it once the code settles down and it's fully separated from my website. With me and demipixel both being more comfortable with regular js, I think it won't add much value now.

[simonvizzini]

Sorry to hear you had trouble with TS. I won't be able to help with further refactoring then, so good luck guys and I hope to see a full fledged blueprint editor by the end of next week ;)