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

Organize JS into new repository #1

Closed
demipixel opened this issue Jun 14, 2017 · 14 comments
Closed

Organize JS into new repository #1

demipixel opened this issue Jun 14, 2017 · 14 comments
Assignees

Comments

@demipixel
Copy link

Kinda self explanatory. Better for code organization, easier for people to use, prevents issues about the site being cluttered with issues about rendering.

I didn't want to do it myself because I figured you'd want it under your name and you'd know better what's worth copying over.

That being said, it might be more of a challenge then it seems because it looks like there's some general fbpviewer JS code mixed in with the renderer (as oppose to a renderer folder and then all other scripts accessing a renderer object).

@trakos
Copy link
Owner

trakos commented Jun 14, 2017

I'm also wondering what should happen when user clicks displayed entity, like a combinator. Right now it's displaying entity json in a simple bootstrap dialog. Maybe the js library should only send event with entity details?

Implementing some UI inside canvas for displaying the details feels like big overkill.

@demipixel
Copy link
Author

As of right now, I think the best idea is to try to transfer over the code to a new github but keep what is currently here, here. Once stuff is cleaned up, you'd then start to use the new & approved library.

So for how this relates to displays... One of the benefits of adding a function like .showCombinatorGUI(x, y, { ...data_here... }) is that it makes it much easier for developers to get a project running (same with chest inventories and other things) and can also look/feel like Factorio's GUI. The negative is that it can't pop outside the canvas. Regardless, I think this is something that could be left on fbpviewer and then decided later.

@simonvizzini
Copy link
Contributor

First off, the JS code itself is really not bad, only thing necessary is some restructuring. Today is a holiday and tomorrow I have a day off, so I'd have some time to help get this started, but only if you want of course, if not then I have some other projects I'll work on, no problem :)

I took a look at the JS part and here is how I would personally approach it:

  1. First move all of the JS and dependencies out of src/AppBundle/Resources/public to its own directory, e.g. src/FactorioBlueprintRenderer (you decide naming and casing, I don't care)
  2. Add a npm package.json with the dependencies (bootstrap, PIXI, etc)
  3. Change code to require lib dependencies
  4. Add a bundler (I suggest webpack) and let it output the JS bundle to src/AppBundle/Resources/public, and adjust code to make it work.

Should be simple enough until now and you could move this to a seperate repo at this point, but I wouldn't yet. At this point I would convert everything to TypeScript. I know a lot of people dislike TS, and I can understand because it seems frightening and complicated at first look, but it's much simpler than it seems and the advantages should be clear: once you have types you get tooling and compiler support, while still having the dynamic power of JS. It makes life so much easier, the code is easier to explore and grasp as a new contributor, adding new features without breaking existing things is much simpler... and so on. Oh and it is possible to mix TS with JS code without problems.

So if you want, I would do the TS conversion. It shouldn't take me longer than a few hours (at work I'm converting our huge JS code base to TS since around 8 weeks by now, and we are not done yet, so I have a bit of practice by now)

After that I would think about further refactoring: How to properly separate the renderer from the website, think about a nice public API, and move it to it's own repo. But that's something you will have to decide, this is your baby :)

@demipixel
Copy link
Author

this is your baby

Oh god. I'd love to make it "my baby" but the first part is organizing. I looked into some of this stuff today—the sprite generator specifically being a pain because I can't tell how the factorio graphics are organized (I did my best guess by bringing it core/graphics and renaming it core/ and bringing in base/graphics/* into images, but then some internal folders have their contents put in the parent...

It also becomes an issue because it ends up evalling a bunch of the files. However, this could be a bit cleaner when using require() (which would now be possible).

No experience with webpack, only slightly with browserify and gulp, but I suppose it's the new hip thing :P

I (unfortunately) don't know TS yet. I'm sure if the project was converted I'd learn it, but I'm not sure if I'd be willing to convert it over myself.

Obviously it needs an API! I think the best bet is to support passing in a canvas, so you can also run it via node, and pass in a server-side canvas (for bots to generate images/gifs or whatnot). I've written a library, factorio-blueprint, which supports a lot of the basics you'd expect (creating entities, performing actions such as setting their direction, recipe, or connecting them to anothers via wire). Do you think it would be benefitial to the project? This reduces the amount of "parsing" you have to do for things like wire connections (although there's still some unrefined things in factorio-blueprint such as entity.items holding modules, but there isn't much parsing as it's mainly just renamed entity.modules).

Just one clarification—I was presuming this would be a separate project, so there wouldn't necessarily be AppBundle folders or anything like that, just an npm module with some tests in an index.html to show it off. How did you expect to go about it?

@simonvizzini
Copy link
Contributor

simonvizzini commented Jun 15, 2017

I wouldn't worry about the sprite generator right now, I'd focus on the renderer first, and the renderer doesn't care how the sprite is generated, it just needs a sprite and there is one for now. That's something that could be later improved. Same for the entity data which is currently coded as functions. I've seen some Github projects that extract Factorio entity data to JSON, I'll have to check again but I think this could be used to provide the data (possibly including modded data?)

No experience with webpack, only slightly with browserify and gulp, but I suppose it's the new hip thing :P

I've used browserify, gulp, grunt and webpack, and yep, webpack is the current hip thing ;) Webpack is finally a simpler solution to the bundling problem than the previous tools. For this project I guess the webpack config will have around 30 lines that are easy to understand.

I (unfortunately) don't know TS yet. I'm sure if the project was converted I'd learn it, but I'm not sure if I'd be willing to convert it over myself.

I'd do the conversion, no problem, this project is still small enough to do it in a few hours. After that all you need is a editor with TS support (I use VS Code, but there are plugins for most editors like Atom, Sublime, ...), browse the code and be amazed :) You'll easily figure out how to use types once you want to write/change code. Some compiler errors may seem cryptic at first, but there is a pattern to easily understand what's wrong, and I'll be here to answer questions.

I have to go for lunch now, but I'll quickly address your questions on how I imagine this:

The end result should be a npm module that accepts a canvas and a blueprint, like you suggested, so no AppBundle folder. I'll have to look into your factorio-blueprint lib, but it sounds like it could help?

On top of that renderer module could be built another module that does the browser integration, with some nice dialogs, entitiy manipulation and what not. Others could then use the low level canvas renderer (on the server or browser), or the "browser integration" (however you wanna call it) module to integrate into their own web app. fpbviewer website could be then built upon this browser module, for example.

@demipixel
Copy link
Author

Awesome, sounds like we're on the same page! Time to make it work I suppose.

@simonvizzini
Copy link
Contributor

I'm almost ready to start this. I hope @trakos wakes up soon :D I guess he had a long night after all the reddit fame and the quick open sourcing on Github ;)

In a hour or so I'll make a fork and start on steps 1-4 that I proposed, I think it's necessary and I hope @trakos won't mind. But I won't do any TS conversion until he shows up and agrees.

@simonvizzini
Copy link
Contributor

I'm having some trouble to get this whole thing running with docker. I've never used docker before (am unfortunately stuck to MS tech at work) and I get some errors executing the dev_init.sh script.

I'm on Arch linux and so I installed docker, docker-compose and docker-machine. docker-compose up -dexecuted fine and downloaded all images and docker ps shows the new containers. But I'm not sure what to do with docker-machine? docker-machine ip tells me there is no default machine configured. I guess I need to configure it first.

And dev_init.sh asks me some questions I'm not sure about (like mysql username and password, which seems to be already defined in the docker_compose.yml?), followed by some errors: https://hastebin.com/manuvejula.coffeescript

Any ideas?

@jodli
Copy link
Contributor

jodli commented Jun 15, 2017

Hi :)

docker-machine is like a vm for docker containers. You only need it if you want to separate your containers from others.

So when the containers are running (you can also type docker-compose ps) you should connect to the nginx container via localhost:80 in your browser.

Doctrine looks like the ORM for the php backend but I can't see where it gets user and password for the mysql database...

@simonvizzini
Copy link
Contributor

simonvizzini commented Jun 15, 2017

Hi and thanks @jodli. I was really hoping this docker stuff will just work (that's what I keep reading everywhere)... but it's not as simple it seems. I started to go through the docker docs and set up a docker-machine (it sounds quite nice) with virtualbox as driver, the basic hello world examples work, but docker-compose up -d fails for this project (I set my env to my new docker-machine): https://hastebin.com/upapapogoj.vbs

Really lost what to do here, Google brings up some results and it seems to be related to volume mounting, but I can't spot the error in the docker_compose.yml file...

And I haven't used PHP in over 10 years, so no idea how Symfony or Doctrine works, but it seems really complicated for what it does. Ah well, looks like I'll go play some Factorio for now :)

Edit: I gave it another shot, without docker-machine, and it seems to work. I set php-docker.local to localhost in my hosts, as suggested in the README, but then I got directory permission errors from Symfony. Turns out the var folder that got created in the repo root by docker was owned by root:root for some reason. I couldn't figure out which user/group is supposed to get write access, so in the end I changed permissions to 777 on the whole var folder, and now it works.

@trakos
Copy link
Owner

trakos commented Jun 17, 2017

Those errors you pasted when using docker-machine were probably because of mounting problems. Docker-machine by default mounts only your user directory, so if your project is outside of it it simply doesn't mount anything at all, so the chdir failed. I usually simply add the shared directories to virtualbox myself.

I forgot about parameters.yml. Generally I didn't include those in the repo because they're obviously different on production. Here are values required for docker:

parameters:
    database_host: mysql
    database_port: null
    database_name: symfony
    database_user: symfony
    database_password: symfony
    mailer_transport: smtp # whatever
    mailer_host: 127.0.0.1 # whatever
    mailer_user: null # whatever
    mailer_password: null # whatever
    secret: ThisTokenIsNotSoSecretChangeIt  # whatever
    shared_blueprint_host: 'http://php-docker.local'

The mailer stuff doesn't matter, I don't use those (yet?). Maybe I'll simply include those values in readme later on.

Also, when you're using docker, most of the things are run by root, (not your real root, the one inside the vm/container). So that's why the var directory was owned by root. If you encounter more permissions problem, you can add umask(0000); somewhere in the web/index.php, then the directories it will create will have 777.

Let me know if you'll have more problems.

@jodli
Copy link
Contributor

jodli commented Jun 17, 2017

The permission problem could always be fixed by moving the cleanup of the dependencies to the php container itself. There it runs as the same root user as all the other commands. As I was seeing the cleanup is the only command that has problems with the folder permissions.

I've created a PR #3 that fixes this.

After changing the database_host to the container name it works on my machine. 👍

@simonvizzini
Copy link
Contributor

@trakos I'll look into docker-machine again when I have the time, for now just docker works. All my projects are on my home partition by the way, if that is what you meant with it mounts the user directory by default? Apparently that didn't work in my case, but I'll see how to add it myself in virtualbox. Also the parameters you provided work for me now.

@jodli, thanks! Your PR solved that problem, although I'm wondering if this is really how docker is supposed to work? I cannot delete folders created by docker as my normal user, and I wasn't allowed to edit the parameters.yml that was generated. Is there a better way to handle files written by docker?

@trakos
Copy link
Owner

trakos commented Jun 18, 2017

@simonvizzini Well, the partition doesn't matter, it just has to be in the $HOME directory. And it is the way that docker is usually used, as far as I know. You can change the user in docker-compose (simply add line user: ":" for every container), but the tricky thing is that you have to specify your local uid, which makes no sense in public repo. They had some discussions about it in docker community: docker/compose#1532

@demipixel I've moved javascript package to https://github.com/trakos/fbpviewer-js , for now it's still tightly coupled, but it should be a bit easier now to split it with changes from simon.

By the way, simon mentioned discord, I've created one today: https://discord.gg/T5dvK3t , if you'd be interested in using it.

@trakos trakos self-assigned this Jun 18, 2017
@trakos trakos closed this as completed Jun 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants