Xcone / factorio_pump

MIT License
1 stars 5 forks source link

Add missing directions and improve directory discovery #22

Closed joelverhagen closed 9 months ago

joelverhagen commented 9 months ago

This is only for the layout tester tool. Factorio's util.lua script now uses 4 more directions for a util.direction_vectors global.

I enhanced the directory discovery for solutionRoot and factorioDir to allow for Steam locations and different current directory settings.

I love your mod. Thanks for innovating here.

I'm currently working on running my planner algorithm in a Factorio mod. Up until now it's a .NET web app (https://factorio-tools.vercel.app/oil-field) but I've been able to transpile the core algorithms to Lua using the CSharp.lua project. I have my planner algorithm running in Factorio now with a hacked-up version of your mod. I was thinking of forking your mod so I can change/add additional planner options but if you are interested in enhancing your mod to have additional planner options, I'd be interested in working with you. I plan on doing a performance analysis (both runtime and plan quality) of your existing planner vs. FBE vs. my algorithms. In the future, I want to port Autotorio's planner algorithm too so all of them are available to the user.

edit: I also want to add beacon placement to the mod. If these things sound too much for P.U.M.P. I am happy to fork but I also want to sense out the opportunity of making changes directly to P.U.M.P.

Xcone commented 9 months ago

Nice improvements. Thanks :-)

Sorry for the late reply. I got the notification at an inconvenient time, and completely forgot about it afterwards...

I think you missed the path in TestLayoutRunner.xaml.cs. It's not essential, but it adds a filesystem watch so the layout tool refreshes when you save a lua file. Might be nice if that one plays ball with the other paths.

So, .. I understand you have a similar goal in mind as P.U.M.P. currently provides, but with a different planner (presumably with slightly different priorities, and therefor different outcomes)? And you'd like to add it to P.U.M.P.?

I am open to that. I've toyed with the idea to have different planner-flavours in P.U.M.P. but the opportunity/need was never there. Seems that might change with your proposal. Especially with a thing like beacons. It's not something I'd bother to add, but there has been a request for it. So if P.U.M.P. has different planner-options; players could chose a planner that does include beacons.

So, if you still want to add your planner to P.U.M.P. that sounds good to me. How do we proceed?

joelverhagen commented 9 months ago

I think one way to proceed is for me to fork P.U.M.P. and start PR'ing additions to your mod incrementally. This is probably the easiest for you to review if you want to go line by line but it's harder for me since I would be undoubtedly go back and edit things I already changed which is review overhead. Alternatively, I can fork it and get to some "goal state" and show you what I've come up with. Then you can say which parts if any you want ported back into the main mod. Or, I could make a mega PR but those are never easy :). It's pretty much up to you!

As far as user experience, I've noticed my planners are way more compute-intensive than yours so I think having an option of "fast" vs. "intensive" should be given to the user in addition to the entities they want placed (pipes, poles, beacons, etc). My .NET implementation runs all of my planner combinations and returns the best. I don't think this is best for Factorio since it causes noticeable lag when the calculation is done (1-10 seconds of hanging).

Xcone commented 9 months ago

So as you maybe know, P.U.M.P. works with a few stages. The prospector, planner, and constructor stage. All three rely on a toolbox, which is basically the small UI offering a selection which pump, pipe and poles to use.

I think I'd like close control on the first and the last stage as well as the toolbox, and then make the planner stage changeable. Either by user option in the menu, or maybe even try to read planners from other mods. (I think mods can depend on each other that way. This might add some complication, but would save on the review-effort as well as release-flow).

I hope this general approach with the 3 stages doesn't need changing (apart from expanding it a little). And that means you can go nuts with your planner(s) and I don't really need to care so much about what goes along inside of it. As long as it can do it's job with the prospector output, and provides a valid constructor input.

Sooo, ... the toolbox and the constructor needs to be expanded with options for beacons (if the selected planner supports it). P.U.M.P. currently depends on the mod module-inserter to add modules to the pumps. I am not entirely sure that still makes sense when beacons become a thing in P.U.M.P. But you know that better then me. This might need a "use speed" or "use productivity" or "defer to module-inserter settings" kind-of system.

So, .. with those things said; Do you think more changes are needed to prospector and constructor? And if so, what kind of changes? And would you prefer embedded planners, or rather go for the extra-planners-via-mod-dependencies route?

joelverhagen commented 9 months ago

I think I'd like close control on the first and the last stage as well as the toolbox, and then make the planner stage changeable. Either by user option in the menu, or maybe even try to read planners from other mods. (I think mods can depend on each other that way. This might add some complication, but would save on the review-effort as well as release-flow).

Yup, I like the logical separation. It was clear as I was playing with the mod source code.

I think I'd like close control on the first and the last stage as well as the toolbox, and then make the planner stage changeable. Either by user option in the menu, or maybe even try to read planners from other mods. (I think mods can depend on each other that way. This might add some complication, but would save on the review-effort as well as release-flow).

Mod dependencies seem like a good option for the future if there are a lot of planners iterating at different rates and by many parties, but I think the extra work to maintain multiple mods or separate authorship and also user install flow make it probably not worth it yet. I'm happy to go down that route if you feel strongly about it but personally, I think having one great liquid extractor planner mod is enough, as long as the options are nicely configurable and not too complex for the basic cases.

I've done software engineering professionally for a long time and it feels like the mod-depending-on-mod option would be a premature/uninformed abstraction. We might cut the wrong abstraction at first and there may only ever been one more implementation (mine). And changing the abstraction is painful and breaking in the future if we get it wrong. And it add complexity for the user. But anyways that's just my gut feeling as a software engineer :)

I hope this general approach with the 3 stages doesn't need changing (apart from expanding it a little). And that means you can go nuts with your planner(s) and I don't really need to care so much about what goes along inside of it. As long as it can do it's job with the prospector output, and provides a valid constructor input.

Yup, I think that will work. I was able to integrate the Lua build of my FactorioTools project into a hacked up P.U.M.P. just by swapping out plumber. There was just some simple mapping glue code before and after my planner and commenting out electrician (refactoring needed).

Sooo, ... the toolbox and the constructor needs to be expanded with options for beacons (if the selected planner supports it). P.U.M.P. currently depends on the mod module-inserter to add modules to the pumps. I am not entirely sure that still makes sense when beacons become a thing in P.U.M.P. But you know that better then me. This might need a "use speed" or "use productivity" or "defer to module-inserter settings" kind-of system.

I think you actually have 2 planners already. A pipe planner and an electric pole planner (plumber and electrician respectively -- I love the names 😀). I have my own electrician and several plumbers, as well as some beacon planners. Here's a screenshot of the steps I have implemented today:

image

I'm imagining an advanced mode where you can pick 1-N plumbers, 1-N beacon planners, and 1-N electricians (maybe). And then the mod runs all the combinations and returns the results. A simple mode would have a "fast", "balanced", and "optimal" choice where "fast" uses the single fastest planner (based on perf tests I can run), "balanced" runs a small number of well performing planners, and "optimal" runs all the combinations, maybe making the game hang for 10 seconds. This is just brainstorming but I hope you get the idea.

Said another way P.U.M.P.'s plumber can run with my beacon planner or FBE's. Or my pipe planner can run with your electrician. So each step is swappable as far as I have seen.

So, .. with those things said; Do you think more changes are needed to prospector and constructor? And if so, what kind of changes? And would you prefer embedded planners, or rather go for the extra-planners-via-mod-dependencies route?

prospector and constructor would need enhancements to support beacons. I think electrician would need to be refactored a bit to be swappable similar to plumber. But I'll know more as I dive in.

Perhaps step 1 is add beacon support (even if it's dead code at first), step 2 is to refactor so there's a clear "spot" to put other planners, step 3 is to add the other planners but only allow swapping via a Lua global or something, and step 4 is enhancing the UI to allow the user to pick different planners.

joelverhagen commented 9 months ago

Oh I forgot to address your question about modules. Modules in both pumps and beacons should be an option IMO. From my chatting with other Factorio players, it seems like some players want modules in both, sometimes even different kinds. Productivity modules don't work in beacons but do in pumps for example.

Xcone commented 9 months ago

Hmm, fair point at the multiple planners. I hadn't considered looking at plumber and electrician as separate planners. There's a fairly clear order between them. I can't just swap the order and hope it works. And esp. with beacons this becomes a thing, as plumber would have to the leave space clear to allow for beacons. So even if by interface they're stackable, functionally it's just a single planner. Just split between files as the file got big and I had to split it somehow in order to be able to find stuff.

Then again, .. to some degree the pipe-burying step I can see separate in your list is one I also do in plumber. So even if it's the same file, it's is in fact a separate step. So ... maybe? :-)

Not sure if players choosing planners going to lead somewhere useful. Though I guess internally it might work if we pick the "pretty, snug, efficient, fast" presets and map each option to a series of planners ourselves. Also, having such extreme flexibility is going to mess up toolbox, too. It's not really pluggable. And if we're going to facilitate that kind of stacking of planners, I fear P.U.M.P. is going to be mod within a mod anyway. Which is going to get more extreme thinking back of embedding the autotorio planners (mentioned in you initial post) as well.


On a sadder note, .. as I had time now to mull things over a little, I am beginning to have second thoughts.

I think you're looking for something quite a bit more sophisticated/advanced then I ever imagined P.U.M.P. to be. It doesn't meet the kind of direction it has been going so far which gives me doubt if I really want it, despite it being really cool to contemplate it. Which, is not a bad thing per se as long is has good defaults. But that combined with me barely working on the mod lately (as a result of me not playing the game anymore), I don't think I'll be engaged very long and will end up in me feeling disconnected from my own mod, and you waiting for me to finally approve PR's. Which is not really fair towards you as well.

Finally, I am unsure how supporting the mod will go after you've added the planners, consider it done, and go on your merry way, and I'll have a mod full of planners that I don't know and understand. I guess this concern is what motivated me to mention the mod-dependency route. As your "extension" would by managed by you instead of me. As an active software developer myself, I agree with your concerns on this approach, though. It's just that I have no way of knowing if you'll be around to support your planners., so embedding them into P.U.M.P. gives me a sense of responsibility I might not really want.

So, .. that said, I think the best way forward is for you to fork P.U.M.P. after all. I am sorry to get your hopes up, but the above concerns/doubts make feel like I need to put in commitments that I'm currently not ready to fulfil. I hope you understand.

I do love the idea, and I would be happy if you make use of P.U.M.P. as a start to make your own thing. Maybe rebrand it a little to avoid confusion between versions. ;-) And if there's some specifics you need help with within P.U.M.P. or some small changes I can make in P.U.M.P. to help you along, feel free to ask.

For now, good luck! And do let me know when you have a release. I'd be curious to see how it turns out. I might even ditch P.U.M.P. and use your mod instead? :p

joelverhagen commented 9 months ago

No worries at all 🙂. Best to chat about the options and plan in advance no matter the next steps. Thanks for mulling it over and providing clarity on your desired next steps!