Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a default unitType for units to gain global uniques #12690

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SeventhM
Copy link
Collaborator

Currently, there's no way to have global uniques for mods. At best, you have to hand out promotions to units that give whatever global effects you want. While this technically works, not only does it require the promotion system to always handle out the unit (which I think is always doable, but it's possible there's some edge case I'm not aware of), but some modders don't seem to be aware that this is an option. As such, you get mods that have extremely ugly/ likely unmaintable UnitTypes files1. Pretty sure this comes up occansionally as a mod request2

Footnotes

  1. https://github.com/Jiangeshi/Leader-Mission-2-Rising-Power/blob/main/jsons/UnitTypes.json

  2. https://github.com/yairm210/Unciv/issues/3242#issuecomment-1813988667

@yairm210
Copy link
Owner

This is not the correct way to go about this. This adds an extra lookup every time we try to resolve uniques.
Instead, the "all" uniques should be loaded into the uniquemap of each unit type - we pay memory to get speed, that's definitely the right tradeoff

Also, this feature ("all" unittype) is also currently undocumented, so as for "modders aren't aware this is an option" - let's document it in .md files

@SeventhM
Copy link
Collaborator Author

Instead, the "all" uniques should be loaded into the uniquemap of each unit type

🤔Hmm... Might need to look into this.

This is not the correct way to go about this.

Tbh, I agree. Even ignoring the performance impact, tbh I think this solution is kinda ugly. I guess a better solution would actually be to put these uniques in GlobalUniques as var unitUniques = ArrayList<String>(). Was just not sure a good spot to put the uniques that didn't break the mod structure or introduce an unnecessary extra json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants