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

FastBiomeQuery #151

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from
Draft

FastBiomeQuery #151

wants to merge 4 commits into from

Conversation

gotmachine
Copy link
Contributor

@gotmachine gotmachine commented Jun 5, 2023

The stock biome maps (CBAttributeMapSO class) store a texture as RGB color data in a byte array where every pixel takes 3 bytes. Each biome definition (CBAttributeMapSO.MapAttribute) is given a specific color, and is stored in an array.
Querying a biome at a position (CBAttributeMapSO.GetAtt()) is a complex method that perform bilinear interpolation on that color array, attempts to find the closest color in the biome definition array, and does a bunch of heuristics in case the texture colors don't exactly match the ones defined in the biome definitions.

Some issues with this :

  • Biome querying is quite slow as a result, which can be a bottleneck with mods doing a lot of such queries (Kerbalism, ScanSAT, Parallax scatters, ContractConfigurator...) , but with some stock code too (Contracts, KerbNet...).
  • Storing full pixel color data takes up a lot of unnecessary RAM
  • The stock bilinear interpolation has a bug resulting in jagged transitions between biomes along a specific direction

This is a replacement for the stock class, implemented as a KSPCFFastBiomeMap class deriving from CBAttributeMapSO :

  • Instead of storing color data taking 3 bytes per pixel, store biome indices, taking 1 byte per pixel, reducing memory usage by a factor 3.
  • Ability to reinterpret the pixel color data as biome indices (either from an already compiled CBAttributeMapSO or from a Texture2D), following the same heuristics as the stock GetAtt() method, allowing upfront validation of biome map, and preventing having to do it on every query.
  • Reimplement the CBAttributeMapSO.GetAtt() method doing a direct lookup for the biome index instead of parsing colors, performing a correct bilinear interpolation.
  • Fix incorrect coordinate wrapping at the north pole (stock bug), resulting in the south pole biome being returned near the north pole.

The benefits are :

  • Reduced memory usage :
    • Stock : from 189MB to 63MB
    • RSS : from 82MB to 27MB
    • OPM : from 549MB to 183MB
  • An order of magnitude faster, and constant time biome querying (GetAtt()), unaffected by biome count. On average 15-20x faster than the stock implementation, up to 30x on bodies with a large amount of biomes.

Drawbacks :

  • This introduce a limitation in that amount of biomes per body is limited to 256 whereas the stock implementation supports a virtually infinite amount (2^24). I doubt this can be an issue in practice.
  • This require an upfront conversion of all the CBAttributeMapSO instances into a KSPCFFastBiomeMap. This is performed after PSystemSetup to catch Kopernicus changes, and before the main menu is loaded. The conversion is multithreaded, but it will introduce a perceptible delay on lower end machines. For reference, on a 5800X3D :
    • Stock maps conversion, 1 thread : 1.4s
    • Stock maps conversion, 16 threads : 0.2s
    • Stock + OPM maps conversion, 1 thread : 3.2s
    • Stock + OPM maps conversion, 16 threads : 0.5s

While I think this is acceptable, it should be noted that doing this in the context of biome maps loaded by Kopernicus is utterly inefficient, as the complete chain of events is something like :

  • Kopernicus load the biome texture files as a byte[] from disk
  • For some texture formats, it first parse it as a Color32[], then copy it as a Texture2D, in other cases it relies on Unity to perform the conversion from the byte[] to a Texture2D.
  • It feed that texture to the stock CBAttributeMapSO parser, which :
    • Convert it as a Color32[]
    • Convert the Color32[] to a 3Bpp RGB data byte[]
  • Then KSPCF uses that RGB 3 Bpp byte[] to produce a biome indices 1Bpp byte[], with a Color32[] conversion in between.

It would be vastly more efficient for Kopernicus to directly feed the KSPCF implementation the Texture2D or Color32[] without the useless conversion to a CBAttributeMapSO in between. This is difficult to implement from the KSPCF side (at least efficiently). Aside, Kopernicus would benefit from an overhaul of its biome querying stuff, so it seems more sensible to actually have a side by side implementation of the KSPCF fast biome in Kopernicus.

From a practial PoV, I will be AWOL from the KSP modding scene for some time, effective right now. Consequently, I will delay pushing this patch to KSPCF in case there are issues popping up that require some fixing. Plus I would like to contribute that work to Kopernicus, ideally synchronizing that feature release both in KSPCF and Kopernicus.

In the meantime, here is a build with the changes, for those willing to test it :
KSPCommunityFixes_1.29.0_FastBiomeQueryPR.zip

For reference, this illustrate the bug with biome transitions in the stock bilinear sampler :
image

…, need to condition BiomeMapOptimizer from a KSPCF patch

- MapSOCorrectWrapping : fixed some issues and improved performance a bit
@gotmachine gotmachine added kspBug Identified KSP issue kspPerformance Possible performance improvement in KSP labels Jun 5, 2023
@R-T-B

This comment was marked as resolved.

@gotmachine

This comment was marked as resolved.

@R-T-B

This comment was marked as resolved.

@R-T-B

This comment was marked as resolved.

@R-T-B

This comment was marked as resolved.

@R-T-B

This comment was marked as resolved.

@gotmachine

This comment was marked as resolved.

@gotmachine
Copy link
Contributor Author

On a side note, I did a bit of perf profiling between the Kopernicus "biome cache" used by Parallax and the FastBiomeQuery
implementation.
FastBiomeQuery is consistently 5-10x faster than the Kopernicus biome cache when it is at its default level (ScatterLatLongDecimalPrecision = 5), and that's when the cache is already populated.
When it's not, the cost is closer to 20-50x, and this is the case in many situations (for example low/mid altitude flying in a straight line). Looking at the cache implementation, there are a few inefficiencies, but I don't think that would change the overall figure, the dictionaries lookups are more costly than just querying the biome directly.

I think I will look into pushing the FastBiomeQuery implementation as a Koperncius PR as well as a KSPCF patch. The existing Kop biome sampler can be rewired to use it, and I can disable the KSPCF patch when Koperncius is present.

@R-T-B

This comment was marked as resolved.

@R-T-B

This comment was marked as resolved.

@gotmachine

This comment was marked as resolved.

@R-T-B

This comment was marked as resolved.

@gotmachine
Copy link
Contributor Author

For reference, the Kopernicus implementation of this patch has been released.
Since the KSPCF patch only tries to convert the stock CBAttributeMapSO type in the OnDestroy() of PSystemSpawn, maps already converted by Kopernicus in PSystemSpawn are of type KopernicusCBAttributeMapSO and should be ignored by KSPCF, so the two implementations should be able to coexist without issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kspBug Identified KSP issue kspPerformance Possible performance improvement in KSP
Development

Successfully merging this pull request may close these issues.

2 participants