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

CvTraitInfo: tag CityExtraYields is not used correctly #459

Closed
Nightinggale opened this issue May 15, 2021 · 4 comments
Closed

CvTraitInfo: tag CityExtraYields is not used correctly #459

Nightinggale opened this issue May 15, 2021 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@Nightinggale
Copy link
Contributor

CvTraitInfo's tag CityExtraYields lacks a proper implementation. It seems the bonus is never added, or rather maybe it is, but then the cache isn't updated... not sure. Either way getting a trait through an FF (discovered with Ivan Aleksandrovich Kuskov) doesn't work as expected.


I propose scrapping the implementation and redo it with strict typing in CivEffects. This way anything providing CivEffects can get it.


Perhaps it would be best to introduce some sort of "city promotion". This will create cleaner and reusable code (less risk of bugs) and quite possibly allow more aggressive caching in CvCity, making the game faster. It could also be used for more detailed GUI regarding how colonies are affected.

@Nightinggale Nightinggale added the bug Something isn't working label May 15, 2021
@Nightinggale Nightinggale self-assigned this May 15, 2021
@raystuttgart
Copy link
Collaborator

raystuttgart commented May 15, 2021

This was definitely working correctly in previous releases and I think it still is in "Large Rivers".
I tested it ingame and it worked fine, when I implemented it.
(But I will check again to be sure.)

My current suspicion:
It must have been broken in 2.8.2.x ...

But again:
I will verify.

@Nightinggale
Copy link
Contributor Author

Nightinggale commented May 15, 2021

I got it.

CvPlayer::updateCityExtraYield needs to be called to update the cache. However CvPlayer::processTrait calls CvPlayer::updateYield without using CvPlayer::updateCityExtraYield, hence it doesn't update the cache.

The calls looks like an awful waste of CPU cycles. For each yield, which changes, all traits are looped and all plots are recalculated for for each yield. Same is true for extraYieldThreshold. Also if either one of them changes anything, for good measure all plots recalculate all yields once done.

Looks like this is vanilla code.

I'm going to fix this, but it will be a quick fix, which ignores performance. It's not like it will be worse than the current performance anyway.

@raystuttgart
Copy link
Collaborator

Ok just checked.
(Using Cheat Menu for Founding Fathers)

I was wrong:
It is already broken in Large Rivers

By the way:
Might have been broken already since TAC or even Vanilla.
(The only other traits that use such a modifier are given to Natives - so maybe nobdy ever noticed ...)

Summary:
This is definitely not a new bug of 2.8.2.x
No idea why I thought it was working and never noticed ingame ...

Nightinggale added a commit that referenced this issue May 15, 2021
…rrectly

Vanilla bug.
The issue was CvPlayer::processTrait did not update the cache in CvPlayer.
Instead it updated the cache in CvPlot, which relies on the CvPlayer cache being set.
@raystuttgart
Copy link
Collaborator

I confirm as well.
(Just also tested.)

Working nicely. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants