BurnySc2 / sc2-planner

MIT License
48 stars 13 forks source link

Auto-add needed tech when adding an item #26

Closed pikiou closed 3 years ago

pikiou commented 3 years ago

When adding e.g. Broodlords in an empty bo, it will add all the required structures and tech needed for it. Works for upgrades and structures as well. Will do the same for terran and protoss next if this pull request is merged.

pikiou commented 3 years ago

New files weren't added to git because I used git commit -am "explanation" when it should have been git add -A && git commit -m "explanation" Added auto-add for protoss. TechLab and Reactor give me a harder time for terran. Added a new menu button like Settings, but for optimizations on the build. There is a lot to add here it'll be a ton of fun!

jtsauls commented 3 years ago

Awesome looking updates!

BurnySc2 commented 3 years ago

Thank you.

But let me slow you down, I want to merge this PR first before it gets too complicated and I am unable to follow the changes. I hope that is alright with you. If not, please add a [WIP] (for work in progress) in front of the PR title - maintainers should usually know that they shouldn't merge those PRs.

BurnySc2 commented 3 years ago

1) I found a "bug" where if you remove an item after optimizing the build order by workers, the "selection" drops one to the left So from image it goes to image while the position of the "selection" shouldn't change. However I am currently unable to reproduce the bug.

2) A suggestion would be to hide the "Optimize" drop-down for races that are not working at the moment (so terran right now). If you can't add it, I could add it later - I won't have much time until monday evening.

pikiou commented 3 years ago

1) I think it was happening before from my commits. Here is an example on https://burnysc2.github.io/sc2-planner/?&race=protoss&bo=002eJyLrlbKTFGyMjPQUSqpLEhVslIqLikqTS4pLUpVqtWBShrhkTS1xCNpYQKXLM8vyk4tIkLG3BguU5qXWUKRWSTJxAIA32hVIA== If you place the selection say after the nexus and you double click on the zealot, hence removing the zealot and the following probe, it will move the selection one square to the left. Screenshot from 2021-05-07 02-14-42 Or are you referring to another bug?

2) I didn't know my commits were added to this pull request, so there are multiple features in this one but from now on I'll create branches to prevent this. Sorry! The features are :

I'll try to maintain this gh page up to date so you can test out these features before merging them.

pikiou commented 3 years ago

This is ready to be pulled (do I need to comment about it or changing the pull title was sufficient?)

BurnySc2 commented 3 years ago

If you place the selection say after the nexus and you double click on the zealot, hence removing the zealot and the following probe, it will move the selection one square to the left.

Yes, exactly - very weird 'bug'. I'll try to check out what causes it or how to fix it.

I'll try to maintain this gh page up to date so you can test out these features before merging them.

I'll pull the PR and npm run start it on my end to try it out anyway - thanks though!

Edit:

when adding an item to the build order, it also adds all its requirements (it doesn't work for terran but I'm working on it)

Doesn't seem to work when you want to add build order items in the middle of it. For example you start out with a pylon https://pikiou.github.io/sc2-planner/?&race=protoss&bo=002eJyLrlbKTFGyMjPQUSqpLEhVslIqLikqTS4pLUpVqo0FAJDqCgM= and then move the 'selection' to the start, and then build a tempest. You are really trying to implement a huge feature, in my opinion.

by default all the build order is visible, and a button allows to limit it to one scrollable line.

I guess that's a nice feature. I don't know if I'm a big fan of it though.

Edit2: If I was working on the sc2 planner a bit more, I'd probably prefer to work on the correctness of the build order (timing for example), which involves improving the income calculation or assigning workers per base, or even simulating them moving between bases. Currently, sc2 planner over-estimates the income, so if you'd play the build order you would be 10-30 seconds behind on a 4 or 5 minute build order.

pikiou commented 3 years ago

The performance issue mostly came from JSON.stringify being called about 100000 times per bo simulation as an error message detail in the idleUnits loop from trainUnit() in GameLogic That was hell to spot.

Indeed the auto-add requirements feature only works if you add something at the end of the build. It could work everywhere now that I understand the project, I'll have a look at the possibilities but it's less exciting than new features I'm working on...

All the builds I created were on one base, I didn't notice problems regarding timings. My next builds will be on 2 bases I might need accurate income estimations! So I might fix it before you do :P

Edit: Auto-adding requirements can work in the middle of a bo, just remove the insertIndex === bo.length - 1 condition in gamelogic.ts Problem is, it can get ugly. It will screw up build orders that users meticulously planned if they happen to misclick, e.g. on an add action button instead of an add building button with the same icon >< So I'd advise against it, but it's your call.

pikiou commented 3 years ago

I have a few pull requests waiting for this one to be merged. You may have a lot on your plate already, or maybe it's not good enough >< Again I'm new to github maybe should I just wait. Please don't hesitate to tell me how things are done around here !

BurnySc2 commented 3 years ago

Sorry, it's good. I just need a couple more days to merge it