Split into view, viewmodel, and factorio-blueprint #48

ryepup · 2017-12-02T22:05:23Z

Refactored to break apart the logic, and use https://github.com/demipixel/factorio-blueprint/ as the data model.

script.js - run app-startup logic, mostly hooking up DOM to view
view.js - functions for building various UI elements, and reacting to different UI and model events
viewmodel.js - functions for managing the state of the application, a mix of synchronous calls and event-driven callbacks

The view changes the original implementation significantly; I think it's doing more DOM operations, and less CSS positioning.

A lot of this is pretty clumsy, written in an old-school style to support IE11 without any build process. If #45 goes through then much of this code could be better expressed as a handful of classes.

There are still bugs in here, but they seem to match the ones in master so I think this refactor is close enough. Several issues will get much easier if this gets merged: #25, #5, #4. We should also get fewer merge conflicts moving forward, since the code isn't all in one place.

fixes #16
fixes #39
fixes #42
fixes #36

Match the eslint settings from camerongillette#18

Have sidebar and preview implemented, tiles are next then I'll be able to delete a lot of code. * breaking the big script.js into a view.js, responsible for the UI, and viewmodel.js, responsible for the data+logic * lots of small functions * starts to use factorio-blueprint as the model * pull in TinyEmitter to get event-driven behavior between view and viewmodel Still lots of bugs, but a decent evening's progress. Should be more straightforward work to clear out most of script.js, although they'll be some nasty merge conflicts to fix since this refactor touches basically everything. refs camerongillette#16 refs camerongillette#42

* lots of small functions to build the grid and handle DOM events * add a handful of custom events in the viewmodel to help keep in sync without a bunch of plumbing * delete a lot of freshly dead code in script.js, including the big placeable array * move some images into the placeables folder so they'll show up in the siderbar view.js is probably too big; will be able to simplify a lot if ES6 is on the table (see camerongillette#45) refs camerongillette#16

The loading process is pretty clumsy; jumping through hoops to get in/out directions for belts. refs camerongillette#16

* fix some problems with entities not rotating correctly - restored some futzy logic in the view that I deleted from script.js * port bugfixes from camerongillette#44 * port bugfixes from camerongillette#38

…print

* fix some weird data coming from factorio-blueprint * exclude curved rail; it's a special snowflake that will need a lot of work to handle properly * fix broken/dead references in index.html * prevent uncaught exceptions around mouse events * eslint fixes * version bump refs camerongillette#16

Zeragamba · 2017-12-02T23:04:24Z

css/style.css

@@ -72,6 +72,10 @@ html {
    opacity: 0.75;
 }

+.mouse__preview.blocked {


wouldn't blocked be a modifier on the preview? eg. .mouse__preview--blocked

not 100% sure how BEM works for this kind of thing.

I don't know BEM, is this project strict about BEM usage?

Zeragamba · 2017-12-02T23:27:38Z

js/viewmodel.js

@@ -44,7 +44,7 @@ window.FBE = window.FBE || {};

    function loadEntity(placeables, entity) {
        var items = placeables
-            .filter(function (p) { return p.name === entity.name; }),
+                .filter(function (p) { return p.name === entity.name; }),


can we move this onto the previous line, or give item there it's own var statement.

Zeragamba · 2017-12-02T23:32:47Z

package.json

  "description": "Online Editor for Elxeno's Factorio Blueprint Editor",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1",
-    "lint": "./node_modules/.bin/eslint js/**/*.js"
+    "lint": "eslint js/**/*.js"


we should keep using the project's version of eslint instead of relying on a globally specified version

npm handles this already, the ./node_modules/.bin is redundant.

It basically puts ./node_modules/.bin at the front of your PATH so we are using project's dependencies before anything installed system wide.

The official docs are very terse on this: https://docs.npmjs.com/misc/scripts#path

Oh. I did not know that. Nvm then

Zeragamba · 2017-12-02T23:34:28Z

js/viewmodel.js

+    var events = new TinyEmitter(),
+        bp = new Blueprint(),
+        selectedItem,
+        entityNamesThatDoNotRotate = /beacon|roboport|lab|heat_pipe|pipe$|furnace|chest|pole|substation|solar|accumulator|centrifuge|wall|rocket_silo|radar|turret|speaker|power_switch|reactor|lamp|land_mine/i


well, that be a piece of regex...

wouldn't an array and #indexOf work just as well?

it'd be a much bigger array. With the regex I can say chest , whereas an array would need wooden_chest, iron_chest, etc. similar story for turrets, poles, furnaces.

Zeragamba · 2017-12-02T23:34:59Z

js/viewmodel.js

+            : '';
+
+        var fileName = directionPrefix
+            + entity.name.replace(/_/g, '-') // TODO: rename files so we can drop this


opened #49 to get this fixed

Zeragamba · 2017-12-02T23:35:31Z

js/viewmodel.js

+    function getDefaultDirection(entity) {
+        if (entity.type === 'input') { return 2; }
+        if (entity.type === 'output') { return 6; }
+        if (/inserter|offshore_pump/.test(entity.name)) { return 6; }


can we move these regexes out into constants and/or arrays?

Zeragamba · 2017-12-02T23:36:13Z

js/viewmodel.js

+                    // fix spots where height/width are swapped
+                    name === 'pump' && { width: 2, height: 1 },
+                    name === 'decider_combinator' && { width: 2, height: 1 },
+                    name === 'arithmetic_combinator' && { width: 2, height: 1 }


can we move these into a switch statement? It took me a while to understand what these && were doing

Zeragamba · 2017-12-02T23:36:51Z

js/view.js

+        div.style.width = placeable.width * 32 - 2 + "px";
+        div.style.height = placeable.height * 32 - 2 + "px";
+        if (placeable.canRotate) {
+            var rotation = (placeable.direction - placeable.defaultDirection);


seems like the brackets are unneeded

agreed; at some point I was trying to work it out with % but that was a dead end.

Zeragamba · 2017-12-02T23:37:19Z

I apologize for the email spam...

Mostly readability tweaks

Zeragamba · 2017-12-03T02:32:29Z

js/viewmodel.js

-                    name === 'pump' && { width: 2, height: 1 },
-                    name === 'decider_combinator' && { width: 2, height: 1 },
-                    name === 'arithmetic_combinator' && { width: 2, height: 1 }
+                    factorioBlueprintOverrides[name]


That's definitely clearer to read.

Zeragamba · 2017-12-03T02:36:33Z

js/viewmodel.js

@@ -7,7 +7,30 @@ window.FBE = window.FBE || {};
    var events = new TinyEmitter(),
        bp = new Blueprint(),
        selectedItem,
-        entityNamesThatDoNotRotate = /beacon|roboport|lab|heat_pipe|pipe$|furnace|chest|pole|substation|solar|accumulator|centrifuge|wall|rocket_silo|radar|turret|speaker|power_switch|reactor|lamp|land_mine/i
+        entityNamesThatDoNotRotate = /beacon|roboport|lab|heat_pipe|pipe$|furnace|chest|pole|substation|solar|accumulator|centrifuge|wall|rocket_silo|radar|turret|speaker|power_switch|reactor|lamp|land_mine/i,


These regexes still feel wrong, as they are just a list of options. i think these would be better handled by an array.

Also doing a little bit of searching, i think this regex match performs a lot slower than using indexOf

I believe Array.includes() is what you should be using to check the existence of a string in an array

This regex is called once at app startup and whenever we load a blueprint. I don't think it's a performance bottleneck.

Profiling on my aging laptop (i5, chrome, ubunut) it takes 5ms to call getPlaceableItems, and of that, 0.16ms is spent in entityNamesThatDoNotRotate.test.

Is that fast enough?

right, but this still feels like an abuse of regex. It's being used like an array, instead of using an array.

Will merge this in for now, but opened #54 to get this fixed

Zeragamba · 2017-12-03T02:40:47Z

js/viewmodel.js

+        var first = defaultDirectionsByEntityName
+            .filter(function(x){ return x.regex.test(entity.name);})[0];
+
+        return (first && first.direction) || 0;


still doing this strange conditional that returns a non-boolean

Using the logical operators to for short-circuit evaluation is a pretty common idiom, and the simplest way to guard against nulls or provide default values.

See also: http://eloquentjavascript.net/01_values.html#h_3jN0iK4yKW

In this case, I think a ternary will be clearer, but it'll still be using non-booleans / truthiness

Zeragamba · 2017-12-03T02:40:49Z

js/viewmodel.js

@@ -7,7 +7,30 @@ window.FBE = window.FBE || {};
    var events = new TinyEmitter(),
        bp = new Blueprint(),
        selectedItem,
-        entityNamesThatDoNotRotate = /beacon|roboport|lab|heat_pipe|pipe$|furnace|chest|pole|substation|solar|accumulator|centrifuge|wall|rocket_silo|radar|turret|speaker|power_switch|reactor|lamp|land_mine/i
+        entityNamesThatDoNotRotate = /beacon|roboport|lab|heat_pipe|pipe$|furnace|chest|pole|substation|solar|accumulator|centrifuge|wall|rocket_silo|radar|turret|speaker|power_switch|reactor|lamp|land_mine/i,
+        defaultDirectionsByEntityName = [


the variable name sounds like it will contain a map, with a EntityName being the key. entityDefaultDirections maybe?

Zeragamba · 2017-12-04T00:28:57Z

js/viewmodel.js

@@ -7,7 +7,30 @@ window.FBE = window.FBE || {};
    var events = new TinyEmitter(),
        bp = new Blueprint(),
        selectedItem,
-        entityNamesThatDoNotRotate = /beacon|roboport|lab|heat_pipe|pipe$|furnace|chest|pole|substation|solar|accumulator|centrifuge|wall|rocket_silo|radar|turret|speaker|power_switch|reactor|lamp|land_mine/i
+        entityNamesThatDoNotRotate = /beacon|roboport|lab|heat_pipe|pipe$|furnace|chest|pole|substation|solar|accumulator|centrifuge|wall|rocket_silo|radar|turret|speaker|power_switch|reactor|lamp|land_mine/i,


Will merge this in for now, but opened #54 to get this fixed

Zeragamba · 2017-12-04T00:29:32Z

@camerongillette can you resolve the merge conflicts please?

…print Also comply with latest eslint config

ryepup · 2017-12-04T02:28:44Z

resolved merge conflicts. With ES6 on the table there's a lot of cleanup that can be done; I was sorely missing arrow functions especially

ryepup added 7 commits November 30, 2017 00:22

Add .editorconfig

154f9b7

Match the eslint settings from camerongillette#18

WIP: use factorio-blueprint for parsing/encoding

00ae993

The loading process is pretty clumsy; jumping through hoops to get in/out directions for belts. refs camerongillette#16

WIP: Fix rotation and drag bugs

b5d96ff

* fix some problems with entities not rotating correctly - restored some futzy logic in the view that I deleted from script.js * port bugfixes from camerongillette#44 * port bugfixes from camerongillette#38

Merge remote-tracking branch 'upstream/master' into use-factorio-blue…

55f1bf6

…print

Zeragamba mentioned this pull request Dec 2, 2017

Simplify create json #46

Closed

Zeragamba suggested changes Dec 2, 2017

View reviewed changes

Zeragamba reviewed Dec 2, 2017

View reviewed changes

code review feedback

e44cbb9

Mostly readability tweaks

Zeragamba suggested changes Dec 3, 2017

View reviewed changes

Improve readability

02d3637

Zeragamba approved these changes Dec 4, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into use-factorio-blue…

2a6aef3

…print Also comply with latest eslint config

Zeragamba approved these changes Dec 4, 2017

View reviewed changes

Zeragamba merged commit 5f272d7 into camerongillette:master Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split into view, viewmodel, and factorio-blueprint #48

Split into view, viewmodel, and factorio-blueprint #48

ryepup commented Dec 2, 2017

Zeragamba Dec 2, 2017

ryepup Dec 3, 2017

Zeragamba Dec 2, 2017

Zeragamba Dec 2, 2017

ryepup Dec 3, 2017

Zeragamba Dec 3, 2017

Zeragamba Dec 2, 2017

ryepup Dec 3, 2017

Zeragamba Dec 2, 2017

Zeragamba Dec 2, 2017

ryepup Dec 3, 2017

Zeragamba Dec 2, 2017

ryepup Dec 3, 2017

Zeragamba Dec 2, 2017

ryepup Dec 3, 2017

Zeragamba commented Dec 2, 2017

Zeragamba Dec 3, 2017

ryepup Dec 3, 2017

Zeragamba Dec 3, 2017

Danielv123 Dec 3, 2017

ryepup Dec 3, 2017

Zeragamba Dec 3, 2017

Zeragamba Dec 4, 2017

Zeragamba Dec 3, 2017

ryepup Dec 3, 2017

Zeragamba Dec 3, 2017

ryepup Dec 3, 2017

Zeragamba Dec 4, 2017

Zeragamba commented Dec 4, 2017

ryepup commented Dec 4, 2017

Split into view, viewmodel, and factorio-blueprint #48

Split into view, viewmodel, and factorio-blueprint #48

Conversation

ryepup commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zeragamba commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zeragamba commented Dec 4, 2017

ryepup commented Dec 4, 2017