Closed SomeTroglodyte closed 5 months ago
CreateWaterImprovements
But aren't civilians hardcored to only be able to construct? (observation not code analysis)
Being able to convert a military unit to civilian, or vice versa has mod value. If that is possible, I wouldn't "fix" it unless actual problems are resulting.
But aren't civilians hardcored to only be able to construct?
I'm not sure where this assumption comes from
- Isn't
if (!alreadyHasWorkBoat) return
the wrong way 'round????????
It's the right way 'round, but the flag's name is the wrong way 'round, both in and out. It's on if the ruleset has some workboat unit and none was found with a limited BFS. So it's correct to bail if it's off.
I still have to add another point to the list above...
Demo for that last point:
Uh... Did you upload the wrong image?
Thanks. Fixed. Gimp and its virtual layers for text that you never know how to finalize - flatten does it but it's not always evident...
Anyway - any coder welcome to eval this patch:
... started out as attempt to fix #10258, which it does, but with roughly 90% certainty the wrong way. Testing led to looking at ConstructionAutomation.addWorkBoatChoice again... Patch also only addresses the last points.
started out as attempt to fix #10258
Ugh... Wait, I'm knee deep checking something else rn, but why is this in constructIfEnough
? Seems to me free buildings from policies will come along and trample over this anyways because I added the call to validate the construction queue (in case you get a free building that was building, which was a missed bug before to my knowledge). Shouldn't this be in addBuilding
?
Yeah exactly - I grok'ed that the validate the construction queue call might drive around the automation call from remove, but haven't really got a clue how the code flow really is and how it should be...
Anyway, that one line is the kludge that allowed to test the AI, and it threw me that it insisted on useless workboats. That patch is still far from good, it probably needs to make that BFS supply scope for both the need test and the 'already covered' test - I just saw it automate a WB for a tile where the automation already had just finished one in the closer city. Locked closed seas, no chance to mistake, there was only the one unimproved tile in the entire ocean.
but haven't really got a clue how the code flow really is and how it should be...
Doing some quick searches, chooseNextConstruction
is called in CityConstuctionTable.purchaseConstruction
and CityConstuctions.removeFromQueue
. It seems obvious that it's supposed to be called at the start of a turn to decide if it still makes sense (ideally before stuff is calculated to have the same information as a player) and after anything changes the calculation mid turn (through something being built). Seems to me that if you put it at the bottom of addBuilding, it'll cover these situations as intended with even less side effects (removeFromQueue is called before validating anyways in constructionComplete, so it never actually worked as intended and purchaseConstrcution checks for the improvement set for some reason when I swear chooseNextConstruction already dealt with...)
Actually, maybe also put it in BaseUnit.postBuildEvent to keep correct functionality
Edit: I'm dumb, that's still before validation. It would need to be in the validation code tbh
Well, if you think you have a clear "big picture".... Me, I'll pause after a good git gc --prune=now --aggressive
Bugs and other bugs
Applicatio@n
This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.
Questions about
ConstructionAutomation.addWorkBoatChoice
:alreadyHasWorkBoat
doesn't test whether it's your WorkBoat....baseunit in buildableWorkboatUnits.toSet
could be much faster...if (!alreadyHasWorkBoat) return
the wrong way 'round????????~