custom image api module and terrain fixes#1973
Conversation
add LoadTileData to Terrain module add mapbox map extensions class with TryGetElevation, TryGetMapTile methods fix map visualizer to check inside composite modules when searching fix a bug where terrain module target tile id method wasn't clamped add custom image api module change unity map service to have public getters for data fetcher and cache manager
astojilj
left a comment
There was a problem hiding this comment.
PR Review: custom image api module and terrain fixes
Author: @brnkhy | Branch: customImageryApiSupport → develop | +625 / -34 across 30 files
Summary
This PR adds:
- A new
CustomImageryModulefor loading tiles from arbitrary TMS/XYZ tile servers LoadTileDataonITerrainLayerModuleinterface + coroutine-basedGetElevationDataAPI onTerrainLayerModuleMapboxMapExtensionswithTryGetElevationandTryGetMapTilehelpers- Composite module search in
MapboxMapVisualizer.TryGetLayerModule - Public getters on
MapUnityServiceforCacheManager/FetchingManager - Elevation-aware
ConvertLatLngToPosition - Elevation integration tests
Issues
1. 🔴 BUG: TryGetMapTile(LatitudeLongitude) loop never changes the tile ID
MapboxMapExtensions.cs lines 75-88:
var maxTileId = Conversions.LatitudeLongitudeToTileId(coordinates, maxZoomLevel);
for (int i = maxZoomLevel; i > 1; i--)
{
if (map.MapVisualizer.ActiveTiles.TryGetValue(maxTileId, out tile))
{
return true;
}
}The loop decrements i but never uses it — maxTileId is the same on every iteration. This either finds the tile on the first check or loops uselessly ~19 times. It should recompute the tile ID at each zoom level (parent tile), e.g.:
for (int i = maxZoomLevel; i > 1; i--)
{
var tileId = Conversions.LatitudeLongitudeToTileId(coordinates, i);
if (map.MapVisualizer.ActiveTiles.TryGetValue(tileId, out tile))
return true;
}2. 🔴 BUG: Same issue in TryGetElevation — loop variable i is unused
MapboxMapExtensions.cs lines 99-109:
var maxTileId = Conversions.LatitudeLongitudeToTileId(location, maxZoomLevel);
for (int i = maxZoomLevel; i >= minZoomLevel; i--)
{
if (map.MapVisualizer.ActiveTiles.TryGetValue(maxTileId, out tile))
{
break;
}
}Identical problem — queries the same maxTileId every iteration.
3. 🔴 BUG: Same issue in TerrainLayerModule.GetElevationData — loop variable i unused
TerrainLayerModule.cs:
for (int i = 14; i >= 2; i--)
{
if (_rasterSource.GetInstantData(tileId, out var instantData))
{
...
break;
}
}tileId doesn't change inside the loop, i is never used. Should be walking up the zoom hierarchy.
4. ⚠️ Namespace inconsistency — all CustomImageryModule files use Mapbox.Example.Scripts.ModuleBehaviours
Every new file in Runtime/Mapbox/CustomImageryModule/ declares namespace Mapbox.Example.Scripts.ModuleBehaviours, which doesn't match the file location. Especially notable since PR #1972 is specifically renaming things away from Mapbox.Example.Scripts.ModuleBehaviours. Should probably be Mapbox.CustomImageryModule.
5. ⚠️ Hardcoded "mars_elevation" in CustomTerrainLayerModuleScript.cs
Settings.DataSettings.TilesetId = "mars_elevation";and
TilesetId = "mars_elevation"This looks like a development/test artifact. Should this be configurable, or at minimum use a constant?
6. ⚠️ Dead code in CustomApiLayerModuleScript.ConstructModule
The SourceType branching logic (None/Custom/else) sets Settings.DataSettings.TilesetId, but then the method immediately creates a CustomSource with a hardcoded TilesetId = "CustomImagery" — so the branching has no effect.
if (Settings.SourceType == ImagerySourceType.None)
{
// empty block
}
else if (Settings.SourceType == ImagerySourceType.Custom)
{
Settings.DataSettings.TilesetId = Settings.CustomSourceId;
}
else { ... }
// Then ignores all of the above:
var source = new CustomSource(CustomSourceSettings, ..., new ImageSourceSettings()
{
TilesetId = "CustomImagery"
});7. ⚠️ TryGetLayerModule only searches the first composite module
The fix in MapboxMapVisualizer.cs uses FirstOrDefault(x => x is CompositeLayerModule). If there are multiple CompositeLayerModule instances, only the first is searched. Consider iterating all composites.
8. ⚠️ Commented-out code left in
MapInformationStaticMethods.cs://var scaledDeltaMercatorVector3 = scaledDeltaMercator.ToVector3xz();CustomSource.cs://_dataTileMatch.Add(rasterData, tile);CustomTerrainSource.cs://_dataTileMatch.Add(rasterData, tile);
These should be removed before merging.
9. 💅 Empty Start() methods
CustomApiLayerModuleScript.cs and CustomTerrainLayerModuleScript.cs both have empty Start() methods that do nothing.
10. 💅 CustomSourceSettings.UrlFormat tooltip is misleading
Tooltip says '{}' fields for X/Y/Z coordinates but string.Format in CustomTMSTile.Initialize uses {0} for Z, {1} for X, {2} for invertedY. The parameter order should be documented clearly, e.g.: "Format: {0}=zoom, {1}=x, {2}=y".
11. 💅 Missing newline at EOF on most new files
What looks good
- The
GetElevationDatacoroutine API onTerrainLayerModulewith fallback to loading tile data is a solid design CheckExpirationmadeprotected virtualinImageSource— clean extensibility point forCustomSourceTerrainSource.LoadTileCoroutinenull-check onterrainData.Texture— good defensive fixGetDataIdclampingtargetZto min zoom — correct bug fix- The elevation integration tests with real-world landmarks are great for validation
- Public getters on
MapUnityServiceare a reasonable API surface change
fix namespaces for custom api module
add LoadTileData to Terrain module
add mapbox map extensions class with TryGetElevation, TryGetMapTile methods fix map visualizer to check inside composite modules when searching fix a bug where terrain module target tile id method wasn't clamped add custom image api module
change unity map service to have public getters for data fetcher and cache manager