factorio-blueprint icon indicating copy to clipboard operation
factorio-blueprint copied to clipboard

Overlap with Railways

Open CandleCandle opened this issue 6 years ago • 5 comments

Railways are hard!

  • Curves have a non-square overlap box
  • Rails can be aligned in 8 directions
  • There's a constraint that, while signals are a 1x1, there is something like one extra space either side of them.

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

CandleCandle avatar Apr 23 '18 12:04 CandleCandle

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.

CandleCandle avatar Apr 27 '18 12:04 CandleCandle

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

demipixel avatar Apr 27 '18 19:04 demipixel

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

CandleCandle avatar Jun 06 '18 19:06 CandleCandle

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?

demipixel avatar Jun 06 '18 21:06 demipixel

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.

CandleCandle avatar Jun 07 '18 08:06 CandleCandle