splashblot / dronedb

Location Intelligence & Data Visualization tool
http://carto.com
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

[WIP] Tileo feature/raster tiled picker clean #19

Closed apercas closed 7 years ago

apercas commented 7 years ago

This PR closes https://github.com/splashblot/dronedb/issues/1

This PR allows to add tiled layers to a CARTO Editor 3 visualisation.

We make use of the exposed map over Deep Insights over its create-dashboard.js file. I've updated the package.json version so it retrieves the data from this repo.

WIP: It has a basic styling, will improve it over the weekend.

jjmata commented 7 years ago

Doesn't really implement any of the logic described on #1, right? Saying it "closes it" is a bit of an overstatement 😈 I'm happy to give up my contrived logic that I described to avoid needing new UI, but let's not punt on that quite yet.

I can't make it work, for starters, it renders wrong on Chrome (see below), and correctly at the bottom of the screen on Safari. But the "Add layer" button does nothing.

Am I missing any steps? I moved to your feature branch to test, and re-rant grunt. Anything else needed?

screenshot 2017-03-31 12 07 56
jjmata commented 7 years ago

And don't worry about the styling. Much easier to reuse their UI widget and rework the logic than maintain new UI, IMHO. Let's discuss when you get a chance.

apercas commented 7 years ago

Update: This PR requires a npm update in order to grab the forked repos deep-insights.js and cartodb.js. The main logic of this PR relies on https://github.com/splashblot/deep-insights.js/blob/tileo/src/api/create-dashboard.js after all.

The packgage.json file has been updated as well so we'll have some new fresh assets as well.

jjmata commented 7 years ago

Ready for me, correct? Will merge, test and get this puppy ready to roll!

jjmata commented 7 years ago

Almost there!

Problems observed:

pr1a

apercas commented 7 years ago

Added persistence logic: https://github.com/splashblot/deep-insights.js/commit/9147f366c0f40d46ad9e7e20d96ec20f4f8782f6

In this changes we check if a user dataset named tiled_layers_collection exists, if it doesn't we create it. (This takes a while so if needed, here's a gist with a CSV for it https://gist.github.com/apercas/68378bf5f4613b83a1ec91a4f6a6e796)

We check if the user has any saved raster layer and if so, we paint them. When we delete them from the front end the visible field of that layer on the DB changes to false.

Still figuring out the layerindex issue and sorry again for the delay on this :(

PS: this will require another npm update before a grunt execution

jjmata commented 7 years ago

I like where this is going ... also thinking that we could make the hack a little more elegant by treating the tiled_layers_collection SQL table as a special dataset that is treated just like any other dataset for most other intents and purposes (it doesn't show in the Datasets list though to avoid confusion).

So that is to say that we always create an organization-level SQL table for all raster layers that are available to the whole organization, and a user-level SQL table for all raster layers that are user-specific. Either that or one single one with ACL checks.

Taking over here to hack on it for a bit. I am also planning on adding another type of layer as discussed today:

jjmata commented 7 years ago

As mentioned, I've added support for GeoTIFF/raster layers coming straight out of the DB by extending your hack. 😈

I got lazy and didn't make the GeoTIFF code work the first time around, you need to leave the visualization and come back. Basically didn't know how to abstract the code starting in line 107 into a separate function immediately, so I left it for later.

See here.

jjmata commented 7 years ago

Urgh, didn't realize that this doesn't support subdomain-less URLs as written right now. Taking it back for a bit to fix this and make it work both in tileo.localhost and *.tileo.co for now.

jjmata commented 7 years ago

OK, deep-insights.js updated for subdomain-less URLs. Back to you, @apercas for your review and finishing touches.

jjmata commented 7 years ago

I just tested the latest changes coming for this feature from the deep-insights.js repo, observations:

  1. The addition of the API key on the raster SQL made things worse, not better (stopped working for me, had to remove it again)
  2. Raster layer doesn't render after input of dataset name (still need to navigate out, then back into viz)
  3. z-index layering problem still present
  4. removing a layer doesn't remove it from view, just removes it from the UI dialog but raster is still displaying
  5. Still doesn't work for the public user
jjmata commented 7 years ago

FYI, beta.tileo.co got refreshed just now with new bits (GDAL 2.1 and a few other fixes ... export works again, for instance).

This is the viz where I've recreated the "work in progress":

Due to the 0.5 transparency of the raster layer you can still see the data points.

apercas commented 7 years ago

I don't seem able to get to beta (maybe related with the errors on Slack #devops)

apercas commented 7 years ago

OK, answeting https://github.com/splashblot/dronedb/pull/19#issuecomment-293825116

Huge update on https://github.com/splashblot/deep-insights.js/commit/2965b08a2d0f5b869fbcb53c946547ee86331d7c tackling:

URGENT: the damn zindex issue.

jjmata commented 7 years ago

Looking good! So plan is to do some more testing, put it behind a feature-flag and then open a new issue/PR for the remaining work? This should be sufficient for upcoming demos.

apercas commented 7 years ago

I'd love to finally discover how to reorder the layers so I can bring the vector data to the front again. I guess that for now this workaround might have to do the trick: https://github.com/splashblot/deep-insights.js/commit/bf7c25129cd005e774389a228fdbd0303902ecd2

apercas commented 7 years ago

is the feature-flag the only thing remaining here? Should we invest that time on the Mapnik/Windshaft hacking to make this organic?

jjmata commented 7 years ago

A few things remain if we were to keep this approach in the medium term (can't see keeping it long term):

I agree that all of this is secondary right now and nullified by a potential move to mapnik-native layers, so let's put any further work on hold and I'll just merge it as is for now into tileo-develop.

jjmata commented 7 years ago

Taking ownership for the merge.