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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Dec 4, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
154f9b7
Add .editorconfig
ryepup Nov 30, 2017
0d58e35
WIP: Start breaking apart logic and view
ryepup Nov 30, 2017
4b1232b
WIP: Split view/logic for the grid
ryepup Dec 1, 2017
00ae993
WIP: use factorio-blueprint for parsing/encoding
ryepup Dec 1, 2017
b5d96ff
WIP: Fix rotation and drag bugs
ryepup Dec 2, 2017
55f1bf6
Merge remote-tracking branch 'upstream/master' into use-factorio-blue…
ryepup Dec 2, 2017
76fea93
Misc bug fixes
ryepup Dec 2, 2017
e44cbb9
code review feedback
ryepup Dec 3, 2017
02d3637
Improve readability
ryepup Dec 3, 2017
2a6aef3
Merge remote-tracking branch 'upstream/master' into use-factorio-blue…
ryepup Dec 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion js/view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ window.FBE = window.FBE || {};
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);
var rotation = placeable.direction - placeable.defaultDirection;
if (rotation < 0) { rotation += 8; }

//Handles usecases where entity should be horizontally flipped instead of rotated, like inserters. Rotation 4 = 270 degrees
Expand Down
42 changes: 30 additions & 12 deletions js/viewmodel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

defaultDirectionsByEntityName = [
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

{
regex: /inserter|offshore_pump/i,
direction: 6
},
{
regex: /splitter|burner_mining/i,
direction: 4
},
{
regex: /electric_mining|pipe_to_ground|steam_engine|steam_turbine|pump|gate|arithmetic_combinator|decider_combinator/i,
direction: 2
}
],
factorioBlueprintOverrides = {
// fix bad data in factorio-blueprint
'steam_turbine': { width: 5, height: 3 },
'boiler': { width: 3, height: 2 },
// fix spots where height/width are swapped
'pump': { width: 2, height: 1 },
'decider_combinator': { width: 2, height: 1 },
'arithmetic_combinator': { width: 2, height: 1 }
}
;

FBE.viewmodel = {
Expand Down Expand Up @@ -156,13 +179,7 @@ window.FBE = window.FBE || {};
name: name
},
rawEntities[name],
// fix bad data in factorio-blueprint
name === 'steam_turbine' && { width: 5, height: 3 },
name === 'boiler' && { width: 3, height: 2 },
// 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 }
factorioBlueprintOverrides[name]
Copy link
Contributor

Choose a reason for hiding this comment

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

That's definitely clearer to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

);
})
.filter(isPlaceable)
Expand Down Expand Up @@ -212,10 +229,11 @@ window.FBE = window.FBE || {};
function getDefaultDirection(entity) {
if (entity.type === 'input') { return 2; }
if (entity.type === 'output') { return 6; }
if (/inserter|offshore_pump/.test(entity.name)) { return 6; }
if (/splitter|burner_mining/.test(entity.name)) { return 4; }
if (/electric_mining|pipe_to_ground|steam_engine|steam_turbine|pump|gate|arithmetic_combinator|decider_combinator/.test(entity.name)) { return 2; }
return 0;

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

return (first && first.direction) || 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

still doing this strange conditional that returns a non-boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

})(window.FBE);