xlenstra
commented
2 years ago Closed Caballero-Arepa closed 2 years ago
xlenstra
commented
2 years ago 
SomeTroglodyte
commented
2 years ago Well, the code looks a bit unfinished. There's at least one expression evaluated without side effects or storing the results... Blame says Yair's work 14 months ago. Might have regressed in that time. Still, I would doubt at least one design decision: Which tile was selected is stored as improvementInProgress with turnsToImprovement==-1 and no direct association with the construction queue entry. So, even with the crash fixed, I'd expect weird behaviour when a Worker lands on such a tile, or when queueing several buildings that create a farm and then choosing the same tile for all of them and so on.
Is this reproducible with the public+current version of Civilization 6 Mod or would one need a manual modification, as the OP seems to suggest?
Caballero-Arepa
commented
2 years ago Is this reproducible with the public+current version of Civilization 6 Mod or would one need a manual modification, as the OP seems to suggest?
The test district is in the public ver. of Civ 6 mod, but is currently blacked out. So you need to just remove the /* and load the game.
Or you could copy-paste this:
{ "name": "Test District", "cost": 1, "hurryCostModifier": -100, "maintenance": 0, "uniques": ["Creates a [Farm] improvement on a specific tile"] },
SomeTroglodyte
commented
2 years ago public ver
Does not show in mod manager??? This one I assume - but it's not playable at all due to too many modcheck failures (from newgame - loading above game unsurprisingly crashes in TechManager.updateEra). That can't be 4.0.6 and the original Civilization 6 Mod. Meaning I'm no longer interested, amigo :colombia:
Caballero-Arepa
commented
2 years ago No, This one bruh
SomeTroglodyte
commented
2 years ago Then you should have said so - besides, the name differs from what the linked message says, so that's another hidden manipulation to cunfose oss
SomeTroglodyte
commented
2 years ago No, This one bruh
The savegame up above is not compatible with that mod, even after patching around the baseRuleset name -> Nation Brussels not found. Even after hacking all the city states to the different ones in your mod it fails - different natural wonders :cursing_face: . Have any better save to easily repro?
By the way - my working notes currently look like this:
xlenstra
commented
2 years ago Flyby: Made Citadel tile takeover a typed unique
Ah, I had fixed that as well in a local branch. I'll wait for your PR on this and work my thing around that
Caballero-Arepa
commented
2 years ago The savegame up above is not compatible with that mod, even after patching around the baseRuleset name -> Nation Brussels not found. Even after hacking all the city states to the different ones in your mod it fails - different natural wonders 🤬 . Have any better save to easily repro?
Oh, right, now the mod is different... let me get you another save with it's own mod.
What if building is bought?
Imo the improvement should be instantly placed as well
What if a city gets razed while owning a marked tile?
So, simple option: destroy the improvement if city is razed; Ideal, but complex option: Only destroy the imp. if it's unbuildable. If the imp. is a normal one (i.e: Fort) it doesn't get destroyed.
SomeTroglodyte
commented
2 years ago I said "my notes" so the question marks were addressed to me not you, all that was just to show it's far more complex to finish that feature than just fixing some mistake.
I'm 90% sure the code won't handle buying properly, but that's easier seen in the debugger than by code inspection. Which is no fun as the game slows to a crawl - 10s per click - with that mod on a small map, probably #6468 again. And your second hint - nope there is no improvement to destroy, but leaving the "marker"(s) would be an unrecoverable impediment, thus extra code is still needed to remove them. Same as with pillage and nuke, really, those would have destroyed the "marker" without cleaning the queue so a finished building would not place the imp. As I said, it's abusing turnsToImprovement = -1 as marker. I'm gradually becoming more attracted to changing the marker to a fresh field - but I already centralized that so it's just two places in code (two because: respect existing code regions).
... but gracias for paying attention!
Caballero-Arepa
commented
2 years ago Ok, here is another save, this is just with a simple specific mod: Test district.zip
In order:
**Thanks Xlenstra for the edits ;)
Caballero-Arepa
commented
2 years ago Platform: Desktop
Version: 4.0.9
Rulesets: [Policy mod, Victoria II Music, Warfare Expanded Unciv Bucketeer, RekMOD, Civilization 6 Mod, DeCiv Redux, 3rd and 4th Unique Component, Barbarian xp farm, TS Industrial v2, Latin American_Civs, More Luxuries, Tech Stopper Industrial, Default units, Difficulty_Sandbox, Civ6Nations, Test district, Civ V - Vanilla, Civ5Nations, Civ style set by Bucketeer, Resource Recyclers, Civ V Leader portraits, Dreams of Steam, mi tileset, Civ Army Color Style Sheet, Alpha Frontier, HexaRealmTest, Smoky Skies Redux, Civ V - Gods & Kings, Panamericana, Bubbly Borders Example, Closer Cities Distance of 1, 5Hex Tileset, Extra Resources]
Last Screen: com.unciv.ui.cityscreen.CityScreen
OS: Windows 10 Java: Oracle Corporation 25.232-b09 Max Memory: 910 MB
Message:
com.unciv.logic.city.CityConstructions$getConstruction$NotBuildingOrUnitException: Farm is not a building or a unit!
at com.unciv.logic.city.CityConstructions.getConstruction$core(CityConstructions.kt:232)
at com.unciv.logic.city.CityConstructions.addToQueue(CityConstructions.kt:562)
at com.unciv.ui.cityscreen.CityScreen$addTiles$1.invoke(CityScreen.kt:253)
at com.unciv.ui.cityscreen.CityScreen$addTiles$1.invoke(CityScreen.kt:244)
at com.unciv.ui.utils.ExtensionFunctionsKt$onClick$1.invoke(ExtensionFunctions.kt:78)
at com.unciv.ui.utils.ExtensionFunctionsKt$onClick$1.invoke(ExtensionFunctions.kt:78)
at com.unciv.ui.utils.ExtensionFunctionsKt$onClickEvent$1.clicked(ExtensionFunctions.kt:71)
at com.badlogic.gdx.scenes.scene2d.utils.ClickListener.touchUp(ClickListener.java:88)
at com.badlogic.gdx.scenes.scene2d.InputListener.handle(InputListener.java:71)
at com.badlogic.gdx.scenes.scene2d.Stage.touchUp(Stage.java:355)
at com.unciv.ui.crashhandling.CrashHandlingStage.access$touchUp$s80204510(CrashHandlingStage.kt:10)
at com.unciv.ui.crashhandling.CrashHandlingStage$touchUp$1.invoke(CrashHandlingStage.kt:24)
at com.unciv.ui.crashhandling.CrashHandlingStage$touchUp$1.invoke(CrashHandlingStage.kt:24)
at com.unciv.ui.utils.ExtensionFunctionsKt$wrapCrashHandling$1.invoke(ExtensionFunctions.kt:333)
at com.unciv.ui.crashhandling.CrashHandlingStage.touchUp(CrashHandlingStage.kt:24)
at com.badlogic.gdx.InputEventQueue.drain(InputEventQueue.java:70)
at com.badlogic.gdx.backends.lwjgl3.DefaultLwjgl3Input.update(DefaultLwjgl3Input.java:189)
at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Window.update(Lwjgl3Window.java:394)
at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.loop(Lwjgl3Application.java:143)
at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.<init>(Lwjgl3Application.java:116)
at com.unciv.app.desktop.DesktopLauncher.main(DesktopLauncher.kt:61)
Save Mods:
[Civ V - Gods & Kings, Test district]Save Data:
Caballero-Arepa
commented
2 years ago SomeTroglodyte
commented
2 years ago NotBuildingOrUnitException
Come on. Ooooold and boring and mouldy and buried in dust. Plain to see why here :stuck_out_tongue_winking_eye: - yes go on, read that line, it tries to add an improvement name to the city construction queue - so just name your building and its improvement the same! Problem solved! :roll_eyes: :laughing:
SomeTroglodyte
commented
2 years ago Progress, see checklist above. Now I'm out of ideas - or, to be precise, there were ideas to improve the code in my nightmares, and I forgot to jot them down.
That means testing. @ArchDuque-Pancake , I declare you testing manager. Please install Android Studio, and go ahead check out my branch and test. :wink: Seriously, is that within your scope or would you like to be able to test with a jar or apk? My tests were on a fresh game.
Some of these may be quite hard to set up a test situation for, I know. Do your best.
ALL others daring to help are of course welcome to try that branch out. I'm done with this for the moment, far too costly already, and may not react to questions for a day or two, but this won't be forgotten.
Caballero-Arepa
commented
2 years ago Alrighty, I can do it with Android Studio, but if I could get the .jar it could speed up the testing, it would help me, but don't worry if you don't want to provide it.
Anyways, thank you so much for your work on beforehand, testing will be started.
SomeTroglodyte
commented
2 years ago get the .jar
here, for a limited time. Why and when has it become so bloated?
Caballero-Arepa
commented
2 years ago Caballero-Arepa
commented
2 years ago Testing results:
All marked above are successful.
Bugs:
SomeTroglodyte
commented
2 years ago can be placed on top of Natural wonders or Mountains
By manual choice or by automatics? How could I repro? Should only be possible if the TileInfo.canBuildImprovement function says yes, that's not changed by me AFAIK.
Cannot pillage
Well of course that would only be possible if there's something to pillage??? The "under construction" "marker" doesn't count as such, but it would be incompatible with what pillaging usually does. By the way, I have no idea what the current status on pillaging roads is... And nukes don't "pseudo-pillage' all possible tiles, there's the random factor before that. When a nuke treats a non-improved tile there can't be a conflict with the "under construction" "marker", so that should be absolutely unchanged by this branch. There's no other options - either the marker protects from pillage+nuke, or pillage+nuke can remove an improvement (removing marker too) while also unqueueing the building, or additional data in save games & memory. I chose option two...
The important part - either the building in the queue and the "under construction" "marker" are both still there or both gone. Unsure how to test that myself - maybe hotseat multiplayer?
I noticed the improvement may significantly reduce the tile yield, and still that tile would be auto-chosen - if it's the only feasible one. Upstream, the AI has no way to know the actual "value" (as in AI measure of benefit) of the building, as the "value" of the improvement can't be taken into account, and even more extra code for that - sorry, I'm too lazy. I did the bare minimum to automation (I believe) to avoid conflicts, AI is not really my thing.
SomeTroglodyte
commented
2 years ago Testing results
Respect! I couldn't have done it in that time (well, I also had RL taking precedence for a while). Muchas Gracias.
SomeTroglodyte
commented
2 years ago can be placed on top of Natural wonders or Mountains
When I try - those have a red circle, and clicking that just stops the "ask for tile" thing, the building stays unqueued/unbought. As I envisioned the UI should behave (or would you rather it continue the process and ignore the red-tile-click?). If you get green for a mountain - a mod thing?
SomeTroglodyte
commented
2 years ago ... but I noticed a missing updateTileGroups call right after manually placing the target marker ... How did that happen? I fought hard earlier to make the mark visible when supposed to...
Caballero-Arepa
commented
2 years ago By manual choice or by automatics? How could I repro?
Both, I could do it, and AI as well. I was using an Academy as the improvement.
Test district.zip
Note that improvements that explicitly say that can't be build somewhere, like moai, can't be placed aywhere. So the problem is only with improvements that don't have any restrictions to be placed on mauntains or wonders.
The important part - either the building in the queue and the "under construction" "marker" are both still there or both gone.
Both gone when there is something to pillage.
Unsure how to test that myself
I do it by setting two nations as humans, no need for multiplayer.
SomeTroglodyte
commented
2 years ago ... I think I should tweak the color when tiles are technically allowed but choosing them would be a disadvantage. I have yellow if it would stop a Worker construction in progress, but not it it would kill a valuable Improvement. (My Civ6 mod copy doesn't mark Districts as Irremovable so that could be a big difference when that "Test District" places a Farm on it). I wouldn't really like to code an AI value comparison to control colors showing desirability, however. Yellow too if there is an improvement of any kind (exl roads) there....
Academy
Ah, that is maybe normally guarded against being plonked onto mountains only by the unit not being able to enter??? Valuable hint. -> Yes, TileInfo.L529 improvement.terrainsCanBeBuiltOn.isEmpty() && improvement.turnsToBuild == 0 && isLand -> true - not sure how to solve. adding a && !isImpassible() test would do it - but that feels like hardcoding and limiting combinations that a modder might otherwise get to work.
Caballero-Arepa
commented
2 years ago I think I should tweak the color when tiles
Yeah, they are a bit hard to spot. I did noticed the yellow dot and understood what it meant, I like it.
guarded against being plonked onto mountains only by the unit not being able to enter???
Yes, altough now that I think about it... it doesn't really matter, as the unique will only be only used in mods, so the modder should add "cannot be placed in []" if he doesn't want that to happen.
SomeTroglodyte
commented
2 years ago so the modder should
Happy to hear. But - needs dox. Where would that go...?
Caballero-Arepa
commented
2 years ago In the improvement json of course. For the case of Civ6 mod, districts are new so no problem there. *I did a test with Moais instead and in fact it didn't allow me to add them without coasts.
SomeTroglodyte
commented
2 years ago a bit hard to spot.
Still? I upped the alpha a bit, and thought it a big improvement. Don't want to hide everything on that tile. Then again, you could gain insight into which tile might be best by looking before triggering the please choose tile.
In the improvement json of course
No, the modder using the Unique themed here should find such special considerations in the modding wiki, and the problem is - auto generated file, no provision for extra text. Dox to help the modder find your solution, not dox left by the modder as to why his mod changes the Academy this way...
Caballero-Arepa
commented
2 years ago you could gain insight into which tile
Yeah, personally it's not a big deal, as it's mostly just on grassland tiles. And, the most important dots are the red and yellow ones, which are easy to see. So no need to change it if it was by me, I said it because other players might not see them, idk.
should find such special considerations in the modding wiki
I don't think it would be that hard to realise a fix for [my improvement] being able to be placed in a tile I don't want. Altough to be honest I dont quite understand fully your frase there... either way, I say leave it as it is, just let the modder decide where it can or cant be placed, idk.
SomeTroglodyte
commented
2 years ago What I said earleir "a missing updateTileGroups call" was wrong - that highlight I was missing only shows when the queue entry is selected, as intended. So, that leaves us with only cosmetic changes so far? I can update the jar easier than the PR/online branch as I had to merge another branch to even be able to fluidly test (which is why it shrank by 30%), and I don't want to mix. That jar changes the "please select tile" highlights only. Sources later.
SomeTroglodyte
commented
2 years ago It just came to me this is intended to replace the District Builder Units in Civ6 mods -> But no matter how, they either need protection or some (nonexisting) feature to refund the ability to grow to the allotted number of districts - if the builder is killed, the improvement nuked away (which can either place the "repair" state or destroy outright depending on uniques, and a repair state can be removed), - once you're one short you stay one short.... Not scope of that PR, but needs thought.
Trying to use the unique of "Creates a [tileFilter] improvement on a specific tile", and the game reads it, but upon clicking one of the green circles the game crashes.
To Reproduce
"Creates a [Farm] improvement on a specific tile"Crash Log
(Unciv discord server, bugs channel) https://discord.com/channels/586194543280390151/588357826758574106/966057960067432468
Expected behavior Game shouldn't crash obviously, and it should then place the improvement on the selected tile when the building finishes.