redruin1 / factorio-draftsman

A complete, well-tested, and up-to-date module to manipulate Factorio blueprint strings. Compatible with mods.
MIT License
94 stars 17 forks source link

Error in 'filtered_train.py' #34

Closed flameSla closed 2 years ago

flameSla commented 2 years ago

cargo_wagon.tile_position <- incorrectly sets the x and y coordinates

cargo_wagon = CargoWagon("cargo-wagon", position=[7 * train_car_position, 0], orientation=0.75) <- works correctly

cargo_wagon.tile_position = (7 * train_car_position, 0) <- incorrect coordinates { x=... , y=2.5 ??? }

redruin1 commented 2 years ago

This was due to tile_position handling coordinates in terms of the top left corner tile of the entity. However, this doesn't really make much sense for an entity like a cargo wagon or locomotive, as their "tile position" is highly dependent on their orientation, which isn't currently being taken into account. For the next version 1.0.0, I swapped all references from cargo_wagon.tile_position to cargo_wagon.position, which fixes the unexpected offsets.

As to what to do with tile_position for CargoWagon and other rolling stock, I'm not sure. It can be useful assuming the wagon is facing one of the cardinal directions (0.0, 0.25, 0.5, 0.75), but once it's out of this range this position makes little sense without complex transformations (which I'm not sure are really a smart decision, both from a user-experience perspective as well as a performance perspective). Another option would simply be removing this particular attribute from rolling stock and only allow setting the position from the position attribute, but this would require some refactoring on the back end, as all entities act as if they have a tile_position.

flameSla commented 2 years ago

You can create a "Train" class:

t = Train()
t.append( CargoWagon )
t.append( Locomotive )

And then: t.move_to( NewPosition, NewOrientation )

redruin1 commented 2 years ago

In all likelihood, something like that will probably get added underneath the RailPlanner class that I've been procrastinating on, which will allow you to place rails via pen commands (move_forward(10), turn_left(2), place_signal(RailChainSignal(), "right"), and would probably do well with place_stock(CargoWagon()) or similar)

Unfortunately, the reason why I've been procrastinating on it is that rails are a royal PITA to work with, and have been the source of many errors and issues throughout Draftsman's lifetime. If anyone is doing a lot of work with rails and wants to contribute to RailPlanner I will (happily!) merge it.