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

Update ruins notification to show exact amount of culture #12784

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SpacedOutChicken
Copy link
Contributor

Now that the ruins reward granting culture adjusts to game speed, it no longer necessarily grants 20 culture. This PR changes the notification to reflect this fact.

Update notification of ruins to allow for displaying of exact amount of culture granted, once modified by game speed
Update Vanilla branch as well as G&K branch to make ruins culture notification correct
@yairm210
Copy link
Owner

This won't work, since we don't inject the amount into the notification... And starting to inject trigger data into the notification sounds like a nightmare
Our options are to either roll back this change, OR to have "+20 culture" be incorrect in some speeds, OR to delete the amount of culture in the notification entirely

@SpacedOutChicken
Copy link
Contributor Author

I know that in my own Deciv 2 mod, I have a notification that uses [cultureAmount] Culture in it, and it seems to be working. The ruins grants a variable amount of culture ([20]-[30]) and the notification appears to be providing the exact amount of culture being granted. Is something else going on with the notification that Deciv is using? Like I said, it seems to be working for that mod. You can see it here: https://github.com/SpacedOutChicken/Deciv-2/blob/main/jsons/Ruins.json

@yairm210
Copy link
Owner

You're right, we do have a provision for similar changes, I forgot
So it won't be a huge change, but will be a change, since it's unique specific IIRC

@SomeTroglodyte
Copy link
Collaborator

do have a provision

For reference: That one is here:

notification.fillPlaceholders(statAmount.tr())

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.

3 participants