search

yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.32k stars 1.56k forks source link

City State Unit Gifts pull from the recipient's Unit Pool #3967

Closed TheRexYo closed 3 years ago

TheRexYo commented 3 years ago

Recently, I noticed that City States that gift players units give units using the construction list of the friendly player's nearest city. This breaks immersion when the city state gifts a unit that they can't build on their own, and also limits the options of modders. In particular, when you give a city state a unique unit in a mod, it will not be gifted to players since the AI will pull from the player's pool.

AI gifts should always pull from the creator's pool and not the recipient's. Of course, this would cause issues when the AI tries to gift a unit that the recipient cannot hold in any cities, but there's an easy workaround for that - just iterate through all possible units that COULD be gifted (in other words those that the AI can build) and select the first one that the player can actually receive. If none are found, message the player to tell them that the AI tried to gift them a unit, but failed, or perhaps just default to the old method instead when such a thing occurs.

Considering that this might not be vanilla Civ 5 behavior, perhaps a "giftsUseCreatorsPool" boolean option in ModOptions.json could be added to allow the new changes to be enabled on a per-mod basis rather than being the default option.

I'd being willing to take this project on myself, but only if there's no problems with the idea that might make it a poor candidate for merging. In other words, if you have access to Pull Request merging (that means you, Yairm), please inform me of your thoughts on the matter.

SomeTroglodyte commented 3 years ago

might not be vanilla Civ 5 behavior

Might be worth checking - or is it on the sticky already? No doesn't seem so... Hey here's a ton ov Civ5 behaviour we haven't implemented yet! Sorry for the joke. Well, this certainly sounds like you are right, even if not stated obviously. Plus the unique unit bit which afaik we also do not yet have.

SomeTroglodyte commented 3 years ago

Oh and if you just start out with the ecosystem you surely know Yair's wiki, but there's also my notes which I collected to better remember stumbling points if you want. You'll want to start looking at CivilizationInfo.giftMilitaryUnitTo() near line 634... Ooops, not in a dedicated city-state manager? Sounds like a refactoring project...

TheRexYo commented 3 years ago

Thanks for the info. Tbh, I wasn't referring to the whole "at the start of the game, each city state is assigned a unique unit from a civilization that isn't in the current save" bit. Rather, I literally meant that when you add a unique unit (using the "uniqueTo" flag) to a city-state, it will never be gifted, as at current Unciv just kinda takes the nearest city belonging to the recipient and gives a random unit from that city's construction list. Which doesn't make sense since it's supposed to be the city state giving you one of their units. I'd rather not play a game and be like "how did that guy get access to my Jaguar Warriors? He's not even Aztec!" every time I get gifted a unit.

TheRexYo commented 3 years ago

I think I already made a demo for this awhile back, but because my mouse broke, I was unable to test it. I posted it here because, like I said before, I need to figure out if the idea is actually likely to get merged if I actually get it working.

TheRexYo commented 3 years ago

You'll want to start looking at CivilizationInfo.giftMilitaryUnitTo() near line 634... Ooops, not in a dedicated city-state manager? Sounds like a refactoring project...

Well, it works just fine as-is. The problem is the way that the function goes about selecting the unit. That's the function that I read when I was trying to figure out how exactly city-state military gifts work. Before reading the fandom article you sent me, I had no idea how vanilla Civ 5's unit gifts worked because, back when I played it, I rarely paid attention to city states in the first place lol. Which is why I originally said that it should be a mod options thing, but if it's supposed to be in-game and just hasn't been implemented, I can totally avoid adding a json config for that.

SomeTroglodyte commented 3 years ago

Well, you seem to be doing just fine...:+1:

ravignir commented 3 years ago

Thanks for the info. Tbh, I wasn't referring to the whole "at the start of the game, each city state is assigned a unique unit from a civilization that isn't in the current save" bit. Rather, I literally meant that when you add a unique unit (using the "uniqueTo" flag) to a city-state, it will never be gifted, as at current Unciv just kinda takes the nearest city belonging to the recipient and gives a random unit from that city's construction list. Which doesn't make sense since it's supposed to be the city state giving you one of their units. I'd rather not play a game and be like "how did that guy get access to my Jaguar Warriors? He's not even Aztec!" every time I get gifted a unit.

but that's how it works in civ5. Friendly CS give non-unique units that can be build by the receiving civ (they must have the right tech) and CS also have a random unit they specialize in (borrowed from civ that's not in the game) - they will provide it when allied civ has the right tech - to unlock it and not to make it obsolete.

SomeTroglodyte commented 3 years ago

Well, how it is and how CIv5 does it is clear enough by now. To sum up, to get CS behaviour same as the original several things are needed:

TheRexYo commented 3 years ago

If I can interject, Vanilla Civ 5 behavior does indeed allow you to be gifted units that you can't build (for example, if they use a tech you haven't researched). As the wiki also states, they can even gift you their unique units. So here's my solution. Have it select units from the City State's city (and thus follow vanilla behavior). This is in fact the simplest fix for the issue, and the best one, since it allows both the City State unique unit and any uniqueTo units to be gifted. All that would need to be added after that would be the assignment of the extra unique unit, which I probably won't be handling. Also, don't lock modders into using vanilla behavior if you don't have to. Making it so that it selects from the receiving nation's unit pool is both arbitrary and non-vanilla.

On Tue, May 25, 2021, 2:33 PM SomeTroglodyte @.***> wrote:

Well, how it is and how CIv5 does it is clear enough by now. To sum up, to get CS behaviour same as the original several things are needed:

  • Randomly assigning unit uniques to CS: New code, new persistent data
  • CS gets units from another civ, not buildings as I read it, so you can't simply overwrite their nation internally and just set a flag forbidding settlers.
  • New code allowing CS to build its uniques for own ends
  • Amend giftMilitaryUnitTo so it uses the getConstructableUnits from the other city
  • Refactor the tech level check - the existing getRejectionReasons does some tests that should not apply in the CS gifting units case.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yairm210/Unciv/issues/3967#issuecomment-848205136, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOB2TVIQPOUYBVEG37RU5GDTPP3RVANCNFSM45FPNRRA .

TheRexYo commented 3 years ago

Also, no. I can confirm that City States do get access to units as any other nation does. They do research and everything, according to the wiki, and while they are often at the same degree of technology as players at any given point, they do not copy the their buildables from civs. Where did you get that information? The only thing that is assigned to them is their unique unit, so just have them iterate through all available civs and pick one, then store that in a second field "countsAs" (useable with nations). Whenever the function that checks for whether a unit has "uniqueTo" is called, have it use an |OR| operator that checks for either "uniqueTo" or "countsAs".

On Tue, May 25, 2021, 2:33 PM SomeTroglodyte @.***> wrote:

Well, how it is and how CIv5 does it is clear enough by now. To sum up, to get CS behaviour same as the original several things are needed:

  • Randomly assigning unit uniques to CS: New code, new persistent data
  • CS gets units from another civ, not buildings as I read it, so you can't simply overwrite their nation internally and just set a flag forbidding settlers.
  • New code allowing CS to build its uniques for own ends
  • Amend giftMilitaryUnitTo so it uses the getConstructableUnits from the other city
  • Refactor the tech level check - the existing getRejectionReasons does some tests that should not apply in the CS gifting units case.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yairm210/Unciv/issues/3967#issuecomment-848205136, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOB2TVIQPOUYBVEG37RU5GDTPP3RVANCNFSM45FPNRRA .

TheRexYo commented 3 years ago

Also, again, I'd be willing to program this, the only thing that's stopping me is the fear that it won't get accepted and I'd have wasted my time coding it. If you guys think it isn't worth adding, for example, I wouldn't code it, but if you think it'd be difficult to add it, I don't really mind and I'll do it anyway.

On Tue, May 25, 2021, 3:09 PM TheRexYo Gaming @.***> wrote:

Also, no. I can confirm that City States do get access to units as any other nation does. They do research and everything, according to the wiki, and while they are often at the same degree of technology as players at any given point, they do not copy the their buildables from civs. Where did you get that information? The only thing that is assigned to them is their unique unit, so just have them iterate through all available civs and pick one, then store that in a second field "countsAs" (useable with nations). Whenever the function that checks for whether a unit has "uniqueTo" is called, have it use an |OR| operator that checks for either "uniqueTo" or "countsAs".

On Tue, May 25, 2021, 2:33 PM SomeTroglodyte @.***> wrote:

Well, how it is and how CIv5 does it is clear enough by now. To sum up, to get CS behaviour same as the original several things are needed:

  • Randomly assigning unit uniques to CS: New code, new persistent data - CS gets units from another civ, not buildings as I read it, so you can't simply overwrite their nation internally and just set a flag forbidding settlers.
  • New code allowing CS to build its uniques for own ends
  • Amend giftMilitaryUnitTo so it uses the getConstructableUnits from the other city
  • Refactor the tech level check - the existing getRejectionReasons does some tests that should not apply in the CS gifting units case.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yairm210/Unciv/issues/3967#issuecomment-848205136, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOB2TVIQPOUYBVEG37RU5GDTPP3RVANCNFSM45FPNRRA .

SomeTroglodyte commented 3 years ago

get that information

From Unciv code. It gifts what the city the gift is placed near would be able to build, as you said when you opened the issue. The CS itself builds from the vanilla ruleset w/ zero nation specials. Right now. As you said, too.

Fear? Wrong, mate. Happy to see help, always. Your code will be reviewed and tested, so worst case - we'll all learn.

Here's the Wiki link again just in case. There's some more details that work differently in Unciv, though whether they warrant code change needs application of common sense. e.g. "They do start with a Settler, who founds the city state on the same tile it spawned at the beginning of the game" - not so in Unciv, you can sometimes watch them wander, and checking an early save before they get assimilated shows turnAcquired>1... I'd say no effect on fun, not worth it. There's more differences... ... ...

Wait a minute - The unique unit per CS is a Gods&Kings update! Not Unciv's goal... I'll trust you to pick the worthy changes.

ravignir commented 3 years ago

Well, units having 100 HP, Gatling guns and many others are also Gods&Kings.

ravignir commented 3 years ago

If I can interject, Vanilla Civ 5 behavior does indeed allow you to be gifted units that you can't build (for example, if they use a tech you haven't researched). As the wiki also states, they can even gift you their unique units. So here's my solution. Have it select units from the City State's city (and thus follow vanilla behavior). This is in fact the simplest fix for the issue, and the best one, since it allows both the City State unique unit and any uniqueTo units to be gifted. All that would need to be added after that would be the assignment of the extra unique unit, which I probably won't be handling. Also, don't lock modders into using vanilla behavior if you don't have to. Making it so that it selects from the receiving nation's unit pool is both arbitrary and non-vanilla. … On Tue, May 25, 2021, 2:33 PM SomeTroglodyte @.***> wrote: Well, how it is and how CIv5 does it is clear enough by now. To sum up, to get CS behaviour same as the original several things are needed: - Randomly assigning unit uniques to CS: New code, new persistent data - CS gets units from another civ, not buildings as I read it, so you can't simply overwrite their nation internally and just set a flag forbidding settlers. - New code allowing CS to build its uniques for own ends - Amend giftMilitaryUnitTo so it uses the getConstructableUnits from the other city - Refactor the tech level check - the existing getRejectionReasons does some tests that should not apply in the CS gifting units case. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3967 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOB2TVIQPOUYBVEG37RU5GDTPP3RVANCNFSM45FPNRRA .

just tested it in vanilla civ5 and they work exactly as i described.

ravignir commented 3 years ago

even worse, because the unit spawns in city state territory. I used IGE to make CS have all techs (can be seen by their combat strength). image i have zero techs so i received a warrior.

ravignir commented 3 years ago

another thing: City states tech is always at least on the same level as half of the civs in the game. If half of the major civs have Machinery tech, all of the city states will also have it.

yairm210 commented 3 years ago

For now, I'm disallowing CSs to grant you unique units. That should be enough for the moment.

TheRexYo commented 3 years ago

another thing: City states tech is always at least on the same level as half of the civs in the game. If half of the major civs have Machinery tech, all of the city states will also have it.

Are you saying that they do both (meaning they get free techs and research techs), or that they don't research techs at all? If the latter, read the wiki article posted in this issue; they do in fact research techs.

TheRexYo commented 3 years ago

For now, I'm disallowing CSs to grant you unique units. That should be enough for the moment.

When did this issue become "City States grant you their unique units"? That's what I'm trying to get in the first place! XD

TheRexYo commented 3 years ago

Anyhow, I will start working on this today. If possible, I'm going to implement a modoptions config that allows you to select the behavior that the game will use when gifting units (as in, will it gift from the city state's pool or the recipient's pool). Some time soon, I'll be adding the vanilla Civ 5 behavior where it selects a random unused nation's unique unit and assigns it to the city state at the start of the game. After that, I plan on adding a unique for nations (ie. City States) that allows them to ignore the random unit assignment so that they don't get a random nation's unit (for use by modders), as well as a unique for units (namely those that have a "uniqueTo" flag) that prevents them from getting assigned as a random city-state unique unit.

TheRexYo commented 3 years ago

Again, the overall objective of this issue was not to instate vanilla Civ 5 behavior; I even said in my original post that it doesn't follow vanilla behavior, where I suggested that it should be a modOptions thing so that the changes won't impact the base ruleset. Long story short, I came here trying to figure out whether or not non-vanilla features are allowed as long as they aren't enabled by default. I didn't want to spend hours working on a new feature for mods only for it to get turned down because it isn't vanilla. So I basically came to ask what the protocol is for non-vanilla features. If they aren't vanilla, I know they aren't going to be actively worked on, but what you never specified in your feature requests section is whether or not such a feature can be added at all (for example, if someone else - me - did all the work).

TheRexYo commented 3 years ago

For now, I'm disallowing CSs to grant you unique units. That should be enough for the moment.

Can you open this issue again? The fix you added was a fix for the things that other people's comments were asking for, not the issue itself. The real issue (see my recent posts) has not been fixed. I will be working on it myself, so you could also technically remove the issue as well (since it would not only be incorrect to state the issue as fixed, but also to open it again, since the issue isn't really a feature request anymore since I'm working on it).

SomeTroglodyte commented 3 years ago

Don't be discouraged. I can't speak for the boss, but code gets rejected because it breaks things, or because it adds little 'value' (which is a bit but not 100% measured by faithfulness to Civ5) while at the same time introducing high risk of issues (e.g. high complexity in stuff that runs all the time)... I think with this subject chances are good to both bring vanilla Unciv closer to its 'idol' and to keep the 'risk' factor low (modularize, maybe separate complex stuff behind a clear interface that can be nailed down with a unit test or such and/or make complex stuff configurable enough so potential bugs won't 'activate' in the vanilla ruleset).

If we can help - I for one am still listening here.

ajustsomebody commented 3 years ago

@yairm210 what will happen to this thread? check #4602 about this issue as well please