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

Need ASSERT checks for array memory #11391

Open
RecursiveVision opened this issue Dec 7, 2024 · 1 comment
Open

Need ASSERT checks for array memory #11391

RecursiveVision opened this issue Dec 7, 2024 · 1 comment

Comments

@RecursiveVision
Copy link
Collaborator

RecursiveVision commented Dec 7, 2024

After adding ASSERT checks to Diplo AI memory, I was confronted with a very hard to catch bug where approaches towards player 0 would be randomly replaced with huge negative or positive numbers. Even player 0's approach scores towards themselves, which shouldn't ever be updated.

Turns out SetCivApproach() was setting -1 for m_aeCachedSurfaceApproach[MAX_MAJOR_CIVS] (the array right before the approach scores) for every City-State. Because this went beyond the bounds of the array, it would randomly overwrite approach scores towards human players with garbage data, resulting in the AI being extremely passive or aggressive for no reason.

This was a silent failure for probably several versions now. I was unaware there's no out-of-bounds exception in C++, and there may be other instances of this in the code somewhere. We need assert checks for all arrays to make sure the index is in bounds!

@JohnsterID
Copy link
Contributor

JohnsterID commented Dec 7, 2024

There are quite of few CvAsserts for out of bounds, follow this pattern?
https://github.com/LoneGazebo/Community-Patch-DLL/blob/master/CvGameCoreDLL_Expansion2/CvBeliefClasses.cpp#L545-L550

edit: Difference that I can see between CvAsserts, focused on Civ 5 game debugging and FAsserts, more generalized asserts.
Past comment on the FAssert work: #9155 - CvAssert vs FAssert in reference to "Invariants" noting we have working CvAssert dialog now. Project should move eventually towards standardisation of asserts.

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

No branches or pull requests

2 participants