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

Allow visualizations to present multiple tiled raster layers #1

Open
2 of 4 tasks
jjmata opened this issue Feb 6, 2017 · 31 comments
Open
2 of 4 tasks

Allow visualizations to present multiple tiled raster layers #1

jjmata opened this issue Feb 6, 2017 · 31 comments
Assignees

Comments

@jjmata
Copy link
Member

jjmata commented Feb 6, 2017

For our purposes, we're going to be working with a lot of tiled rasters since it looks like the future of Carto support for visualization of TIFFs is hanging by a thread. As a result we would like to allow for a visualization to contain multiple tiled raster layers that one can easily toggle.

Sub-tasks:

  • Collect a tiles URL from user, and persist it
  • Add it to the visualization on top of the basemap
    • Log every time a user adds a layer in a Hubspot event (really, I'd like to implement Segment ASAP)
  • Protect all this behind tileo-raster feature flag

What we've come up with informally, in the spirit of a quick MVP, is this:

  • Extend current Custom Basemap UI flow (much better approach was taken! 👍)
  • Treat anything beyond the 1st (custom) basemap definition as a "layer on top" of the 1st (custom) basemap
  • Start with 2 layers for now, extend to multiple later.
  • When more than one custom basemap is defined, make the 1st one unselectable and always display it ... the toggle now acts to select which of the other basemaps should be visible on top of the 1st one
  • Perfectly OK for opacity to be hardcoded for now

Of course, any of this can change as you see fit when implementing.

@jjmata jjmata assigned jjmata and apercas and unassigned jjmata Feb 6, 2017
@jjmata
Copy link
Member Author

jjmata commented Feb 6, 2017

@apercas: If you have new ideas / approach since work started on this already, don't hesitate to share!

@jjmata
Copy link
Member Author

jjmata commented Feb 6, 2017

Keeping a few tilesets here for testing purposes.

@apercas
Copy link

apercas commented Feb 19, 2017

Ok, so the problem here is that in order to have the same status of the map everywhere in the builder (widgets, analysis and the map itself) we can't change the map object (Leaflet) without jeopardizing the other two elements. If we do that, we'll lose their power as tools. This means each of the main elements can modify the 'map object' and the refresh of the map is lighter as we don't need to repaint everything every single time.

The only way to achieve this (if I'm not wrong here) is adding a new attribute (let's call it baseSlide, for instance) that lives between the basemap and the first (as in bottom) layer that has just the tiles we want to cover the basemap with. And, obviously, having this baseSlide present always, being completely transparent if the basemap URL is blank. Plus, as we will add this as another permanent L.tilelayer() pushing the index of Carto's tiler served tile layers one index up. So we would have, if we follow this path: BASEMAP (layer 0), BASESLIDE (layer 1), DATASET1 (layer 2), DATASET2 (layer 3) ... DATASETN (layer n+2).

The worst thing here is that keeping this scheme up with the upstream from Carto will become a pretty little nightmare as the codebase changes interfering with the proper map and the other components would be quite huge.

@apercas apercas added this to the april3rd milestone Mar 6, 2017
@apercas
Copy link

apercas commented Mar 9, 2017

Update:
We can hardcode a demo as the one on this screenshot where we see a 'spinal tap' inspired basemap from Thunder Forest and our own tiles from the lettuces dataset plus vector data (diameters as radius values).
image

I've achieve this hardcoding the Tilelayer directly to the map and bypassing it in the Leaflet factory.

Code:
vis.map.addLayer(new L.TileLayer('http://tileo-alpha-tiles.s3-website-us-east-1.amazonaws.com/ZV-2017-02-06%20Grupo%20Gs%20Lettuce%20crop/{z}/{x}/{y}.png')); on L66 of https://github.com/CartoDB/deep-insights.js/blob/master/src/api/create-dashboard.js#L66

if (! !!layerModel.get('type')) {
    layerModel.set('type','tiled');
    layerModel.set('urlTemplate',layerModel.get('_url'));
    layerModel.set('val','modified');
    layerModel.set('url',"http://{s}.basemaps.cartocdn.com/light_all/{z}/{x}/{y}.png");
  }

on L33 of /dronedb/node_modules/cartodb.js/src/geo/leaflet/leaflet-layer-view-factory.js (couldn't find the proper repo)

@jjmata
Copy link
Member Author

jjmata commented Mar 30, 2017

Lots of progress on #14 today, so you should be able to make this work on a fresh rebase of Carto, @apercas.

@apercas
Copy link

apercas commented Mar 30, 2017

On it.

@jjmata
Copy link
Member Author

jjmata commented Apr 3, 2017

We've already discussed that most of the (complicated) logic I described in this issue about reusing the existing basemap selector can wait/be ignored for now. But one thing I do want to do is put all of this (everything "raster related" behind a feature flag. What do you think @apercas?

I would vote for tileo-raster and of course we can wait until after we have this on beta this Wednesday to implement the feature flag.

Also, related, I would like a Hubspot event for tracking. Will add these to the issue as sub-tasks.

@jjmata
Copy link
Member Author

jjmata commented Apr 3, 2017

I can take care of some of the subtasks once you are done (Hubspot/Segment, for instance).

Edit: I actually started on the Segment.io piece today Tuesday 4/4 ...

@apercas
Copy link

apercas commented Apr 4, 2017

All in for the feature-flag. I'd like to see Diego use it before we close this.

@jjmata jjmata modified the milestones: James May, April O'neil Apr 10, 2017
@jjmata jjmata modified the milestones: Juno, James May May 8, 2017
@apercas
Copy link

apercas commented May 26, 2017

screen shot 2017-05-26 at 18 27 51

Update (for real):

The new and buggy version relies on the by-default CARTO and cartodb.js flow with its mapnik/pgraster/postgis integration.

Before you start, make sure you have a georeferenced raster file at reach, I used: https://github.com/MapWorkshops/CartoRecipes/blob/master/postgisraster/loadraster/foto_pnoa.tif

To make it run:

  • git pull this repo and checkout to tileo-feature/raster-layer-builder, being at least here: fadd834
  • rebuild the assets (including carted.js and deep-insights from the forked repos)
  • run grunt
  • (!important) rename TIFF file adding raster to the name: foto_pnoa --> foto_pnoa_raster [*]
  • drag and drop the file to the dataset tiles
    • ignore any 404 error after uploading the file [*]
  • (!important) change the visibility of the table from the tables list [*] splashblot/cartodb.js@558cc2f
  • reuse/create a visualisation
  • add the layer from the add button (opens a dialog with your postgres tables, including rasters 💫)

[*] Will take care of this *soon

That's all :)

Happy hacking!

@apercas
Copy link

apercas commented May 29, 2017

  • Bug: Wouldn't run on Safari

UPDATED: fix here splashblot/cartodb.js@26daa5a

@apercas
Copy link

apercas commented May 30, 2017

Refactoring raster class: splashblot/cartodb.js@991e54e

apercas added a commit to splashblot/cartodb.js that referenced this issue May 30, 2017
@apercas
Copy link

apercas commented Jul 4, 2017

Updates on: https://github.com/splashblot/dronedb/tree/tileo-feature/raster-flow-issues

  • 404
  • List/edit rasters

@apercas
Copy link

apercas commented Jul 6, 2017

  • Toggle

@jjmata
Copy link
Member Author

jjmata commented Jul 8, 2017

OK, almost have this ready for a "preview release" 😄

What's going on now that I have merged the tileo-feature/rebased-raster-over-tileo-june2017 into tileo-june2017 is that the Dockerfile picks up Carto's cartodb.js because I believe it comes from the Git submodule.

If I run npm update on the current image I am testing I get:

root@d83670bc0f2b:/dronedb# npm update cartodb.js
- [email protected] node_modules/tangram.cartodb/node_modules/argparse
- [email protected] node_modules/tangram.cartodb/node_modules/esprima
- [email protected] node_modules/tangram.cartodb/node_modules/js-yaml
- [email protected] node_modules/tangram.cartodb/node_modules/regenerator-runtime
- [email protected] node_modules/tangram.cartodb/node_modules/babel-runtime
- [email protected] node_modules/tangram.cartodb/node_modules/tangram
[email protected] /dronedb
├── [email protected] (git://github.com/cartodb/deep-insights.js.git#d340b02989df36eb771df2adfbea4a6b704cf9b4)
└── [email protected]  (git://github.com/splashblot/cartodb.js.git#2177259602a97d81e6cb5e68e6a3697d503bbeb6)

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@^1.0.0 (node_modules/chokidar/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

So there are obviously new things that need to be included that npm install does not cover for us.

So, still working here to get a working image with this feature in mostly functional state. But still a couple more steps to go! 🚶🚶🚶

@jjmata jjmata assigned jjmata and unassigned apercas Jul 8, 2017
@jjmata
Copy link
Member Author

jjmata commented Jul 8, 2017

The new docker-dronedb image now works out-of-the-🗳 (hash eeb43d0c7827) ... 🍾🍾🍾

Items remaining as far as I can tell from some basic clicking around:

  • Clicking on a raster dataset name in /user/dev/dashboard/datasets 404s in my testing, I suggest we don't hyperlink them for now.
  • Some rasters will not work, here is one example. I'm sure more will come, we should start understanding the limitations we have.
  • Toggle gets confused when I have Positron Labels and it toggles those on / off / the raster / then I get two rasters. Try it!
  • Implementing the event tracking (Hubspot and/or Segment), let's talk since I need to turn Hubspot on "for realz!"
  • Dashboard thumbnails need to -at the very least- not break. OK if for a first pass they don't include the raster layer in them and skip/ignore it, but they need to have the basemap and other layers included (see Fix thumbnails generation (center, zoom, bounds) CartoDB/cartodb#12434 for help & inspiration!)

All the above I would probably address before we close this issue. New issues can be created for the below, but feel free to disagree with me, close this and create the whole bunch as new issues (probably too late for our feature flag dreams, no?)

  • Colorization of rasters.
  • Extract bounding box for raster and zoom in as appropriate after visualization is created / implement layer "centering" flow.
  • Hyperlink raster dataset names in /user/dev/dashboard/datasets and show some sort of "raster information summary" (gathered/persisted from GDAL at import time?)
  • Make thumbnails work with rasters in them.

Other thoughts?

💪 work, @apercas!

@jjmata jjmata assigned apercas and unassigned jjmata Jul 8, 2017
@apercas
Copy link

apercas commented Jul 8, 2017

Great!
I'm just guessing but toggling and the 404 issue were addressed on the previous branch and could have been broken over the merging (I tried to fix them but maybe I skipped some user cases), so digging that now.

@jjmata
Copy link
Member Author

jjmata commented Jul 9, 2017

Interesting, I was just re-testing and the 404 error that I thought was gone when creating the viz from the dashboard (via drag & drop of raster file) re-appeared. It worked the first time, and when I was going to create the second one for another test it popped up again. 😕

@jjmata
Copy link
Member Author

jjmata commented Jul 9, 2017

It would also be interesting to watch/debug the postgres errors that we see when importing rasters:

postgis_1        | ERROR:  attribute "the_geom" does not exist
postgis_1        | STATEMENT:
postgis_1        | 	      SELECT _postgis_stats ('raster_simple', 'the_geom');

and

postgis_1        | ERROR:  attribute "the_geom" does not exist
postgis_1        | STATEMENT:
postgis_1        | 	      SELECT _postgis_stats ('raster_simple', 'the_geom');
postgis_1        |
postgis_1        | ERROR:  column "the_geom" does not exist at character 76
postgis_1        | STATEMENT:
postgis_1        | 	        SELECT DISTINCT ST_GeometryType(the_geom) FROM (
postgis_1        | 	          SELECT the_geom
postgis_1        | 	        FROM public.raster_simple
postgis_1        | 	          WHERE (the_geom is not null) LIMIT 10
postgis_1        | 	        ) as foo

... for example.

@jjmata
Copy link
Member Author

jjmata commented Jul 11, 2017

Let's review this together tomorrow, @apercas. Really need to get this branch cleaned up and ready to forget about ... ;-)

@apercas
Copy link

apercas commented Jul 11, 2017

About

Some rasters will not work, here is one example. I'm sure more will come, we should start understanding the limitations we have.

This is related with the raster naming issue.

@apercas
Copy link

apercas commented Jul 11, 2017

Naming and toggling fixed on tileo-feature/rebased-raster-over-tileo-june2017, ⚠️ will require an update of cartodb.js node module.

@apercas
Copy link

apercas commented Jul 12, 2017

Checking the thumbnails issue, please ping me for the Hubspot tracking task.

@apercas
Copy link

apercas commented Jul 28, 2017

The task here now is to set Nokia as the default map

@jjmata jjmata removed this from the Juno milestone Jul 31, 2017
@apercas
Copy link

apercas commented Jul 31, 2017

Done.

@jjmata
Copy link
Member Author

jjmata commented Aug 1, 2017

The task here now is to set Nokia as the default map

Is that all? :trollface:

@apercas
Copy link

apercas commented Aug 1, 2017

@apercas
Copy link

apercas commented Oct 4, 2017

We added some fixes to this #69

apercas added a commit to splashblot/cartodb.js that referenced this issue Oct 7, 2017
@apercas
Copy link

apercas commented Dec 21, 2017

Mapnik/Windshaft rendering implementation deployed to production.

@ivanprado
Copy link

We keep that ticket open for sentimental reasons... as it is the first open ticket :-D

@apercas
Copy link

apercas commented Feb 12, 2018

I forgot: 🍰🎉🎁Happy Birthday, ticket #1! 🍰🎉🎁

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

No branches or pull requests

3 participants