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

Fixed caching of new WorldData locations #2586

Conversation

KABoissonneault
Copy link
Collaborator

@KABoissonneault KABoissonneault commented Feb 17, 2024

Before the change, the logic for whether to cache the result of WorldDataReplacement.GetDFRegionAdditionalLocationData was as follows:

bool success = false
foreach(asset in loose files)
  success = AddLocationToRegion(asset)
foreach(asset in packed mods)
  success &= AddLocationToRegion(asset)
...
if(success)
  cache result

AddLocationToRegion pretty much always returns true. Therefore, if we have any loose files, success is assigned true in the first loop, and the second loop will do success = true & true, and success will stay true.

If we do not have any loose files, success stays false, and the second loop will do success = false & true, making success false.

This means setups where a region has new locations in packed mods but not locations in loose files always fail to cache the result, resulting in poor performance and log spam. This process is called each time a map pixel is loaded.

How to repro:

  • Install betony restored.dfmod in a release build (editor builds never cache results, so that doesn't change)
  • Make sure your StreamingAssets/WorldData has no files named locationnew-*-19.json (new betony locations)
  • Go to Betony

With this change, I changed the logic to

bool success = true
foreach(asset in loose files)
  success &= AddLocationToRegion(asset)
foreach(asset in packed mods)
  success &= AddLocationToRegion(asset)
...
if(success)
  cache result

We still only cache if no errors occur in the process, but at least we don't need to go through loose files to start as true.

@KABoissonneault KABoissonneault added this to the DFU 1.0.1 milestone Feb 17, 2024
@ajrb
Copy link
Collaborator

ajrb commented Feb 18, 2024

Gah, stupid logic screw up on my part. The second assignment should have been |= and that's the fix I'd have made. Your fix does make it a bit clearer though. Thumbs up from me for merging.

BTW If you see anything like this in my DFU code in future please feel free to just let me know and I will deal with it. (slower maybe tho) While I appreciate the fix and the long explanation, I think you can probably use your time better.

@KABoissonneault KABoissonneault merged commit bd11b24 into Interkarma:master Feb 19, 2024
@KABoissonneault KABoissonneault deleted the fix/worlddata-new-location-caching branch February 19, 2024 00:29
@KABoissonneault KABoissonneault mentioned this pull request Mar 6, 2024
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