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

[core] add raster-dem source type and hillshade layer type #10642

Merged
merged 1 commit into from
Jan 23, 2018

Conversation

mollymerp
Copy link
Contributor

@mollymerp mollymerp commented Dec 6, 2017

@mollymerp mollymerp added feature GL JS parity For feature parity with Mapbox GL JS in progress ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Dec 6, 2017
@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Dec 13, 2017
@mollymerp
Copy link
Contributor Author

@kkaefer so the rendering working ok now, but I'm not sure how to proceed with a couple things

  • to backfill the borders, I'd like to call a function on every tile load, or onTileChanged at the source level so I have access to the full tile pyramid or all the loaded tiles in a source, and I'm just not sure where to put that. I hacked it inRenderSource::onTileChanged but that does not seem like an ideal solution. If there isn't a rendersource-specific listener that is invoked when a tile loads, do you have an idea of how/where that would go?

  • there is a lot of code duplication between the various RasterSource and RasterDEMSource classes and I'm not sure if I need to use templates or simply inheritance. I (briefly) tried to use inheritance on RasterDEMTile inheriting from RasterTile but I had trouble appeasing the compiler with the correct use/removal of final override etc.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

For Source types
The only difference between the RasterSource and RasterDEMSource types is the SourceType. VectorSource also seems to differ by only SourceType

You could have RasterDEMSource inherit from RasterSource, remove RasterDEMSource::Impl, and have the RasterSource::Impl constructor take SourceType as a parameter.

Alternatively, all of the tileset-based sources could derive from a new TilesetSource (or other better name) base class and a common TilesetSource::Impl.

For Tile Types
This one's a bit harder. RasterDEMTileWorker can be removed in favor of RasterTileWorker<class Tile, class Bucket>, but that doesn't remove the duplication of the tile types. Templatizing on the bucket type could help resolve this, but I'm not sure if this would be a real improvement.

ActorRef<RasterDEMTile>(*this, mailbox)) {

const CanonicalTileID canonical = id_.canonical;
const u_int32_t dim = pow(2, canonical.z);
Copy link
Contributor

Choose a reason for hiding this comment

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

The u_int32_t type is non-standard, use uint32_t instead.

void setMetadata(optional<Timestamp> modified, optional<Timestamp> expires);
void setData(std::shared_ptr<const std::string> data);

void cancel() override;
Copy link
Contributor

@asheemmamoowala asheemmamoowala Jan 3, 2018

Choose a reason for hiding this comment

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

Give Tile::cancel an empty implementation so this override and the one in RasterTile are not needed.

@@ -55,6 +55,51 @@ void RenderRasterDEMSource::update(Immutable<style::Source::Impl> baseImpl_,
[&] (const OverscaledTileID& tileID) {
return std::make_unique<RasterDEMTile>(tileID, parameters, *tileset);
});
auto rendertiles = getRenderTiles();
Copy link
Contributor

Choose a reason for hiding this comment

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

getRenderTiles() returns only the RenderTiles that are about to be rendered. This excludes the previously loaded tiles that may have been cached in TilePyramid.cache.

Iterating all the tiles on each update seems unnecessary.

Alternatively:

  • Overload onTileLoaded/onTileChanged in RenderRasterDEMSource (may require changes in RenderSource).
  • When a tile is loaded query the TilePyramid for its specific neighbors and backfill both. Ensue that the query includes both the tiles map and the TileCache
  • Change TileCache to access the cached tile directly using something like Tile* get(OverscaledTileID& key) const. Change the existing get method to pop

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 in 0f0aaf1 and 85818c8

@mollymerp mollymerp force-pushed the raster-hillshade branch 2 times, most recently from 7c1493c to a02cb6c Compare January 9, 2018 23:28

// update the bitmasks to indicate that these tiles have been backfilled by flipping the relevant bit
demtile.neighboringTiles = demtile.neighboringTiles | mask;
borderTile.neighboringTiles = borderTile.neighboringTiles | mask;
Copy link
Contributor

Choose a reason for hiding this comment

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

The mask for borderTile needs to be computed correctly. i.e. I am the downstairs neighbor of my upstairs neighbor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♀️

const RasterDEMTile& borderTile = static_cast<RasterDEMTile&>(renderableNeighbor->get().tile);
RasterDEMTile& demtile = static_cast<RasterDEMTile&>(tile);

if (tile.isRenderable() && demtile.neighboringTiles != 0b11111111) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an enum to define the neighbors and some of the helpful constants:

enum class DEMTileNeighbors  {
Empty: 0
TopLeft: 1 << 1
Top: 1 << 2,
TopRight: 1 << 3,
...,
Complete: 0b11111111
BottomMost: Empty | BottomRight | BottomLeft | Bottom
}

@@ -20,31 +20,6 @@ RasterDEMTile::RasterDEMTile(const OverscaledTileID& id_,
mailbox(std::make_shared<Mailbox>(*Scheduler::GetCurrent())),
worker(parameters.workerScheduler,
ActorRef<RasterDEMTile>(*this, mailbox)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For tiles on the bottom edge or top edge, consider updating neighboring tiles to mark those neighbors as backfilled.

Tile* TileCache::get(const OverscaledTileID& key) {
auto it = tiles.find(key);
if (it != tiles.end()) {
return std::move(it->second.get());
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to move

@@ -84,7 +84,7 @@ class RenderSource : protected TileObserver {

bool enabled = false;

virtual void onTileChanged(Tile&);
void onTileChanged(Tile&);
Copy link
Contributor

Choose a reason for hiding this comment

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

add override

}

public:
const int32_t dim;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spell out dimension or change to size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm its dim in JS too...

border(border_),
stride(dim + 2 * border),
image({ static_cast<uint32_t>(dim + 2 * border),
static_cast<uint32_t>(dim + 2 * border) }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use stride to init image

for (const RenderTile& tile : renderTiles) {
assert(dynamic_cast<HillshadeBucket*>(tile.tile.getBucket(*baseImpl)));
HillshadeBucket& bucket = *reinterpret_cast<HillshadeBucket*>(tile.tile.getBucket(*baseImpl));

Copy link
Contributor

Choose a reason for hiding this comment

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

nit empty line

}

void RenderRasterDEMSource::onTileChanged(Tile& tile){
RasterDEMTile& demtile = static_cast<RasterDEMTile&>(tile);

std::map<DEMTileNeighbors, DEMTileNeighbors> opposites = {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


if ( id.canonical.y == 0 ){
// this tile doesn't have upper neighboring tiles so marked those as backfilled
neighboringTiles = neighboringTiles | 0b00011100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be safer to use the enum values here. Or create an enum value for this constant.

BottomRight = 1 << 7
};

inline unsigned char operator|(unsigned char a, DEMTileNeighbors b) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Define RasterDEMTile::neighboringTiles of type DEMTileNeighbors. Then these operator overrides can uniformly use DEMTileNeighbors.

@@ -6,6 +6,38 @@

namespace mbgl {

enum class DEMTileNeighbors {
Copy link
Contributor

Choose a reason for hiding this comment

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

The underlying type of a C++ enum is implementation-defined. It may not choose the smallest integral type to fit all the values. We can be more specific by providing a type enum class DEMTileNeighbors: unsigned char


std::vector<OverscaledTileID> neighbors = {
// left and right neighbor
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use a comment here to indicate that the order of the tile ids defined here must be kept in sync with the order of enum values defined in DEMTileNeighbors.

Alternatively, you can remove this vector and add a lambda function to dynamically compute the id in each iteration of the for-loop.

@@ -23,12 +23,12 @@ RasterDEMTile::RasterDEMTile(const OverscaledTileID& id_,

if ( id.canonical.y == 0 ){
// this tile doesn't have upper neighboring tiles so marked those as backfilled
neighboringTiles = neighboringTiles | 0b00011100;
neighboringTiles = neighboringTiles | DEMTileNeighbors::NoUpper;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Empty = 0 << 1,

// the order of the neighbors in this enum (or rather the order of their flipped bits
// must stay the same as
Copy link
Contributor

Choose a reason for hiding this comment

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

Incomplete comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

derp – yeah I think I decided to use a lambda while writing this

@@ -47,7 +48,7 @@ class DEMPyramid {
bool isLoaded() {
return loaded;
};
std::vector<Level> levels;
std::unique_ptr<Level> level;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that DEMPyramid doesn't account for multiple levels, change it to DEMData to match gl-js, and fold in the Level class into DEMData.

As part of folding in the Level class, also consider:

  • using a getter method for image. You'll have to return as a const reference.
  • defining the dim, stride, and border members as const. They are assigned to in the constructor and shouldn't be changed after.

As a bonus, this will eliminate the need to zero out the image data in the Level constructor.

}
dem = context.createTexture(pyramid.level->image);
auto& image = pyramid.level->image;
context.updateTexture(*dem, image);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need to update the texture right after creation.


TileMask mask{ { 0, 0, 0 } };
bool prepared = false;
DEMPyramid* getDEMPyramid();
Copy link
Contributor

Choose a reason for hiding this comment

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

Return const reference instead of pointer.

@mollymerp
Copy link
Contributor Author

mollymerp commented Jan 16, 2018

@kkaefer the remaining tasks on this PR as I see it are:

image

The first two items should be straightforward and this should be ready for a more in-depth review after those are completed. Debugging the backfill issue is proving to be tricky, but I hope to track it down soon.

@mollymerp
Copy link
Contributor Author

I have taken care of the last three issues on my list above, and I will work on code-duplication-reduction quest today.

Thank you @asheemmamoowala for your super helpful continuous feedback and code reviews over the past week or so! Can you confirm that a724ce0 covers what you were suggesting to me in chat?

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

circle.yml Outdated
filters:
tags:
only: /node-.*/
#- ios-sanitize-thread
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to remove these temporary changes when ready


class DEMData {
public:
class Level {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need for the inner Level class anymore? Can DEMData own the image and other members directly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in #10964

@@ -289,7 +290,7 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
RenderLayer* layer = getRenderLayer(layerImpl->id);
assert(layer);

if (!parameters.staticData.has3D && layer->is<RenderFillExtrusionLayer>()) {
if ((!parameters.staticData.has3D && layer->is<RenderFillExtrusionLayer>()) || (!parameters.staticData.has3D && layer->is<RenderHillshadeLayer>())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified to !has3D && (is<Fill> || is<Hillshade>)

if (renderableNeighbor != nullptr && renderableNeighbor->isRenderable()) {
RasterDEMTile& borderTile = static_cast<RasterDEMTile&>(*renderableNeighbor);
demtile.backfillBorder(borderTile, mask);
borderTile.backfillBorder(demtile, opposites[mask]);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a rare situation where borderTile has already been back-filled with a previous instance of demTile.
A tile might be loaded, then eventually evicted from the cache. Next time it is loaded, it may attempt to back-fill a bordering tile for a second time. This can be guarded against in RasterDEMTile::backfillBorder


namespace mbgl {

DEMData::DEMData(PremultipliedImage& image):
Copy link
Member

Choose a reason for hiding this comment

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

This object should either get a &&, or a const &. You're currently passing in a modifiable reference, which in most cases isn't what we want because it means that this constructor can modify the image object, but doesn't take ownership of it. Looking at the code below, we don't seem to modify any of it, so using const & should work. The other option would be to use && and move image into this function/constructor. This would potentially allow the constructor to take ownership of the object.


DEMData::DEMData(PremultipliedImage& image):
level(image.size.height, std::max<int32_t>(std::ceil(image.size.height / 2), 1)){
assert(image.size.height == image.size.width);
Copy link
Member

Choose a reason for hiding this comment

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

assert is checked only when we compile in Debug mode (and it crashes the app if the assertion fails). Since this data is originally from the image we download from the internet (which could be any size), we should always validate that the size is correct, and throw an exception otherwise (and make sure that the exception is handled elsewhere!)

for (int32_t x = 0; x < level.dim; x++) {
const int32_t i = y * level.dim + x;
const int32_t j = i * 4;
level.set(x, y, (image.data[j] * 256 * 256 + image.data[j+1] * 256 + image.data[j+2])/10 - 10000);
Copy link
Member

Choose a reason for hiding this comment

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

In Release mode, this could lead to undefined behavior/memory access out of bounds in the following scenario:

  • assert isn't run (see above)
  • level.dim is larger than the image's height
  • computing the offset produces a number that is larger than the number of bytes in image.data

It looks like you're making assumptions about the image width having to be exactly 256 pixels, but it also seems that this isn't checked in the constructor. We should add a check and throw an exception if that isn't the case (probably combine with assertion above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I'm assuming the tile is 256px; the 256 in that line comes from the terrain-rgb decoding formula

Copy link
Member

Choose a reason for hiding this comment

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

You’re right 🤦‍♂️
However the comment about assert still holds true

loaded = true;
}

void DEMData::backfillBorder(const DEMData& borderTileData, int8_t dx, int8_t dy) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a few code comments to this function explaining what it does? We're using a few abbreviations here that are a bit unclear.

assert(x >= -border);
assert(x < dim + border);
assert(y >= -border);
assert(y < dim + border);
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, these assertions aren't going to be run for Release builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's ok in this case – per your comments in chat, these conditions should never be false under any circumstances given how this function is used and how dim and border are determined.

Copy link
Member

Choose a reason for hiding this comment

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

👍


if (!bucket.hasData())
continue;
if (!bucket.isPrepared() && parameters.pass == RenderPass::Pass3D) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we pull out the RenderPass check to avoid looping over all of these tiles in the first place if the pass isn't 3D?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -153,3 +153,4 @@ void RenderRasterLayer::render(PaintParameters& parameters, RenderSource* source
}

} // namespace mbgl

Copy link
Member

Choose a reason for hiding this comment

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

Nit: additional whitespace

tileURLTemplates = tileset->tiles;

// TODO: this removes existing buckets, and will cause flickering.
// Should instead refresh tile data in place.
Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be addressed ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is copied directly from RenderRasterSource so I guess it hasn't been addressed there either 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should address both in a follow up PR?

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, yeah, let's do this in a follow up PR if this is even still true.

class TileParameters;
class HillshadeBucket;

enum class DEMTileNeighbors : unsigned char {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can we use uint8_t to be in line with how we use this type in other parts of the codebase?

}
}

std::unique_ptr<Tile> TileCache::pop(const OverscaledTileID& key) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you check other uses of ::pop to make sure we're not accidentally changing existing behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I replaced all preexisting calls to TileCache::get(old name for this pop function) with TileCache::pop and TileCache::get is only used to backfill raster-dem tiles

@halset
Copy link
Contributor

halset commented Jan 19, 2018

I tried ca7203e on iOS and it crashes at z16 in ./src/mbgl/renderer/sources/render_raster_dem_source.cpp:81

        const uint16_t dim = std::pow(2, canonical.z);
        const uint32_t px = (canonical.x - 1 + dim) % dim;

When doing 2^16 and handling that as a 16 bit, it looks like the result dim is 0. At the next line there is a divide by this same 0.

@mollymerp mollymerp force-pushed the raster-hillshade branch 2 times, most recently from d329da8 to e648e72 Compare January 20, 2018 00:50
@mollymerp
Copy link
Contributor Author

oooh thanks for the catch @halset !!

@mollymerp mollymerp force-pushed the raster-hillshade branch 5 times, most recently from 4accffb to eddcad8 Compare January 22, 2018 18:12
@@ -190,6 +190,7 @@ In style JSON | In the SDK
`circle` | `MGLCircleStyleLayer`
`fill` | `MGLFillStyleLayer`
`fill-extrusion` | `MGLFillExtrusionStyleLayer`
`hillshade` | `MGLHillshadeStyleLayer`
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, darwin-style-code is unable to automate the entire process of adding a new layer type, so some changes are still needed before this layer type can be used on iOS and macOS:

This can be tail work if necessary, since the builds are passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the heads up @1ec5 – I would prefer to do this in a follow up PR because this is already a giant set of changes if thats ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ticketed #10991

@mollymerp
Copy link
Contributor Author

@kkaefer @asheemmamoowala Unless there are any other changes you'd like me to make, I think this just needs the final ✅ 🙃

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

All systems go 🚀

dx -= dim;
}
}
const HillshadeBucket* borderBucket = borderTile.getBucket();
Copy link
Member

Choose a reason for hiding this comment

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

getBucket() can return nullptr if we have received a tile that is invalid or empty. In this case, the worker code eventually calls RasterDEMTile::onError or RasterDEMTile::onParsed respectively. The former doesn't explicitly set the bucket pointer, which means that it'll remain empty. The latter explicitly sets a nullptr (via std::move).

We should guard accessing this code, and just don't backfill for this particular tile that is missing.

add DEMPyramid class

add raster-dem source and hillshade layer types

update with layer-specific lighting properties

fix build errors

use 3dpass for hillshade prepare step

fix DEM population, change border to int32

update shaders and fix rendering

update gl-js sha and un-ignore hillshade render tests

start backfill border logic

remove unused dem pyramid code

implement backfill border

use bitmask to keep track of backfilled neighbors

fix tests

TileCache::get -> TileCache::pop, add new getter

fix node build

remove unnecessary std::move

override/final onTileChanged

update hillshade shaders

android style code

update hillshade shaders

appease clang-tidy

address review comments

use enum for neighboring tiles

enforce+update DEMTileNeighbors type

add RasterDEMTile test

clean up DEMPyramid

define constructor for DEMPyramid

test DEMPyramid

DEMPyramid->DEMData

return reference to DEMData instead of pointer

refactor backfillBorder owner

fix texture filter and wrap settings

remove hardcoded tilesizes and allow for @2x tile requests

address review comments

make RasterDEMSource inherit from RasterSource (#10952)

comment DEMData::backfillBorder

guard against redoing backfill work for the borderTile

add RasterDEMSource tests

reenable all builds

ios codegen

remove Level class (#10964)

cleanup and add hillshade to android style layer exceptions

dim as uint32

guard against nullptr borderBucket
@mollymerp mollymerp removed in progress ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jan 23, 2018
@mollymerp mollymerp merged commit f329420 into master Jan 23, 2018
@jfirebaugh jfirebaugh deleted the raster-hillshade branch July 27, 2018 22:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl feature GL JS parity For feature parity with Mapbox GL JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants