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

Play scrubber #4336

Merged
merged 29 commits into from
Feb 16, 2018
Merged

Play scrubber #4336

merged 29 commits into from
Feb 16, 2018

Conversation

betodealmeida
Copy link
Member

@mistercrunch, this is the play slider/scrobbler for Deck.gl scatterplot. I started creating an NPM package for the <PlaySlider> component, but the layout and style is coupled with Superset, so I thought it would be better to have it here.

out

One thing I'm worried about these changes is that in order to get the start/end values for the slider the code is scanning the payload to get the min/max timestamps from the data. This has some advantages:

  • If you select a wide range of data (the long_lat sample table has a query since 2004, IIRC) the play slider will cover only the available data, making it better for exploring the results.
  • We don't need to parse the freetext ("now", "7 days ago") in Javascript.

The only downside is that it could be slow. Let me know what you think.

@@ -1933,6 +1933,7 @@ class DeckScatterViz(BaseDeckGLViz):
viz_type = 'deck_scatter'
verbose_name = _('Deck.gl - Scatter plot')
spatial_control_keys = ['spatial']
is_timeseries = True
Copy link
Member

Choose a reason for hiding this comment

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

I was expecting to see something that turns is_timeseries on or off in the query_obj based on whether a Time Grain is selected. Eventually we'll hide the Time Grain control where is_timeseries==False

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

@@ -0,0 +1,21 @@
.play-slider {
height: 100px;
Copy link
Member

@mistercrunch mistercrunch Feb 7, 2018

Choose a reason for hiding this comment

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

counldn't figure out how the DeckGL container knows how to set itself to height() -100 or is it just rendering on top of it?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the space currently left in the bottom, below the DeckGL container. TBH I wasn't really sure about the best way to place this.

@@ -79,6 +81,7 @@
"react-addons-shallow-compare": "^15.4.2",
"react-alert": "^2.3.0",
"react-bootstrap": "^0.31.5",
"react-bootstrap-slider": "2.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Cool, looks suited to support a nice SliderControl eventually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it should be straightforward to add the SliderControl — I actually added it at some point, and then removed.

(The latest version of the component is incompatible with other dependencies, which is why I fixed the version to 2.0.1, BTW.)

@@ -69,6 +70,7 @@
"mapbox-gl": "^0.43.0",
"mathjs": "^3.16.3",
"moment": "2.18.1",
"mousetrap": "^1.6.1",
Copy link
Member

Choose a reason for hiding this comment

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

Nice. we should sprinkle hotkeys everywhere! (and document them)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's worth doing a PR introducing some basic shortcuts.

At some point I was playing with more shortcuts (left and right to step the scrubber, up and down to increase/decrease its width), but I was worried it could conflict with other things.

}
render() {
return (
<div className="row play-slider">
Copy link
Member

@mistercrunch mistercrunch Feb 7, 2018

Choose a reason for hiding this comment

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

Let's use react-boostrap's Row and Cols here, in general we do all of the boostrap support through react-bootstrap as opposed through using className

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I didn't know about them.

@mistercrunch
Copy link
Member

I few minor nits and questions. Otherwise this looks solid!

Copy link
Member Author

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Will push the fixes.

@@ -0,0 +1,21 @@
.play-slider {
height: 100px;
Copy link
Member Author

Choose a reason for hiding this comment

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

That's the space currently left in the bottom, below the DeckGL container. TBH I wasn't really sure about the best way to place this.

@@ -79,6 +81,7 @@
"react-addons-shallow-compare": "^15.4.2",
"react-alert": "^2.3.0",
"react-bootstrap": "^0.31.5",
"react-bootstrap-slider": "2.0.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it should be straightforward to add the SliderControl — I actually added it at some point, and then removed.

(The latest version of the component is incompatible with other dependencies, which is why I fixed the version to 2.0.1, BTW.)

@@ -69,6 +70,7 @@
"mapbox-gl": "^0.43.0",
"mathjs": "^3.16.3",
"moment": "2.18.1",
"mousetrap": "^1.6.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's worth doing a PR introducing some basic shortcuts.

At some point I was playing with more shortcuts (left and right to step the scrubber, up and down to increase/decrease its width), but I was worried it could conflict with other things.

}
render() {
return (
<div className="row play-slider">
Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I didn't know about them.

@@ -1933,6 +1933,7 @@ class DeckScatterViz(BaseDeckGLViz):
viz_type = 'deck_scatter'
verbose_name = _('Deck.gl - Scatter plot')
spatial_control_keys = ['spatial']
is_timeseries = True
Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

@betodealmeida
Copy link
Member Author

@mistercrunch, I refactored the animation code based on our discussion, can you take a look and see if this could be reused in your animated lines viz? Basically you need to pass a getLayers function that takes the time range of the frame (from the PlaySlider) and returns an array of layers.

In the scatterplot viz, the values (start and end, but they can be equal) are timestamps that are used to further filter the payload. In your animated lines viz you can increase the frame rate (might require changing PlaySlider a bit) and force start and end to be identical (to give you a single instant in time).

I tested changing the time grain and zooming in/out to ensure that the viewport and the scrubber controls are correctly being updated:

out

@mistercrunch
Copy link
Member

This looks solid, I may need to adapt things a bit on my side but will do so in my PR if needed.

@mistercrunch mistercrunch merged commit 4ee0833 into apache:master Feb 16, 2018
@mistercrunch mistercrunch deleted the play_time branch February 16, 2018 01:55
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* Initial working prototype

* Small fixes

* Refactoring dekgl

* Show all data when no time grain is selected

* Refactor layers

* Standardize function name

* Fix exports

* Fix require

* Initial working prototype

* Small fixes

* Show all data when no time grain is selected

* Moving play bar to correct location

* Split component

* Working on CSS

* Remove control

* Positioning the play slider

* Fix refresh of slider state

* Fix lint

* Small fixes

* Smoother animation for scans

* Fix versions

* Play/pause with spacebar.

* Small fixes

* Clean stuff that went to other PRs

* Address issues

* Refactor scatter animation
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Initial working prototype

* Small fixes

* Refactoring dekgl

* Show all data when no time grain is selected

* Refactor layers

* Standardize function name

* Fix exports

* Fix require

* Initial working prototype

* Small fixes

* Show all data when no time grain is selected

* Moving play bar to correct location

* Split component

* Working on CSS

* Remove control

* Positioning the play slider

* Fix refresh of slider state

* Fix lint

* Small fixes

* Smoother animation for scans

* Fix versions

* Play/pause with spacebar.

* Small fixes

* Clean stuff that went to other PRs

* Address issues

* Refactor scatter animation
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants