Interkarma / daggerfall-unity

Open source recreation of Daggerfall in the Unity engine
http://www.dfworkshop.net
MIT License
2.7k stars 327 forks source link

Fixed caching of new WorldData locations #2586

Closed KABoissonneault closed 6 months ago

KABoissonneault commented 6 months ago

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:

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.

ajrb commented 6 months ago

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.