Texture cache fixes #4970
Merged
Texture cache fixes #4970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've restored the old ordering of the variable declarations in |
If "address" is not page-aligned, the calculated end address for the corresponding page is incorrect which can lead to false positives, causing some textures to be re-uploaded unnecessarily. This commit fixes this typo.
When the trampled range changes, get_intersecting_set restarts the outer loop. However, due to an off-by-one error, it skips the first cache entry when doing so. This can cause a texture not to be correctly unlocked, which could lead to issues or even deadlocks. This commit fixes this off-by-one error.
The existing implementation restarts the loop immediately after finding a range_data instance that updates the trampled_range. This commit refactors this method to continue the loop with the updated trampled_range, and then repeat only those range_data instances that were iterated through before the trampled_range was last updated. As a result, the number of total iterations required is reduced.
invalidate_range_impl_base does not mark all textures that will only be unprotected as dirty when doing a deferred flush, since that is done by flush_all. However, if there are no sections to flush, the deferred flush will use the same code path as non-deferred flushes for unprotecting textures and forget to mark them as dirty. This commit fixes this bug.
|
I have added a third fix to this PR. After a lot more time reviewing the texture cache code and pestering @kd-11 with many questions (thank you for your help!) I found a third bug:
With this fix I believe I have finally tracked down the bug causing the P5 texture swaps (see #4298), but further testing is needed. |
|
I have updated #4298 with some test results for this PR. tl;dr: P5 texture swap bug is fixed during normal gameplay, but not inside the Velvet Room. I suspect there is a fourth bug somewhere that only triggers in there. |
kd-11
approved these changes
Aug 16, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While investigating the P5 texture swap issues, I stumbled into two different bugs in the texture cache, specifically in the
overlaps_pageandget_intersecting_setmethods.This pull request fixes these two bugs:
overlaps_page, ifaddressis not page-aligned, the calculated end address for the corresponding page is incorrect which can lead to false positives, causing some textures to be re-uploaded unnecessarily.get_intersecting_setrestarts the outer loop. However, due to an off-by-one error, it skips the first cache entry when doing so. This can cause a texture to not be correctly unlocked, which could lead to issues such as texture corruption or even deadlocks.I have ran these through @kd-11 and he has confirmed these are actual bugs.
In addition, after some discussion with @kd-11, I have refactored
get_intersecting_setto avoid unnecessary looping. It will now only execute the minimum number of iterations necessary to ensure all textures have been processed using the correcttrampled_range. I have tested this last change by comparing it directly with the old implementation (see ruipin@25b6f2f) and saw no difference in behaviour.Note that I have only tested these changes with P5, since that is the only game I have available right now.
(This is my first PR here, comments/suggestions/criticism/etc are welcome!)