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.

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

Extract train definition logic #39

Closed
AP-Hunt opened this issue Aug 26, 2016 · 7 comments
Closed

Extract train definition logic #39

AP-Hunt opened this issue Aug 26, 2016 · 7 comments

Comments

@AP-Hunt
Copy link

AP-Hunt commented Aug 26, 2016

Hi Choumiko,

Having read through your code, it looks like you've really dug in and solved the problem of working out which locomotives & wagons belong to which train, and how to persistently identify those trains. I think other Factorio modders could really benefit from being able to reuse that code (unashamedly myself included).

Is creating a reusable library from that code something you'd be interested in doing? I'd certainly be willing to help out.

Regards,
Andy

@Choumiko
Copy link
Owner

Sure, the current functions are still a bit WIP i think and i have a few TODOs left from the old train tracking, where i have no idea anymore what they do.
I can easily pull it into an extra file with comments what function should be called where.

@AP-Hunt
Copy link
Author

AP-Hunt commented Aug 27, 2016

Thanks, I appreciate your openness to the idea. I think there could be 2 other interesting ways of distributing the code:

  1. As a standalone library mod that people interact with through remote calls
  2. As part of the Factorio stdlib project.

What do you think?

@Choumiko
Copy link
Owner

I thought about option 1 before, but discarded it at the time (not sure why), but that was when finding data for a specific train was O(n), now it is more like O(1) (if the train wasn't changed) so i guess there's not much to gain for the trouble/overhead (optional dependencies, remote.calls between mods) now.

Haven't thought about integrating it into stdlib yet. Sounds interesting, since it already has functions to store data for individual entities and support multiple handlers for events.

I don't know whether there is a need to share train related data between mods. I guess one could gain a few milliseconds if e.g. SmartTrains and FatController shared the inventory counts of the trains, though i don't know if the remote.call overhead isn't too much.

I'll definitely give the stdlib way a go (soon TM) xD

@AP-Hunt
Copy link
Author

AP-Hunt commented Aug 27, 2016

I don't think a standalone mod version would exist for the purpose of sharing train data between mods, but instead to act as a library a modder can call in to find out about trains that exist in the world; as opposed to writing that code themselves..

Eg

me = game.players[1]
nearby_trains = remote.call("trains", "nearby-trains", me.position)

Of course, that's the same benefit as adding it to the stdlib. The only difference is the remote.call usage and the ability for the mod author to depend on it through the mod system.

I appreciate I'm the one who brought this up, so to save you some time and effort would you be OK with me attempting to implement your code in the stdlib project? It would still be credited as your code wherever such credit is given, of course.

@Choumiko
Copy link
Owner

Sure, go ahead with stdlib integration, anything i don't have to do is fine with me. Next time i'm working on SmartTrains i'll try to remember moving the train tracking stuff into it's own file and clean it up a bit and refernence this issue in the commit, so you can see if i found any problems/simplifications

@AP-Hunt
Copy link
Author

AP-Hunt commented Aug 28, 2016

Fantastic, thanks. I'll link you the pull request when it's done.

@AP-Hunt
Copy link
Author

AP-Hunt commented Sep 8, 2016

Hi Choumiko,

I've created a pull request to Factorio StdLib as discussed above. In the end, I ended up lifting and shifting very little of your code explicitly. Instead, I found myself studying it and writing my own code to do a similar job. Regardless, I'm grateful to you for spending the time to understand the problem to begin with and for being open to me copying your code to another project.

I'm going to close this issue now, since discussion will take place on the PR.

Andy

@AP-Hunt AP-Hunt closed this as completed Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants