Map update develop merge fresh #994

greglemonmapbox · 2018-09-05T23:00:50Z

Related issue

Runtime map updating

Description of changes

Added EditorHelper class, which allows us to easily get class references in prop drawers
Added MapboxDataProperty, which contains a settable HasChanged bool and a corresponding System.EventHandler that gets fired when HasChanged is set to true
ModifierBase, LayerProperties and CoreVectorLayerProperties extend MapboxDataProperty
PropertyDrawers for relevant AbstractMap UI classes use resulting bool return value of SerializedObject.ApplyModifiedProperties to set HasChanged to true
Image section: Changing DataSource at runtime reloads image layer of map
Terrain section: Changing DataSource, ElevationLayerType, and ExaggerationFactor reloads terrain and vector section of map

Known issues

Vector section UI changes fire events; right now, VectorLayerVisualizer.UpdateVector listens for these events, but only displays debug message for now.
Changing terrain UI causes tiles to be un/re-registered, which means VectorLayerVisualizer re-adds VectorLayerVisualizer.UpdateVector to the various layer properties PropertyHasChanged event, resulting in continually multiplying listener triggering, which can be seen in the resulting debug messages in the console window.

QA checklists

Add relevant code comments. Every API class and method should have <summary> description as well as description of parameters.
Add tests for new/changed/updated classes and methods!!!
Check out conventions in CONTRIBUTING.md.
Check out conventions in CODING-STYLE.md
Update the changelog
Update documentation.

Reviewers

Tag your reviewer(s). Choose wisely.

…Data method, which overrides DataFetcher.FetchData and accepts DataFetcherParameters as the sole param, which is them cast to required subclasses as needed.

…using _activeTiles dictionary in AbstractMapVisualizer instead. MapVisualizer.RedrawImageLayer listens for event OnMapIdUpdated and handles business of redrawing/refetching image layer, triggering event MapVisualizer.OnImageLayerRedrawn upon completion. Public event AbstractMap.OnImageLayerRedrawn listens for event MapVisualizer.OnImageLayerRedrawn; developers can register custom gameplay functions to listen for this event in their code.

…d return type of TerrainLayer.Factory to TerrainFactoryBase; was being downcast to AbstractMapTileFactory.

… map visualizer add current terrain strategy field in unity tile

…creation

…ges, and do so by calling UpdateProperty in the property script. Removed INotify interface; replaced with simple delegate events instead.

change VectorLayer to inherit from AbstractLayer

…ct map remove layer redrawn even in map visualizer as command is coming from abstract map now anyway create OnMapRedrawn event in abstract map to replace individual layer events

…e class they are drawing; initial work on getting general section to trigger changes in UI.

…m/mapbox/mapbox-unity-sdk into INotifyPropetyChanged-exploration

… out SetChildProperties call in AbstractMap line 498.

…sed null values merged OnErrorOccurred and OnError methods in factories change vector layer visualizer to create new modules iff they do not already exist (work in progress)

…Drawer.UpdateProperty to ensure update map functionality uses up-to-date data.

…d of GUI.changed

…-merge-fresh # Conflicts: # sdkproject/Assets/Mapbox/Unity/Editor/MapManagerEditor.cs # sdkproject/Assets/Mapbox/Unity/Editor/PropertyDrawers/ElevationLayerPropertiesDrawer.cs # sdkproject/Assets/Mapbox/Unity/Map/AbstractMapVisualizer.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Data/UnityTile.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/AbstractTileFactory.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/ImageDataFetcher.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/MapImageFactory.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/TerrainDataFetcher.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/TerrainFactoryBase.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/TerrainStrategies/ElevatedTerrainStrategy.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/TerrainStrategies/FlatSphereTerrainStrategy.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/TerrainStrategies/LowPolyTerrainStrategy.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/VectorDataFetcher.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/VectorTileFactory.cs # sdkproject/Assets/Mapbox/Unity/MeshGeneration/LayerVisualizers/VectorLayerVisualizer.cs

fix an issue where terrain layer update didn't handle vector layer corrections (snap terrain) correct

…tement from Earcut.cs

…ayer feature sub property drawers.

…ctLayer.cs function calls with VectorUpdateType enum. UpdateProperty passes new VectorUpdate class instead of serializedProperty. Added additional delegates to VectorLayerProperties for different update types, but maybe we can call OnPropertyUpdated and pass/return VectorUpdateType? Maybe do public Func<VectorUpdateType, VectorUpdateType> OnPropertyUpdated = delegate { return VectorUpdateType}?

…roperties inherits from this. VectorLayerVisualizer assigns methods to delegate; temp solution displays debug messages for "Update Colliders" and "Update Materials". Both property drawers use hard coded 0 index to get a vectorSubLayer, need to find a way to get correct sublayer index to proceed.

…r; vector update is now triggered from material/collider option delegates, figures out which logic it needs to run via dictionary lookup, which retrieves funcion.

…wer scripts.

…wers now use EditorHelper.GetTargetObjectOfProperty to get class instance, and result check of ApplyModifiedProperties bool value to set HasChanged to true.

…re specific property sets w/ HasChanged = true.

…alls. Removed VectorUpdate class and UpdateProperty method from VectorLayerPropertiesDrawer. LayerProperties now inherits from MapboxDataProperty class. Removed unused OnPropertyUpdatedComplete/Collider/Material delegates from VectorLayerProperties.

…ved Begin/End change check calls, using bool result of ApplyModifiedProperties() to set HasChanged property.

… previous commit. Changed AbstractLayer.NotifyUpdateLayer and AbstractLayer.UpdateLayerHandler to use bool instead of VectorUpdateType as second argument.

…dditional layer delegates.

…lper.GetTargetObjectOfProperty(property) method.

abhishektrip

@greglemonmapbox @brnkhy Could you please go through the comments and review.

abhishektrip · 2018-09-06T16:34:50Z

sdkproject/Assets/Mapbox/Unity/DataContainers/GeometryMaterialOptions.cs

 			}
 		}

+		public int layerId;


Why do we need an id in GeometryMaterialOptions? LayerId should be part of feature sublayer properties.

abhishektrip · 2018-09-06T16:38:38Z

sdkproject/Assets/Mapbox/Unity/Editor/EditorHelper.cs

+using System.Linq;
+using System.Reflection;
+
+namespace com.spacepuppyeditor


Change this namespace to Mapbox.Editor , Add acknowledgement for spacepuppyeditor dev , remove all commented code.

abhishektrip · 2018-09-06T16:39:48Z

sdkproject/Assets/Mapbox/Unity/Editor/EditorHelper.cs

+
+				if (element.Contains("["))
+				{
+					//var tp = obj.GetType();


This function can me made to work by swapping the commented lines with the uncommented ones.

abhishektrip · 2018-09-06T16:40:38Z

sdkproject/Assets/Mapbox/Unity/Editor/PropertyDrawers/ColliderOptionsDrawer.cs

 	using UnityEngine;
 	using Mapbox.Unity.Map;
 	using Mapbox.VectorTile.ExtensionMethods;
+	using com.spacepuppyeditor;


Replace namespace with mapbox.editor.

abhishektrip · 2018-09-06T16:41:18Z

sdkproject/Assets/Mapbox/Unity/Editor/PropertyDrawers/CoreVectorLayerPropertiesDrawer.cs

 	using System.Linq;
 	using System;
 	using Mapbox.VectorTile.ExtensionMethods;
+	using com.spacepuppyeditor;


Replace with Mapbox.editor.

abhishektrip · 2018-09-06T17:46:13Z

...ct/Assets/Mapbox/Unity/MeshGeneration/Factories/TerrainStrategies/ElevatedTerrainStrategy.cs

 			}

-			if (tile.MeshFilter.mesh.vertexCount == 0)
+			//_newVertexList.Count is the vertex count this strategy is expected to use


Is this comment relevant? the check is changed to a different thing.

no not anymore, that check will probably change soon as well

abhishektrip · 2018-09-06T17:46:43Z

...ect/Assets/Mapbox/Unity/MeshGeneration/Factories/TerrainStrategies/LowPolyTerrainStrategy.cs

 			}

-			if (tile.MeshFilter.mesh.vertexCount == 0)
+			//_newVertexList.Count is the vertex count this strategy is expected to use


Remove comment if not relevant.

abhishektrip · 2018-09-06T17:47:31Z

sdkproject/Assets/Mapbox/Unity/MeshGeneration/Factories/VectorTileFactory.cs

+			}
 		}

+		//OnColliderChange


abhishektrip · 2018-09-06T17:49:13Z

sdkproject/Assets/Mapbox/Unity/MeshGeneration/LayerVisualizers/VectorLayerVisualizer.cs


-			_defaultStack.MeshModifiers.AddRange(defaultMeshModifierStack);
-			_defaultStack.GoModifiers.AddRange(defaultGOModifierStack);
+			//_defaultStack.MeshModifiers.AddRange(defaultMeshModifierStack);


Is this commented by mistake? Where are we adding default modifiers?

@atripathi-mb it's adding default modifiers directly to the list. previously they were added to defaultMeshModifierStack and that list was added to the real mesh mod list using add range.

abhishektrip · 2018-09-06T17:49:50Z

sdkproject/Assets/Mapbox/Unity/MeshGeneration/Modifiers/ModifierBase.cs

 	using System;
 	using Mapbox.Unity.MeshGeneration.Data;

+


Move MapboxDataProperty to it's own file. Also, namespace for MapboxDataProperty should not be Mapbox.Unity.MeshGeneration.Modifiers

…related to serialized property work and those referencing external classes; removed dead/commented code. Changed using com.spacepuppyeditor to Mapbox.Editor in relevant classes.

…rHelper.CheckForModifiedProperty.

…ugging removed comments from older versions of the ElevatedTerrainStrategy file removed two commented out lines in VectorLayerVisualizer to prevent confusion

* develop: Terrain section event refactor apply property changes at end of gui (#1009) fix an issue where TerrainFactoryBase didn't respect "none" terrain option (#1006) API's for runtime editing. (#1005) Image section event refactor (#1002) Fix buildingid setting (#998) Map update develop merge fresh (#994) Improve query height method (#979) Mid floor calculation bug fix (#968) Updated changelog version to v1.4.6 update changelog change ElevatedTerrainStrategy and ElevatedTerrainWithSidesStrategy classes to create flat terrain mesh on height data errors UWP fixes (and more) (#945)

greglemonmapbox and others added 30 commits July 25, 2018 12:36

Initial test of INotifyPropertyChanged interface.

fd31cf8

Working POC for runtime image texture update.

2a2bc5d

Refactored Image/Terrain/VectorDataFetcher classes to implement Fetch…

2e8691f

…Data method, which overrides DataFetcher.FetchData and accepts DataFetcherParameters as the sole param, which is them cast to required subclasses as needed.

Initial work to extend POC to work with terrain layer as well.

ce93dd7

Continued work on triggering terrain layer changes at runtime. Change…

575e7d6

…d return type of TerrainLayer.Factory to TerrainFactoryBase; was being downcast to AbstractMapTileFactory.

Merge branch 'develop' into INotifyPropetyChanged-exploration

1f7c0c6

change event routing for update system to go through abstract map and…

fa5b802

… map visualizer add current terrain strategy field in unity tile

add a flag in layer settings changed event to trigger vector layer re…

09b10f8

…creation

PropertyDrawers now use if (GUI.changed) to check property value chan…

12aede8

…ges, and do so by calling UpdateProperty in the property script. Removed INotify interface; replaced with simple delegate events instead.

add null check to UpdateLayer event

b659b39

add explanatory comments to TerrainLayer

5a61527

change VectorLayer to inherit from AbstractLayer

remove OnImageLayerRedrawn and OnTerrainLayerRedrawn events in abstra…

de4565d

…ct map remove layer redrawn even in map visualizer as command is coming from abstract map now anyway create OnMapRedrawn event in abstract map to replace individual layer events

Property drawers for layer properties now call methods directly in th…

fc71ccf

…e class they are drawing; initial work on getting general section to trigger changes in UI.

Merge branch 'INotifyPropetyChanged-exploration' of https://github.co…

a625548

…m/mapbox/mapbox-unity-sdk into INotifyPropetyChanged-exploration

General section now triggers change events from UI changes. Commented…

3f2af92

… out SetChildProperties call in AbstractMap line 498.

change SetRasterData and SetHeightData methods to reset data when pas…

bd7d97f

…sed null values merged OnErrorOccurred and OnError methods in factories change vector layer visualizer to create new modules iff they do not already exist (work in progress)

Refactored delegate add from Lambda to method.

31c8d3a

Added property.serializedObject.ApplyModifiedProperties() to Property…

21e09f5

…Drawer.UpdateProperty to ensure update map functionality uses up-to-date data.

Added update to Vector section.

fcd9544

Further improvement to VectorLayer update - using changeChecks instea…

0721c6e

…d of GUI.changed

fix a bug where vector layer was redraw twice

5817e34

fix a bug where terrain update wasn't triggering vector update

9bee8b0

fix add/remove/toggle layer issues for vector layer runtime updates

183373f

fix an issue where terrain layer update didn't handle vector layer corrections (snap terrain) correct

Fixed duplicate elevation layer type in terrain Ui; removed Debug sta…

f75320f

…tement from Earcut.cs

Initial tests for passing update arguments down the chain to vector l…

9dce8e1

…ayer feature sub property drawers.

Added enum for vector update type.

e1b3afe

greglemonmapbox added 10 commits August 31, 2018 08:06

Added Dictionary<Type, Action> _updateMethods to VectorLayerVisualize…

0f56612

…r; vector update is now triggered from material/collider option delegates, figures out which logic it needs to run via dictionary lookup, which retrieves funcion.

Editor helper script, useful for working with editor and property dra…

2a88f66

…wer scripts.

Removed variable-specific properties from options files; property dra…

e5e3a7b

…wers now use EditorHelper.GetTargetObjectOfProperty to get class instance, and result check of ApplyModifiedProperties bool value to set HasChanged to true.

Using GetTargetObjectOfProperty to get class instance; replaced featu…

efdb582

…re specific property sets w/ HasChanged = true.

Removed Action<VectorUpdate> action = null argument from DrawUI. Remo…

eecc4d4

…ved Begin/End change check calls, using bool result of ApplyModifiedProperties() to set HasChanged property.

Set _imagery, _terrain, _vectorData.Update layer lamda syntax back to…

28d0633

… previous commit. Changed AbstractLayer.NotifyUpdateLayer and AbstractLayer.UpdateLayerHandler to use bool instead of VectorUpdateType as second argument.

Removed unused enums.

f899ee3

Removed complicated dictionary lookup method, added UpdateVector to a…

6a19db1

…dditional layer delegates.

Added null checks for class instance references derived from EditorHe…

1f7fa72

…lper.GetTargetObjectOfProperty(property) method.

greglemonmapbox requested review from abhishektrip and brnkhy September 5, 2018 23:00

abhishektrip suggested changes Sep 6, 2018

View reviewed changes

greglemonmapbox and others added 9 commits September 6, 2018 13:51

Changed EditorHelper namespace to Mapbox.Editor; removed methods not …

568911b

…related to serialized property work and those referencing external classes; removed dead/commented code. Changed using com.spacepuppyeditor to Mapbox.Editor in relevant classes.

Removed dead/commented code and unessential namespace imports.

a410453

Moved MapboxDataProperty to separate file; corrected namespaces.

1143130

Moved TileTerrainType to MapboxEnums.

c3abf3e

Removed dead code.

3bcc886

Replaced repetitive ApplyModifiedProperties checks with call to Edito…

205c8c3

…rHelper.CheckForModifiedProperty.

removed a line where it was adding frame number to tile names for deb…

0b19ab2

…ugging removed comments from older versions of the ElevatedTerrainStrategy file removed two commented out lines in VectorLayerVisualizer to prevent confusion

remove unused field from TerrainDataFetcherParameters

0d89393

Removed outdated comment.

6f54810

abhishektrip approved these changes Sep 7, 2018

View reviewed changes

abhishektrip merged commit e14b453 into develop Sep 7, 2018

abhishektrip deleted the map-update-develop-merge-fresh branch October 12, 2018 05:32

Map update develop merge fresh #994

Map update develop merge fresh #994

Uh oh!

Conversation

greglemonmapbox commented Sep 5, 2018

Uh oh!

abhishektrip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants