Skip to content

Feature replacement bugfix #836

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jun 5, 2018
Merged

Feature replacement bugfix #836

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e5f855b
Fix featureId's being overridden by last features ID.
abhishektrip May 29, 2018
e73b012
Fix for replacement of feature with multiple parts.
abhishektrip Jun 2, 2018
1f1edb0
Code cleanup.
abhishektrip Jun 4, 2018
1bfee11
Further code cleanup based on review comments.
abhishektrip Jun 4, 2018
c048dd8
Formating change.
abhishektrip Jun 4, 2018
8818404
Make function less deeply nested.
abhishektrip Jun 4, 2018
5fc4c8c
added a way to explicitly block feature ids
apavani Jun 4, 2018
21a12d3
clear feature list after processing
Jun 5, 2018
bd122c2
fix for duplicate buildings + zoom/pan
abhishektrip Jun 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions sdkproject/Assets/Mapbox/Unity/DataContainers/MapboxEnums.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,11 @@ public enum LocationPrefabCategories
Shops = 1 << 5,
Transportation = 1 << 6
}

public enum FeatureProcessingStage
{
PreProcess,
Process,
PostProcess
}
}
10 changes: 6 additions & 4 deletions sdkproject/Assets/Mapbox/Unity/MeshGeneration/Interfaces/LayerVisualizerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ namespace Mapbox.Unity.MeshGeneration.Interfaces
/// Layer visualizers contains sytling logic and processes features
/// </summary>
public abstract class LayerVisualizerBase : ScriptableObject
{
public bool Active = true;
public abstract string Key { get; set; }
public abstract void Create(VectorTileLayer layer, UnityTile tile, Action callback = null);
{
public bool Active = true;
public abstract string Key { get; set; }
public event Action FeaturePreProcessEvent;
public event Action FeaturePostProcessEvent;
public abstract void Create(VectorTileLayer layer, UnityTile tile, Action callback = null);

public virtual void Initialize()
{
Expand Down
262 changes: 211 additions & 51 deletions sdkproject/Assets/Mapbox/Unity/MeshGeneration/LayerVisualizers/VectorLayerVisualizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@
using UnityEngine;
using Mapbox.Unity.Map;
using Mapbox.Unity.Utilities;
using Mapbox.Unity.MeshGeneration.Filters;

public class VectorLayerVisualizerProperties
{
public FeatureProcessingStage featureProcessingStage;
public bool buildingsWithUniqueIds = false;
public VectorTileLayer vectorTileLayer;
public ILayerFeatureFilterComparer[] layerFeatureFilters;
public ILayerFeatureFilterComparer layerFeatureFilterCombiner;
}


public class VectorLayerVisualizer : LayerVisualizerBase
{
Expand Down Expand Up @@ -47,6 +58,8 @@ public ModifierStackBase DefaultModifierStack
private Dictionary<UnityTile, List<ulong>> _idPool; //necessary to keep _activeIds list up to date when unloading tiles
private string _key;

private Dictionary<UnityTile, VectorLayerVisualizerProperties> _vectorFeaturesPerTile = new Dictionary<UnityTile, VectorLayerVisualizerProperties>();

public override string Key
{
get { return _layerProperties.coreOptions.layerName; }
Expand All @@ -56,7 +69,7 @@ public override string Key
public void SetProperties(VectorSubLayerProperties properties, LayerPerformanceOptions performanceOptions)
{
List<MeshModifier> defaultMeshModifierStack = new List<MeshModifier>();
List<GameObjectModifier> defaultGOModifierStack = new List<GameObjectModifier>();
List<GameObjectModifier> defaultGOModifierStack = new List<GameObjectModifier>();
_layerProperties = properties;
_performanceOptions = performanceOptions;

Expand Down Expand Up @@ -182,8 +195,102 @@ public void SetProperties(VectorSubLayerProperties properties, LayerPerformanceO
//Add any additional modifiers that were added.
_defaultStack.MeshModifiers.AddRange(_layerProperties.MeshModifiers);
_defaultStack.GoModifiers.AddRange(_layerProperties.GoModifiers);

}

/// <summary>
/// Add the replacement criteria to any mesh modifiers implementing IReplaceable
/// </summary>
/// <param name="criteria">Criteria.</param>
protected void SetReplacementCriteria(IReplacementCriteria criteria)
{
foreach (var meshMod in _defaultStack.MeshModifiers)
{
if (meshMod is IReplaceable)
{
((IReplaceable)meshMod).Criteria.Add(criteria);
}
}
}

#region Private Helper Methods
/// <summary>
/// Convenience function to add feature to Tile object pool.
/// </summary>
/// <param name="feature">Feature to be added to the pool.</param>
/// <param name="tile">Tile currently being processed.</param>
private void AddFeatureToTileObjectPool(VectorFeatureUnity feature, UnityTile tile)
{
_activeIds.Add(feature.Data.Id);
if (!_idPool.ContainsKey(tile))
{
_idPool.Add(tile, new List<ulong>());
}
else
{
_idPool[tile].Add(feature.Data.Id);
}
}

/// <summary>
/// Apply filters to the layer and check if the current feature is eleigible for rendering.
/// </summary>
/// <returns><c>true</c>, if feature eligible after filtering was applied, <c>false</c> otherwise.</returns>
/// <param name="feature">Feature.</param>
private bool IsFeatureEligibleAfterFiltering(VectorFeatureUnity feature, UnityTile tile)
{
if (_vectorFeaturesPerTile[tile].layerFeatureFilters.Count() == 0)
{
return true;
}
else
{
// build features only if the filter returns true.
if (_vectorFeaturesPerTile[tile].layerFeatureFilterCombiner.Try(feature))
{
return true;
}
}
return false;
}

/// <summary>
/// Function to fetch feature in vector tile at the index specified.
/// </summary>
/// <returns>The feature in tile at the index requested.</returns>
/// <param name="tile">Unity Tile containing the feature.</param>
/// <param name="index">Index of the vector feature being requested.</param>
private VectorFeatureUnity GetFeatureinTileAtIndex(int index, UnityTile tile)
{
return new VectorFeatureUnity(_vectorFeaturesPerTile[tile].vectorTileLayer.GetFeature(index),
tile,
_vectorFeaturesPerTile[tile].vectorTileLayer.Extent,
_vectorFeaturesPerTile[tile].buildingsWithUniqueIds);
}

/// <summary>
/// Function to check if the feature is already in the active Id pool, features already in active Id pool should be skipped from processing.
/// </summary>
/// <returns><c>true</c>, if feature is already in activeId pool or if the layer has buildingsWithUniqueId flag set to <see langword="true"/>, <c>false</c> otherwise.</returns>
/// <param name="featureId">Feature identifier.</param>
private bool ShouldSkipProcessingFeatureWithId(ulong featureId, UnityTile tile)
{
return (_vectorFeaturesPerTile[tile].buildingsWithUniqueIds && _activeIds.Contains(featureId));
}

/// <summary>
/// Gets a value indicating whether this entity per coroutine bucket is full.
/// </summary>
/// <value><c>true</c> if coroutine bucket is full; otherwise, <c>false</c>.</value>
private bool IsCoroutineBucketFull
{
get
{
return (_performanceOptions != null && _performanceOptions.isEnabled && _entityInCurrentCoroutine >= _performanceOptions.entityPerCoroutine);
}
}

#endregion
public override void Initialize()
{
base.Initialize();
Expand All @@ -205,7 +312,7 @@ public override void Create(VectorTileLayer layer, UnityTile tile, Action callba
_activeCoroutines[tile].Add(Runnable.Run(ProcessLayer(layer, tile, callback)));
}

private IEnumerator ProcessLayer(VectorTileLayer layer, UnityTile tile, Action callback = null)
protected IEnumerator ProcessLayer(VectorTileLayer layer, UnityTile tile, Action callback = null)
{
//HACK to prevent request finishing on same frame which breaks modules started/finished events
yield return null;
Expand All @@ -215,91 +322,123 @@ private IEnumerator ProcessLayer(VectorTileLayer layer, UnityTile tile, Action c
yield break;
}

//testing each feature with filters
var fc = layer.FeatureCount();
Debug.Log("Tile Id -> " + tile.name + "Layer Name -> " + layer.Name + " features -> " + layer.FeatureCount());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this Debug.Log()?

if (!_vectorFeaturesPerTile.ContainsKey(tile))
{
_vectorFeaturesPerTile.Add(tile, new VectorLayerVisualizerProperties
{
vectorTileLayer = layer,
});
}

//Get all filters in the array.
var filters = _layerProperties.filterOptions.filters.Select(m => m.GetFilterComparer()).ToArray();
_vectorFeaturesPerTile[tile].layerFeatureFilters = _layerProperties.filterOptions.filters.Select(m => m.GetFilterComparer()).ToArray();

// Pass them to the combiner
Filters.ILayerFeatureFilterComparer combiner = new Filters.LayerFilterComparer();
_vectorFeaturesPerTile[tile].layerFeatureFilterCombiner = new Filters.LayerFilterComparer();
switch (_layerProperties.filterOptions.combinerType)
{
case Filters.LayerFilterCombinerOperationType.Any:
combiner = Filters.LayerFilterComparer.AnyOf(filters);
_vectorFeaturesPerTile[tile].layerFeatureFilterCombiner = Filters.LayerFilterComparer.AnyOf(_vectorFeaturesPerTile[tile].layerFeatureFilters);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the advantage of removing the variable combiner and writing _vectorFeaturesPerTile[tile].layerFeatureFilterCombiner instead? Same question for filters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Caching the variables, so we don't have to compute the values multiple times + avoid passing 5 variables to every function.

break;
case Filters.LayerFilterCombinerOperationType.All:
combiner = Filters.LayerFilterComparer.AllOf(filters);
_vectorFeaturesPerTile[tile].layerFeatureFilterCombiner = Filters.LayerFilterComparer.AllOf(_vectorFeaturesPerTile[tile].layerFeatureFilters);
break;
case Filters.LayerFilterCombinerOperationType.None:
combiner = Filters.LayerFilterComparer.NoneOf(filters);
_vectorFeaturesPerTile[tile].layerFeatureFilterCombiner = Filters.LayerFilterComparer.NoneOf(_vectorFeaturesPerTile[tile].layerFeatureFilters);
break;
default:
break;
}

for (int i = 0; i < fc; i++)
{

var buildingsWithUniqueIds =
(_layerProperties.honorBuildingIdSetting) && _layerProperties.buildingsWithUniqueIds;
_vectorFeaturesPerTile[tile].buildingsWithUniqueIds = (_layerProperties.honorBuildingIdSetting) && _layerProperties.buildingsWithUniqueIds;

var feature = new VectorFeatureUnity(layer.GetFeature(i), tile, layer.Extent, buildingsWithUniqueIds);

//skip existing features, only works on tilesets with unique ids
if (buildingsWithUniqueIds && _activeIds.Contains(feature.Data.Id))
{
continue;
}
else
////find any replacement criteria and assign them
foreach (var goModifier in _defaultStack.GoModifiers)
{
if (goModifier is IReplacementCriteria &&
goModifier.Active)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could go on line 359 rather than being split into another line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

{
_activeIds.Add(feature.Data.Id);
if (!_idPool.ContainsKey(tile))
{
_idPool.Add(tile, new List<ulong>());
}
else
{
_idPool[tile].Add(feature.Data.Id);
}
SetReplacementCriteria((IReplacementCriteria)goModifier);
}
}

if (filters.Length == 0)
{
// no filters, just build the features.
if (tile != null && tile.gameObject != null && tile.VectorDataState != Enums.TilePropertyState.Cancelled)
Build(feature, tile, tile.gameObject);
#region PreProcess & Process.

_entityInCurrentCoroutine++;
}
else
var fc = _vectorFeaturesPerTile[tile].vectorTileLayer.FeatureCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename fc to featureCount (or whatever descriptive name works best)

do
{
for (int i = 0; i < fc; i++)
{
// build features only if the filter returns true.
if (combiner.Try(feature))
{
if (tile != null && tile.gameObject != null && tile.VectorDataState != Enums.TilePropertyState.Cancelled)
Build(feature, tile, tile.gameObject);
ProcessFeature(i, tile);

_entityInCurrentCoroutine++;
if (IsCoroutineBucketFull)
{
//Reset bucket..
_entityInCurrentCoroutine = 0;
yield return null;
}
}
// move processing to next stage.
_vectorFeaturesPerTile[tile].featureProcessingStage++;
} while (_vectorFeaturesPerTile[tile].featureProcessingStage == FeatureProcessingStage.PreProcess
|| _vectorFeaturesPerTile[tile].featureProcessingStage == FeatureProcessingStage.Process);

if (_performanceOptions!=null && _performanceOptions.isEnabled && _entityInCurrentCoroutine >= _performanceOptions.entityPerCoroutine)
{
_entityInCurrentCoroutine = 0;
yield return null;
}
}
#endregion

#region PostProcess
// TODO : Clean this up to follow the same pattern.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the status of this TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added the TODO. This needs a bigger more involved re-factor. tl;dr - Modifier stack methods need to be consistent in accepting one feature/or multiple features. Using different inputs causes patterns to break.

var mergedStack = _defaultStack as MergedModifierStack;
if (mergedStack != null && tile != null)
{
mergedStack.End(tile, tile.gameObject, layer.Name);
}
#endregion

if (callback != null)
callback();
}

private bool ProcessFeature(int index, UnityTile tile)
{
var feature = GetFeatureinTileAtIndex(index, tile);

//feature not skipped. Add to pool only if features are in preprocess stage.
if (_vectorFeaturesPerTile[tile].featureProcessingStage == FeatureProcessingStage.PreProcess)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a few lines where we check what stage a tile is on; wonder if there's a shorter way we can write this

{
//skip existing features, only works on tilesets with unique ids
if (ShouldSkipProcessingFeatureWithId(feature.Data.Id, tile))
{
Debug.Log("Skipped");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still want this Debug.Log()?

return false;
}
AddFeatureToTileObjectPool(feature, tile);
}

if (IsFeatureEligibleAfterFiltering(feature, tile))
{
if (tile != null && tile.gameObject != null && tile.VectorDataState != Enums.TilePropertyState.Cancelled)
{
Debug.Log(_vectorFeaturesPerTile[tile].featureProcessingStage);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still want this Debug.Log()?

switch (_vectorFeaturesPerTile[tile].featureProcessingStage)
{
case FeatureProcessingStage.PreProcess:
PreProcessFeatures(feature, tile, tile.gameObject);
break;
case FeatureProcessingStage.Process:
Build(feature, tile, tile.gameObject);
break;
case FeatureProcessingStage.PostProcess:
break;
default:
break;
}
_entityInCurrentCoroutine++;
}
}
return true;
}

/// <summary>
/// Preprocess features, finds the relevant modifier stack and passes the feature to that stack
/// </summary>
Expand All @@ -317,6 +456,19 @@ private bool IsFeatureValid(VectorFeatureUnity feature)
return true;
}

protected void PreProcessFeatures(VectorFeatureUnity feature, UnityTile tile, GameObject parent)
{
////find any replacement criteria and assign them
foreach (var goModifier in _defaultStack.GoModifiers)
Copy link
Contributor

@apavani apavani Jun 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This approach works well for now, but, it breaks the general architecture where VectorLayerVisualizer is bypassing the ModifierStack to communicate directly with GoModifiers. It's bad design and we should either get rid of ModifierStack or move this part of the code to ModifierStack in the future.

{
if (goModifier is IReplacementCriteria &&
goModifier.Active)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to line 464 so it's all on one line

{
goModifier.FeaturePreProcess(feature);
}
}
}

protected void Build(VectorFeatureUnity feature, UnityTile tile, GameObject parent)
{
if (feature.Properties.ContainsKey("extrude") && !Convert.ToBoolean(feature.Properties["extrude"]))
Expand All @@ -343,6 +495,14 @@ protected void Build(VectorFeatureUnity feature, UnityTile tile, GameObject pare
}
}

protected void PostProcessFeatures(VectorFeatureUnity feature, UnityTile tile, GameObject parent)
{
var mergedStack = _defaultStack as MergedModifierStack;
if (mergedStack != null && tile != null)
{
mergedStack.End(tile, tile.gameObject, _vectorFeaturesPerTile[tile].vectorTileLayer.Name);
}
}
private string FindSelectorKey(VectorFeatureUnity feature)
{
// TODO: FIX THIS!!
Expand Down
Loading