Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Optional map snapshotter camera position #12029

Merged
merged 1 commit into from
May 31, 2018
Merged

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented May 30, 2018

When looking into render differences from android render tests, I was noticing the following behavior:

expected actual output
expected actual output

With using the following style.json:

{
  "version": 8,
  "metadata": {
    "test": {
      "width": 64,
      "height": 64
    }
  },
  "bearing": 45,
  "sources": {},
  "sprite": "local://sprites/emerald",
  "layers": [
    {
      "id": "background",
      "type": "background",
      "paint": {
        "background-pattern": "cemetery_icon"
      }
    }
  ]
}

Was able to track down that our camera definition in the style.json was overwritten by the default camera:

     if (style.first) {	  
         map.getStyle().loadJSON(style.second);	         
     } else{	  
         map.getStyle().loadURL(style.second);	         
     }	    
     map.jumpTo(cameraOptions);

This PR fixes that behavior by using optional<mbgl::CameraOptions> instead of CameraOptions&.

@tobrun tobrun added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels May 30, 2018
@tobrun tobrun added this to the android-v6.2.0 milestone May 30, 2018
@tobrun tobrun self-assigned this May 30, 2018
@tobrun tobrun requested a review from ChrisLoer May 30, 2018 10:23
@ChrisLoer
Copy link
Contributor

This looks reasonable to me, but I think I'm missing something about how it works. It looks to me like the members of CameraOptions are already optionals, the default constructor will leave them empty, and jumpTo uses value_or(current map state) so that it should be a no-op with a default-constructed CameraOptions. Which part of that didn't work? It it something to do with the style loading asynchronously?

Also, how do we have android render tests hooked up to the MapSnapshotter? This is cool, I didn't even know it was a thing we did, but I'm embarrassed I can't find where it happens.

@tobrun
Copy link
Member Author

tobrun commented May 30, 2018

been working on this for a while but was on the backburner (see #10045).

so that it should be a no-op with a default-constructed CameraOptions

The issue is that the style.json has a camera options definition but even without setting a camera position in java the style.json camera gets overridden. It could be that I can fix this on the java/jni side without any core changes, will need to try that out.

@ChrisLoer
Copy link
Contributor

been working on this for a while but was on the backburner (see #10045).

Awesome! That will incidentally be a good stress tester for MapSnapshotter...

Aha, OK I get what's going on now. The camera options in the style end up in the defaultCamera object, and normally onStyleLoaded applies the default camera to the map object, but the earlier (empty) jumpTo call sets cameraMutated = true, and that short-circuits the use of defaultCamera in onStyleLoaded.

It might make sense to only set cameraMutated if something actually changes, but that might also cause some surprising behavior...

@tobrun tobrun merged commit 930d0bc into master May 31, 2018
@tobrun tobrun deleted the tvn-snapshotter-initial-camera branch May 31, 2018 05:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants