Skip to content
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.

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

min_display_zoom & max_display_zoom #944

Merged
merged 12 commits into from Sep 8, 2016
Merged

Conversation

@hallahan
Copy link
Contributor

@hallahan hallahan commented Aug 30, 2016

This PR allows you to set a min_zoom parameter in your sources block of your scene.yaml.

When a source is given a min_zoom parameter, it does not fetch tiles below that min zoom. This includes the raster sub-sources we see with normals, as found in Walkabout and other terrain styles.

As @bcamper mentioned, we might actually want to call this parameter min_display_zoom, since we don't under-zoom data at lower zooms--we simply don't provide it.

I'm happy to re-factor that into this PR. Also, we could take a similar approach found here to implement max_display_zoom.

tangrams/tangram#394

Thinking about also implementing bounds, we could build that into DataSource#isActiveForZoom, calling it isActive instead.

cc/ @tallytalwar @blair1618 @bcamper

@hallahan hallahan changed the title Min Zoom min_display_zoom & max_display_zoom Sep 6, 2016
@hallahan
Copy link
Contributor Author

@hallahan hallahan commented Sep 6, 2016

At this point, I've refactored min_zoom to be min_display_zoom. I have also implemented max_display_zoom.

@hallahan
Copy link
Contributor Author

@hallahan hallahan commented Sep 6, 2016

Interesting, Travis caught that Android was missing the #define for INT32_MAX. Perhaps I should just #ifndef in the value to the token in the header?

@tallytalwar
Copy link
Member

@tallytalwar tallytalwar commented Sep 6, 2016

Can you rebase this with master? some of the commits you have above have already been merged!

@@ -23,7 +23,8 @@ class ClientGeoJsonSource : public DataSource {

public:

ClientGeoJsonSource(const std::string& _name, const std::string& _url, int32_t _maxZoom = 18);
ClientGeoJsonSource(const std::string& _name, const std::string& _url,
int32_t _minDisplayZoom = 0, int32_t _maxDisplayZoom = INT32_MAX, int32_t _maxZoom = 18);

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

I think its fine to use 18 as the maxDisplayZoom default instead of INT32_MAX.

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

Or a default value of -1 can be used to ignore any pruning of tiles based on min or max display zooms.

This comment has been minimized.

@hallahan

hallahan Sep 6, 2016
Author Contributor

Doesn't the map actually proxy tile higher zooms than 18? What if we wanted to zoom to 28 and explore a map of the stores in a mall or something?

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

Yup in that case what I suggested to use is -1. Similar to how js does it and has a null as default.

@@ -143,6 +145,7 @@ void DataSource::constructURL(const TileID& _tileCoord, std::string& _url) const
bool DataSource::equals(const DataSource& other) const {
if (m_name != other.m_name) { return false; }
if (m_urlTemplate != other.m_urlTemplate) { return false; }
if (m_minDisplayZoom != other.m_minDisplayZoom) { return false; }

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

Missing check for m_maxDisplayZoom for DataSource equality.

This comment has been minimized.

@hallahan

hallahan Sep 6, 2016
Author Contributor

Done via 108ab3c

@@ -69,8 +74,13 @@ class DataSource : public std::enable_shared_from_this<DataSource> {
/* Generation ID of DataSource state (incremented for each update, e.g. on clearData()) */
int64_t generation() const { return m_generation; }

int32_t minZoom() const { return m_minDisplayZoom; }

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

Probably better to rename the accessor to minDisplayZoom to avoid any confusion.

@@ -105,6 +115,12 @@ class DataSource : public std::enable_shared_from_this<DataSource> {
// Name used to identify this source in the style sheet
std::string m_name;

// Minimum zoom for which tiles will be displayed
int32_t m_minDisplayZoom;

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

nitpick, but why not int16_t?

This comment has been minimized.

@hallahan

hallahan Sep 6, 2016
Author Contributor

I can switch m_minDisplayZoom to be int16_t, however, that would be inconsistent with m_maxZoom which has always been int32_t. We really could have all these zoom vars be int8_t (max value of 127)

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

Ohh I forgot about these. You can leave it 32 bit right now. Though I think all these can be 8 bit but I might have to look as why we had it as 32 in the beginning.

This comment has been minimized.

@hallahan

hallahan Sep 6, 2016
Author Contributor

👍

subTasks.insert(it, subTask);
// If the subSource isn't active for the zoom, we should just
// load an empty texture and move on.
rasterSubSource->loadEmptyTexture(std::move(subTask));

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

This might result in some weird artifacts (black tiles), if the style sheet is using some wrong combination of min/max display zooms.
Does it not make sense to include a subSource's min/max display zoom limits also in its main source? This will result in main source's tile not building if the sub/child source tile is not "available" for that display zoom.

cc. @bcamper

This comment has been minimized.

@bcamper

bcamper Sep 6, 2016
Member

Yes, after discussing this I amended the logic to limit the "parent" source by any attached raster source's min/max. So only tiles in the intersection of min/max zoom range (and/or bounding box) would be built.

This comment has been minimized.

@hallahan

hallahan Sep 6, 2016
Author Contributor

When I was implementing this, I was trying things out with Walkabout. My thought was that our normals may not cover a given layer. For example, what if we had normals of zooms 8 - 12, but we wanted to render vectors with 0 - 18? It looks like it works fine as-is--the hillshades just don't get drawn when the sub source doesn't load.

This comment has been minimized.

@bcamper

bcamper Sep 6, 2016
Member

Seems like what you may want to do in that case is attach a raster source with a lower max_zoom value, and the raster texture will be scaled up at the higher zooms, whereas the max_display_zoom parameter was meant to really signify you don't want this tile source rendering at a higher zoom level. In our current styles like Walkabout, we set the terrain normals max_zoom: 15, and let them be scaled up onto vectors up to z20.

I don't know what is happening with the current behavior (@tallytalwar can illuminate), but it may be difficult to ensure correct drawing in all cases where a raster is attached. We have to provide some texture to the GL shader, and with placeholder like a 1x1 pixel image, I would not expect the lighting normal calculations to be correct. You'd want different default textures/behavior for different types of raster attachments (e.g. treating the raster as a direct color texture, vs. treating as a normal map), and there is also the "custom" raster mode where you don't know the intentions of the user.

Am I missing something in the example above, where only the max_zoom needs to be set?

This comment has been minimized.

@tallytalwar

tallytalwar Sep 6, 2016
Member

The reason it won't render the uber time is because it is not "ready" yet. A main tile only becomes ready to be drawn is when all its dependent source tiles are loaded.

Along with what Brett mentioned, there is no point processing a master tile if it's subtIle will never be fetched, we can ignore all such tiles and save on some processing.

@hallahan hallahan force-pushed the hallahan:minZoom branch from 997e28c to 310376e Sep 6, 2016
@hallahan
Copy link
Contributor Author

@hallahan hallahan commented Sep 7, 2016

@tallytalwar - I implemented the proposed changes. Ready for another look. Thanks!

@@ -12,6 +12,26 @@
#import <UIKit/UIKit.h>

This comment has been minimized.

@tallytalwar

tallytalwar Sep 7, 2016
Member

How come these changes are in this PR?

This comment has been minimized.

@hallahan

hallahan Sep 7, 2016
Author Contributor

From my understanding, that's because I did a rebase from tangrams/master to hallahan/minZoom. Git gets kinda weird when dealing with another origin sometimes. It's because I did commits at a point before this iOS work and then rebased yesterday. The nature of rebase is that history gets rewritten, so that commit, as you can see, has a different SHA1. The older problem where we saw old commits that have already been merged is because I branched off of hallahan/master instead of tangrams-es/master. It was up to date, but the fact that one origin merged into another created that effect.

The only way I could resolve this is to make a new branch and cherry pick my commits into a new one and open a new PR. However, I wonder, would it take me out of the commit message once you merge the PR? Since, all of my commits should say it was hallahan with tallytalwar? I can do a new PR if needed. Sorry this is such a headache!

http://stackoverflow.com/questions/32826197/interactive-rebase-after-merging-other-commits-interleaving-mine

This comment has been minimized.

@tallytalwar

tallytalwar Sep 7, 2016
Member

Hmm.. Probably one more thing you can do here is to take this (c159079) commit out.

  • git rebase -i HEAD~12
  • remove the line mentioning this (c159079) commit.
  • save
  • to be sure you are all good, do a diff of this local branch with the remote.

This comment has been minimized.

@tallytalwar

tallytalwar Sep 7, 2016
Member

I also plan to "squash and merge" this PR, so it should say "hallahan with tallytalwar" when merged.

This comment has been minimized.

@hallahan

hallahan Sep 7, 2016
Author Contributor

Hmm, when I delete c159079 in the interactive rebase, I get conflicts for each commit. When I resolve each, I then diff it with tangrams master, and it removes all of the iOS code. Continuing to look into this...

This comment has been minimized.

@hallahan

hallahan Sep 7, 2016
Author Contributor

OK, after looking further into it. I don't think there is a solution to removing cc159079 from the rebased branch without actually removing the diff that that commit applies.

I went ahead and made a new branch and applied a diff of the minZoom branch.

https://github.com/hallahan/tangram-es/tree/display_zoom

We can a) open a new PR with that, or b) just merge this PR as-is.

Sorry for the weird rebase.

@@ -172,6 +173,11 @@ bool RasterSource::loadTileData(std::shared_ptr<TileTask>&& _task, TileTaskCb _c
return status;
}

void RasterSource::loadEmptyTexture(std::shared_ptr<TileTask>&& _task) {

This comment has been minimized.

@tallytalwar

tallytalwar Sep 7, 2016
Member

This is not required now.

This comment has been minimized.

@hallahan

hallahan Sep 7, 2016
Author Contributor

Done via 2c778a2

@@ -9,6 +9,7 @@
#include "glm/gtx/norm.hpp"

#include <algorithm>
#include <data/rasterSource.h>

This comment has been minimized.

@tallytalwar

tallytalwar Sep 7, 2016
Member

We can remove this include now, since loadEmptyTexture is not required now.

This comment has been minimized.

@hallahan

hallahan Sep 7, 2016
Author Contributor

Done via 2c778a2

@tallytalwar
Copy link
Member

@tallytalwar tallytalwar commented Sep 7, 2016

Changes look good, once the comments are addressed and propertly rebased with master will merge.

Thanks @hallahan

msmollin and others added 12 commits Sep 2, 2016
* Implement camera handling APIs in objective-c

Includes tilt, rotation, zoom, and position management, as well as camera type switching. Fixes #849, #850, #851, #858, #897

* Updated for consistency with android. Rebased against master

* Re-set continuous rendering to be explicitly false.

* Update ease type defaults, add animation functions for camera tilt.

* Rename cameraTilt -> tilt

* Tweak camera API names for consistency
@hallahan hallahan force-pushed the hallahan:minZoom branch from 2c778a2 to aeefe08 Sep 7, 2016
@tallytalwar
Copy link
Member

@tallytalwar tallytalwar commented Sep 7, 2016

This is good to go, merging when travis goes green.

@tallytalwar tallytalwar merged commit 94a0ac2 into tangrams:master Sep 8, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.