demipixel / factorio-blueprint

Import and Export blueprint strings automagically with this handy dandy blueprint API
MIT License
105 stars 29 forks source link

Overlap with Railways #32

Open CandleCandle opened 6 years ago

CandleCandle commented 6 years ago

Railways are hard!

I'm looking at improving this as some of my projects also include rails.

CandleCandle commented 6 years ago

My plan is to get to a point where I have a class representing a EntityType and a few implementations that do things like implement tileDataAction differently for curved rails.

Where functionality is different based on the type of entity, that logic is contained in the implementation for that group of entities.

EntityTypes becomes a class with a get(name) which returns an EntityType checkName and checkWithEntityData move to be in the EntityTypes jsName becomes a function on the EntityType class Entity contains a reference to an EntityType and delegates to the type specific implementation where appropriate.

demipixel commented 6 years ago

If rails are the only ones that have this issue, however, maybe it would be a better idea to add a RailroadPlanner or something of the sort? I like the idea of more organization for entities, but right now it works perfectly for everything else, so I'm not sure if it's worth the trouble :P

CandleCandle commented 6 years ago

Work in progress: https://github.com/CandleCandle/factorio-blueprint/blob/entity-mixins/src/entitytypes.js

demipixel commented 6 years ago

This is looking awesome! Only major thing I noticed is that _property() should really use if (value === undefined) and not if (!value).

Also, I might be confused on how this works but:

x(x) {
        if (!this.position) this.position = new Victor({x: 0, y: 0});
        if (!x) return this.position.x;
        this.position.x = x;
        return this;
    }

Why might position not be defined if it's defined as a function just above? And how come you're using this.position.x when this.position is a function?

CandleCandle commented 6 years ago

It's most certainly work in progress.

The first point is certainly a bug and I've changed it to

if (typeof value === 'undefined') return this[name];

The second is also a bug and the property should be _position. The comment on line 7 was a note for me to fix that, seems like (a) I missed that one and (b) there wasn't a test for it.

In index.js you can replace line 7:

const Entity = require('./entity')(entityData);

with

const Entity = require('./entitycompat')(entityData);

to run the tests against my replacement implementation.