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

[not ready] client side hillshading with terrain-rgb raster tiles #4701

Closed
wants to merge 32 commits into from

Conversation

mollymerp
Copy link
Contributor

still very very rough here. will update this ticket soon w the to-do list before this is ready for review.

🗻

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@andrewharvey
Copy link
Collaborator

Would it make more sense to call the layer type hillshade? I think we should try to use terminology which fits in with future 3D terrain support. I've proposed some ideas about terminology over at #1489 (comment). Over there I've proposed using terrain as the source type for terrain-rgb tiles, and elevation as the layer type for 3D surface.

@mollymerp
Copy link
Contributor Author

mollymerp commented May 23, 2017

Here's a rough list of what needs to happen before this can be merged into master:

  • move mip map level creation + resampling to a worker thread
  • fix tile seams with better design for backfilling tile borders
  • get blending to work with other layers
  • clean up shader debug code
  • implement parent tile fading?
  • reduce shader attribute counts for compatibility with native
  • find way to prevent seams flashing on zoom-out
  • implement light to behave consistently with extrusions
  • finalize layer+source properties in the style spec [style-spec] client-side hill-shading source/layer/properties #4711
  • fix undefined behavior in shaders causing artifacts with some GPUs
  • figure out why only one world is being rendered
  • fix tile overlapping causing flashes of darker tilles

@kkaefer
Copy link
Contributor

kkaefer commented Jun 13, 2017

We could move the preparation step for terrain textures to the beginning of a frame to improve rendering performance.

Right now, the workflow of a frame is like this:

  • (main framebuffer is bound)
  • Clear main framebuffer (✅ first operation is clear -> no restore)
  • Render some layers
  • Render terrain layer
    • For non-preprocessed tiles
      • Bind terrain framebuffer
      • Clear terrain framebuffer (✅ first operation is clear -> no restore)
      • Draw prepare step
      • Bind main framebuffer
    • Draw preprocessed terrain tiles
  • Draw other layers (:warning: FBO restore)

When binding a framebuffer and rendering operations on it, the GPU has to restore the contents of the framebuffer. The driver can optimize that when we do a clear as the first drawing operation (since that discards the contents of the framebuffer). Therefore, drawing to the main framebuffer, then binding a different framebuffer, and rebinding the main framebuffer triggers a framebuffer load on the first draw call.

Instead, we could do all operations that require binding a separate framebuffer (computing the preprocessed textures) as the first thing when rendering a frame (i.e. before calling clear on the main framebuffer) and have the following order:

  • (main framebuffer is bound)
  • Preprocess all non-preprocessed terrain tiles (if any)
    • Bind terrain framebuffer
    • Clear terrain framebuffer (✅ first operation is clear -> no restore)
    • Draw prepare step
    • Bind main framebuffer
  • Clear main framebuffer (✅ first operation is clear -> no restore)
  • Render some layers
  • Render terrain layer
    • Draw preprocessed terrain tiles
  • Draw other layers

@mollymerp mollymerp force-pushed the terrain-rgb-rendering branch 3 times, most recently from add7681 to 3efe4a8 Compare June 29, 2017 18:21
Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

It looks like there is still a lot of code for supporting the openness visualization that we can remove.

@@ -56,11 +56,11 @@
"line": 74
},
{
"message": "layers[12].type: expected one of [fill, line, symbol, circle, fill-extrusion, raster, background], invalid found",
"message": "layers[12].type: expected one of [fill, line, symbol, circle, fill-extrusion, raster, terrain, background], invalid found",
Copy link
Contributor

Choose a reason for hiding this comment

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

hillshade?

@@ -346,7 +346,7 @@ const subclasses = {
'fill': require('./style_layer/fill_style_layer'),
'fill-extrusion': require('./style_layer/fill_extrusion_style_layer'),
'line': require('./style_layer/line_style_layer'),
'symbol': require('./style_layer/symbol_style_layer')
'symbol': require('./style_layer/symbol_style_layer'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that compatible with Safari?

this._backfillDEM(tile);
} else {
this._source.fire('data', {dataType: 'source', tile: tile, coord: tile.coord});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave the call to this._source.fire('data', {dataType: 'source', tile: tile, coord: tile.coord}); at the root and just call backfillDEM additionally? backfillDEM calls the same function.

gl_FragColor = accent_color * (1.0 - shade_color.a) + shade_color;
} else {
gl_FragColor = vec4(1.0, 0.0, 1.0, 1.0);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Before we merge this, we should remove all unused modes.

array.emplaceBack(this._tileCoords[2].x, this._tileCoords[2].y, maxInt16, maxInt16);
array.emplaceBack(this._tileCoords[1].x, this._tileCoords[1].y, EXTENT, 0);
array.emplaceBack(this._tileCoords[3].x, this._tileCoords[3].y, 0, EXTENT);
array.emplaceBack(this._tileCoords[2].x, this._tileCoords[2].y, EXTENT, EXTENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we factor out this change into a separate PR that we can merge beforehand? I think it makes sense to have this as a separate change since it affects to many parts of the code that are unrelated to hillshading.

Copy link
Contributor Author

@mollymerp mollymerp Jul 6, 2017

Choose a reason for hiding this comment

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

yep – these changes are in #4770 and mapbox/mapbox-gl-native#9153, but they're blocked until text-pitch changes are merged into native

case 1:
_xMax = _xMin + 1;
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This could break if dx < -1 or > 1

@kkaefer
Copy link
Contributor

kkaefer commented Jul 4, 2017

We'd likely need something like mapbox/mapbox-gl-native#9415 to prevent flickering from overlapping parent/child tiles.

Molly Lloyd added 15 commits July 5, 2017 17:42
add terrain layer type and associated required files

build out DEM pyramid

fix setter+getter

read buffer

process array buffer into texture

attempt to draw offscreen texture for terrain-prepare shader calcs

use framebuffer to gemerate terrain texture

try to render ⛰️

rebase to use monorepo

use browser canvas obj

begin terrain-raster source scaffolding

create DEM from raster source

praise be!!! raster is a go 🙏
@mollymerp
Copy link
Contributor Author

this work is now tracked at #5286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants