mapbox / mapbox-unity-sdk

Mapbox Unity SDK - https://www.mapbox.com/unity/
Other
723 stars 211 forks source link

Multiple failures in GenerateTerrainMesh #759

Closed Naphier closed 6 years ago

Naphier commented 6 years ago

At the bottom of the attached editor log you will see ArgumentOutOfRangeException: Argument is out of range. in Mapbox.Unity.MeshGeneration.Factories.TerrainFactory.GenerateTerrainMesh and Not allowed to access vertices on mesh 'Combined Mesh (root: Static Root 0) Instance' (isReadable is false; Read/Write must be enabled in import settings)

The error happens oftentimes when a new tileset is loaded and prevents y snapping to zero from working properly on some of the generated tiles. On a second attempt to load the map, all is OK if it pulls the data from the cache.

We are using zoom level of 18 and were not seeing this issue at zoom level 17.

We're using _worldRelativeScale = Mathf.Cos(Mathf.Deg2Rad * (float)_centerLatitudeLongitude.x) for a 1:1 scale in Unity.

I'm guessing that this is a timeout issue with the vector tile data download as it doesn't seem to occur on machines with solid internet connection. It seems to occur more on lower tiered ISP connections.

Unfortunately, it is not easy to reproduce, but I'm hoping you folks may have some insights on how to improve. I do think it's surrounding timeouts and retries.

Thanks!

abhishektrip commented 6 years ago

@Naphier This error is sounds related to Unity's static mesh batching. Do you have static batching turned on? Can you try without that ?

abhishektrip commented 6 years ago

@Naphier Let us know if you are still seeing the issue. closing this ticket

Naphier commented 6 years ago

Yes turning off static batching prevents this issue, but we need to static batch this to improve performance on mobile. So apparently even though the tiles are all created, they're still not ready for static batching. At what point in the generation process is it safe to start static batching?

We were using OnMapVisualizerStateChanged listener at ModuleState.Finished then waiting 2 seconds before static batching, but OnMapVisualizerStateChanged was not always firing. So we then did a manual hack to check how many children of the map root has been created then firing off the final static batching sequence (after waiting 2 seconds). What is the safest way to determine when all requests have actually reported back as it seems that OnMapVisualizerStateChanged is not stable, and checking the created children doesn't necessarily mean that the meshes have all been written.

Naphier commented 6 years ago

@atripathi-mb just tagging you as I'm not sure if you got a notification on my last message since the issue was closed. Awaiting your response.

abhishektrip commented 6 years ago

@Naphier We fixed some bugs with Visualizer ModuleState in v1.4.0 and later releases. Since you are using v1.3.0 , I would suggest either updating to v1.4+ (we are in the process of releasing v1.4.3 in a few days.) or posting changes related to ModuleState to your project.

Your use -case is something that we are working to better handle in subsequent releases. We plan to provide hooks for users to know deterministically about certain states. Hope this helps

Naphier commented 6 years ago

@atripathi-mb Thanks. It might take some time before we have the opportunity to update. I'll report back when we do.

Naphier commented 6 years ago

Well, I moved up to 1.4.3 today and this still seems problematic. On one run I got the visualizer state change messages repeated twice. Next run they repeated about 300 times. So there's still no way to really tell when the mapbox processes have all finished. I'm simply subscribing to AbstractMap.MapVisualizer.OnMapVisualizerChanged

using UnityEngine;
using Mapbox.Unity.Map;

public class TestCallbacks : MonoBehaviour {
    public AbstractMap abstractMap;

    void Awake()
    {
        abstractMap.OnInitialized += AbstractMap_OnInitialized;
    }

    private void Start()
    {
        abstractMap.Initialize(new Mapbox.Utils.Vector2d(43.19, -109.1), 17);
    }

    private void AbstractMap_OnInitialized()
    {
        abstractMap.MapVisualizer.OnMapVisualizerStateChanged += MapVisualizer_OnMapVisualizerStateChanged;
    }

    private void MapVisualizer_OnMapVisualizerStateChanged(ModuleState obj)
    {
        Debug.Log("State: " + obj);
    }
}

Zoom at 17, ranges all at 5 Lat/lng = 43.19, -109.1

So... it's like it's calling this on EVERY tile created. I just need to know when it starts and when it finishes with everything so that we can do static batching and some other events that happen. What am I missing?

Naphier commented 6 years ago

@atripathi-mb OK, so I'm going to have to revert for now. The main reason we updated was to get more reliable callbacks when the map was completed. It looks like this is still not possible and I need to manually iterate over the created UnityTiles and check their individual states to see if the map is ready. Bummer because we would like to use the new building styling. However, the map's X,Y, and Z position appear to be inconsistent with previous versions of mapbox and the scale is different. In the Mapbox v1.4.3: We're using MapScalingAtWorldScaleStrategy where the scaleFactor would be computed as 1 and snap to Y zero is on. Inspector for AbstractMap: abstractmap_01 abstractmap_02

As you can see, this results in elevation of the map being inconsistent with previous versions as well as the X/Z position being a bit off. (buildings placed with our own tool - relies on Unity world transform) buildings_offground

In our old code under Mapbox v1.3.0 this is what the scene should look like: buildings_fine

Here's our implementation of AbstractMap that we were using in Mapbox 1.3.0 https://gist.github.com/Naphier/8828d7957e2fea41411b102150b6f8df

Also, I've noted that sometimes the mapbox generated buildings are floating above the ground too in Mapbox 1.4.3.

So.... unfortunately we can't update mapbox. Seems every time we do there's a nightmare of headaches and inconsistencies between positioning from version to version. I'd highly suggest some test procedures for this. Create a scene with some manually placed POIs for a specific map at world scale and check that the position relative to the generated mapbox map does not change. Otherwise we'll all be banging our heads against the wall and avoiding updates.

Thanks!

jordy-isaac commented 6 years ago

@Naphier Thanks for following up. Regarding the map position we noticed a few things about your settings. In v 1.4 the default setting for Placement Options is At Location Center where as in 1.3 it was At Tile Center. You might want to adjust your settings to what it was in 1.3.

screen shot 2018-07-31 at 11 35 34 am

Also, is there a reason why you have Low Poly Terrain specified? Although this setting give you lower profile geometry, it is more resource intensive.

abhishektrip commented 6 years ago

@Naphier

  1. For to get notified about MapVisualizer finished state, here is an example of how we use it in the SDK https://github.com/mapbox/mapbox-unity-sdk/blob/d7d944ce64c9494cb76d68b889d9a6d2b5aeb8d5/sdkproject/Assets/Mapbox/Examples/Scripts/LoadingPanelController.cs#L29

We are in the process of creating simpler and more robust events that you can subscribe to for use-cases such as yours.

  1. Static batching - Would merging all the meshes help? Have you tried using the Group features setting on vector layers?

(buildings placed with our own tool - relies on Unity world transform) We have convenience methods to help you place object on the map Lat Long to Unity -> https://github.com/mapbox/mapbox-unity-sdk/blob/d7d944ce64c9494cb76d68b889d9a6d2b5aeb8d5/sdkproject/Assets/Mapbox/Unity/Map/AbstractMap.cs#L635

Unity to Lat Long -> https://github.com/mapbox/mapbox-unity-sdk/blob/d7d944ce64c9494cb76d68b889d9a6d2b5aeb8d5/sdkproject/Assets/Mapbox/Unity/Map/AbstractMap.cs#L653

unfortunately we can't update mapbox. Seems every time we do there's a nightmare of headaches and inconsistencies between positioning from version to version. I'd highly suggest some test procedures for this. Create a scene with some manually placed POIs for a specific map at world scale and check that the position relative to the generated mapbox map does not change. Otherwise we'll all be banging our heads against the wall and avoiding updates.

We do thorough testing before every release. Positioning of features have not changed between versions, some defaults have changed, which might need to be correctly setup.

Let us know any questions/concerns you have with upgrading. We will be happy to get you onboarded to our latest SDK version since it has more functionality and is much simpler to use. 😃

Naphier commented 6 years ago

@atripathi-mb that's the same thing I'm doing in the code sample above. It reports "finished" 300+ times. While position of features haven't changed, as jordy pointed out (thank you @jordy-isaac ) the placement option may be the issue. I'll need to check again.

At any rate, visualizer.OnMapVisualizerStateChanged fires way too much to be useful to know when the entire map is done. When I get a chance to retry updating I'll rewrite my method for getting the actual finished state and send in a PR. It was simple and I'm surprised it's not done on your side yet. I only ditched it because of your claim that the callback was coming and would work.

abhishektrip commented 6 years ago

@Naphier Please put up a PR when you get a chance. PR's from the community are always welcome.

We are evaluating all such callbacks/hooks/API calls that users like you will require and will be putting them out in future releases. I understand your frustration but we are doing a bigger system-wide change which unfortunately takes a few release cycles.

Naphier commented 6 years ago

@atripathi-mb OK, so @jordy-isaac suggestion on changing the placement option to Tile Center seems to have worked. However, the snap to Y ends up being different than previous versions still. It's off by only about 5 meters (Unity units), but nonetheless that messes up a lot of stuff. What I'd like to understand is the snap Y to zero setting putting the terrain at the correct real-world elevation (i.e. 1m real world elevation is 1 unity unit), or is it messing that up? Thanks.