Skip to content

fix file cache path format#1972

Merged
brnkhy merged 2 commits intodevelopfrom
fixFileCachePathBug
Feb 25, 2026
Merged

fix file cache path format#1972
brnkhy merged 2 commits intodevelopfrom
fixFileCachePathBug

Conversation

@brnkhy
Copy link
Copy Markdown
Contributor

@brnkhy brnkhy commented Feb 25, 2026

fix image file names to use underscore to fix bugs and issues in small x/y coordinates (in example; 223 to 22_3) add runtime cache manager behaviour to be able to change database name (prefix) to start/test with a clean cache

fix image file names to use underscore to fix bugs and issues in small x/y coordinates (in example; 223 to 22_3)
add runtime cache manager behaviour to be able to change database name (prefix) to start/test with a clean cache
Copy link
Copy Markdown
Collaborator

@astojilj astojilj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: fix file cache path format

Summary

This PR fixes an ambiguous file cache path format and adds a RuntimeCacheManagerBehaviour with custom database name support. It also cleans up namespaces and moves editor files to their proper location.

The core fix is correct and important — tile filenames like {X}{Y}{Z}.png are changed to {X}_{Y}_{Z}.png so that coordinates like X=22, Y=3, Z=5 don't collide with X=2, Y=23, Z=5 (both would have produced 2235.png).


Issues

1. 🔴 Editor asmdef will break builds — MapboxBaseModule.Editor.asmdef

The new assembly definition has "includePlatforms": [] which includes it on all platforms. Since RuntimeCacheManagerBehaviourEditor.cs uses UnityEditor, this will cause compilation errors on non-Editor builds. It should be:

"includePlatforms": ["Editor"],

Compare with the existing Editor/Mapbox.UnitySDK.Editor.asmdef which correctly specifies "includePlatforms": ["Editor"].

2. ⚠️ Unused SerializedProperties — RuntimeCacheManagerBehaviourEditor.cs

createSqliteCache and createFileCache are fetched in OnEnable() but never used in OnInspectorGUI(). The inspector only shows useCustomName and conditionally customName. This means users can't toggle SQLite/file cache creation from the inspector. Either expose them or remove the dead fields.

3. ⚠️ Redundant condition — RuntimeCacheManagerBehaviour.cs

if (CreateFileCache)
{
    if (UseCustomName)
    {
        fileCache = CreateFileCache ? new FileCache(...) : null;  // already inside if(CreateFileCache)
    }
    else
    {
        fileCache = CreateFileCache ? new FileCache(...) : null;  // same
    }
}

The ternary check on CreateFileCache is redundant since we're already inside if (CreateFileCache). Should be simplified to:

if (CreateFileCache)
{
    fileCache = UseCustomName
        ? new FileCache(unityContext.TaskManager, CustomName)
        : new FileCache(unityContext.TaskManager);
}

4. ⚠️ Breaking change: existing file cache invalidated

The underscore format change means all previously cached tiles will be orphaned — the SDK won't find them. This is the correct fix, but it should be called out as a breaking/migration note so users know their cache will effectively be cleared on upgrade.

5. 💅 Missing newline at EOF

RuntimeCacheManagerBehaviour.cs, RuntimeCacheManagerBehaviourEditor.cs, MapboxBaseModule.Editor.asmdef, and RuntimeCacheManagerBehaviour.cs.meta all lack a trailing newline.


What looks good

  • The core bug fix (underscore separators in tile filenames) is correct and important
  • Namespace alignment (BaseModule.Unity.ModuleBehaviours) matches the actual file location — good housekeeping
  • Moving editor scripts from Example/Editor/ to BaseModule/Editor/ is the right call
  • Unused using cleanup is welcome
  • The custom name feature design (postfix on folder name) integrates cleanly with existing FileCache/SqliteCache constructors

remove unnecessary if check in runtime cache behaviour
Copy link
Copy Markdown
Collaborator

@astojilj astojilj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review after update (commit 2930482)

Fixed

  • Editor asmdef platform restriction — Now correctly has "includePlatforms": ["Editor"]. Build-breaking bug resolved.
  • Redundant ternary in RuntimeCacheManagerBehaviour — The CreateFileCache ? ternary inside if (CreateFileCache) is gone. The logic is now clean nested if/else blocks.

Still open

  1. Unused SerializedProperties in RuntimeCacheManagerBehaviourEditor.cscreateSqliteCache and createFileCache are still fetched in OnEnable() but never rendered in OnInspectorGUI(). Users can't toggle these from the inspector.

  2. Breaking change (existing file cache invalidated) — Still worth a migration note in release docs. The underscore format change orphans all previously cached tiles.

  3. Missing newline at EOFRuntimeCacheManagerBehaviour.cs, RuntimeCacheManagerBehaviourEditor.cs, MapboxBaseModule.Editor.asmdef, RuntimeCacheManagerBehaviour.cs.meta.

Verdict

The critical bug is fixed. Remaining items are minor. Looks good to merge once #1 (unused properties) is addressed or confirmed intentional.

Copy link
Copy Markdown
Collaborator

@astojilj astojilj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brnkhy brnkhy merged commit 4648c10 into develop Feb 25, 2026
2 checks passed
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.

2 participants