Kitware / minerva

Minerva: client/server/services for analysis and visualization
Apache License 2.0
36 stars 14 forks source link

Adding style configuration controls #355

Closed jbeezley closed 8 years ago

jbeezley commented 8 years ago

I'm at a crossroads on this branch, and I decided to make WIP PR to solicit advice.

Background: We want to be able to generate styles to differentiate features in a dataset. For example, to color states by a "population" field in the properties. This branch adds UI elements to the jsonConfigWidget to allow the user to map properties in their dataset to the styles of the features generated.

My intent was to run a preprocessor on the dataset model that does two things:

I attached a preprocessor to a change event that runs when the dataset loads. The problem is that in the current codebase, the dataset doesn't load on the client until the data is displayed. I would need to load the full dataset to generate the jsonConfigWidget which I think would be a bad user experience. I can think of a couple of alternatives:

  1. Do the preprocessing and normalization server-side and add either a new endpoint to get the properties summary or attach it directly to the dataset metadata. This would mean that all datasets need to pass through the server first, which I don't think is an assumption that we've made before explicitly. This would add more flexibility how we parse datasets because we wouldn't be limited to JS-only libraries and would provide a unified data path. But, it would also make it more difficult to create client-side components that can be used elsewhere.
  2. Add a button on the modal dialog to "Load dataset" if it hasn't already been loaded. This is similar to what QGIS does--there is a button that you have to press any time it needs to parse the dataset to fill in information on the settings dialog. This prevents unexpected lag in the application when responding to user input.

(2) would be much easier in the short term, but I suspect we will end up needing to implement (1) sooner or later. @mgrauer, do you have any thoughts about where we should go with this?

mgrauer commented 8 years ago

@jbeezley

I tried playing with this, but I'm not sure I did it right, or maybe it isn't ready yet.

I loaded a geojson point dataset (and also the states.geojson), and when I tried to render either of them without first setting style properties, I'd get

geojsonUtil.js:118 Uncaught Error: Invalid json type

When I tried to set style properties, they didn't seem to be reflected in the rendering, but I'll assume this is due to WIP.

I say go with (2)

jbeezley commented 8 years ago

Yes, this code doesn't yet work. I'll go ahead and do something simple like a "load data" button to populate the controls client side.

aashish24 commented 8 years ago

@jbeezley let us know when we could try this again.

jbeezley commented 8 years ago

In case anyone is interested, this is close to done. I just have a few bugs to work around. I have limited the scope a bit from my original attempt. Now only scales can only be generated for color values, and users are limited to color ramps defined in color brewer without any configurations of break points in the lookup table. It should at least be useful in most cases.

screen shot 2016-06-13 at 3 15 58 pm

jbeezley commented 8 years ago

This is ready for review.

aashish24 commented 8 years ago

this is great @jbeezley I will start the review now. @dorukozturk it would be good if you could review as well.

dorukozturk commented 8 years ago

I think overall it is really good. I have 2 recommendations.

1) I think the styling button should be on Session Layers panel. Also in order to change the color back and forth we need to delete the layer and add it back. Is there a reason for that?

2) Can we also implement color schemes with more or less steps? We have 6 steps only. I can see the dropdown can get complicated with more or less colors though. Maybe another dropdown which specifies how many steps there will be?

Other than that I think the tool is very nice.

@jbeezley

aashish24 commented 8 years ago

1) I think the styling button should be on Session Layers panel

I think @jbeezley did this because opacity and other visual attributes are on the dataset right now. We need to have backend for representations and then we can store meta on that too.

jbeezley commented 8 years ago

1) I think the styling button should be on Session Layers panel. Also in order to change the color back and forth we need to delete the layer and add it back. Is there a reason for that?

I agree, but I actually put it where it is on purpose because there is currently no way to update the style of an active layer in the API. (See my comment in the refactor PR) In many ways, this goes along with the problem of storing the visual style on the dataset. What we need to come up with is an independent model for describing it so a layer can be represented as {datasetId: '...', styleId: '...'}, etc.

aashish24 commented 8 years ago

This branch is looking very good so far :+1: great work @jbeezley. I just ran it and used a sample dataset. Still have to do the actual code review. On the UI side of things, we could do few things. I think @jbeezley already has some ideas for it. Here is my suggestion for a future PR:

  1. The UI panel says "points", May be we could use "Visual Mappings" or something more general.
  2. The size is under "Properties box" but stroke and fill are not. May be we could make it consistent? May we "Properties" could be "Shape & Size" or "Geometry" or something more intuitive.
aashish24 commented 8 years ago

The UI panel says "points", May be we could use "Visual Mappings" or something more general.

Sorry, typo: "Specify dataset rendering options" --> "Visual Mapping"

jbeezley commented 8 years ago

2) Can we also implement color schemes with more or less steps? We have 6 steps only. I can see the dropdown can get complicated with more or less colors though. Maybe another dropdown which specifies how many steps there will be?

For categorical types, it will always use the number of colors equal to the number of fields, but this is not represented in the drop down. For numeric types, it always uses the maximum number of colors available for a given color ramp (9 in this case). Customizing this could get infinitely complex. I think we will need to break each style property into a separate modal dialog eventually. For now, the controls are intentionally simple and should handle a majority of the use cases.

jbeezley commented 8 years ago

@aashish24 The bugs you saw are a result of an out of date jade version. You will need to update girder for this PR and make sure npm ls jade shows version 1.11.0 used by girder. I'm working on improving the styling of the panels now.

aashish24 commented 8 years ago

thanks @jbeezley, what version of girder would you recommend? I will try with master in the mean time.

jbeezley commented 8 years ago

I always develop on master.

aashish24 commented 8 years ago

sounds good @jbeezley. I was using 1.5.0 as I thought that's what we use for minerva when when we deploy it. May be, we need update our ansible scripts as well after this.

jbeezley commented 8 years ago

I updated the .girder-version file. I think the deployment scripts all use that.

aashish24 commented 8 years ago

Cool! thanks @jbeezley

aashish24 commented 8 years ago

@jbeezley with recent commits, it looks great on my linux system. One observation though: Now I see points, lines, and polygons (all three tabs) even though my data is only polyon. Should I be seeing points and lines mappins?

jbeezley commented 8 years ago

That's a change I made since we last talked. I found it to be more confusing to hide the tabs. I can change it back if you prefer.

aashish24 commented 8 years ago

That's a change I made since we last talked. I found it to be more confusing to hide the tabs. I can change it back if you prefer.

I think it's fine as it is. May be, we can make points and line non-selectable (greyed out) so that a user would know that data only contains polygons. Thoughts?

aashish24 commented 8 years ago

@jbeezley are you planning to add the grey-out feature?

jbeezley commented 8 years ago

Yes, I'll try to get to it tomorrow.

aashish24 commented 8 years ago

Thanks!

jbeezley commented 8 years ago

@aashish24 PTAL

aashish24 commented 8 years ago

LGTM :+1: :tada: thanks!