Generic prop transition #3443
Generic prop transition #3443
Conversation
There was a problem hiding this comment.
I don't have any strong objections to this approach. I should take one more look but as far as I can tell it does avoid most of the deck.gl diffing overhead and just runs in the animation loop calls.
The main part that probably merits some face-to-face discussion/brain storming is of course the layer.prop override.
Mixed comments:
- Overriding props seems a little like a silver bullet, in that we may only get to fire it once (overriding in several layers might get too complex), is this the right use for it?
-Interesting to discuss what alternatives would be. Passing in prop overrides to draw? Setting up transition functions for luma.gl props.
-
Worth discussing our uniform strategy before pulling the trigger?
-
Do we really want layers to set uniforms every draw call? The prop update system is based on that. How is the move to uniform buffers going to happen.
Perhaps a uniform manager that would be able to handle this without prop overriides?
Nits:
- The props override generates a bunch of GC objects (
Object.create) during transitions. Probably hard to avoid even in alternative implementations.
| @@ -128,15 +83,19 @@ export class App extends Component { | |||
| coverage, | |||
| data, | |||
| elevationRange: [0, 3000], | |||
| elevationScale: this.state.elevationScale, | |||
| elevationScale: data && data.length ? 50 : 0, | |||
There was a problem hiding this comment.
One almost wants a way to specify that "transitions" should start at data load time, so this could be dropped...
There was a problem hiding this comment.
In the website data is passed in from the parent, so onDataLoad will not fire.
| if (transitions.size > 0) { | ||
| // clone props | ||
| const propsInTransition = transitions.update(); | ||
| const props = Object.create(this.props); |
There was a problem hiding this comment.
Nits:
- Generates a bunch of GC objects during transitions.
There was a problem hiding this comment.
This is the safest and cheapest way to duplicate the props object with some overrides. We do the same when an async prop is loaded, a layer is cloned, etc.
modules/core/src/lib/layer.js
Outdated
| if (!uniforms.picking_uActive) { | ||
| this.updateTransition(); | ||
| // Overwrite this.props during redraw to use in-transition prop values | ||
| this.props = this.updateTransition(); |
There was a problem hiding this comment.
Unrelated: The fact that layer.props can be changed is interesting. Wonder if applications could use that to subvert having to create new layers...
| } | ||
|
|
||
| const {opacity} = this.props; | ||
| // apply gamma to opacity to make it visually "linear" |
There was a problem hiding this comment.
Nit: Seems like this gamma conversion should be specified in a table somewhere, rather than being a special case here, could even be in the prop types...
Pessimistress
force-pushed
the
x/prop-transition
branch
from
6b89b2c
to
8a28996
Compare
Many layers manipulate their props before sending them to uniforms. Any approach other than overriding the props object would essentially require all layers to remember not to use the prop value as is and do something different.
I am moving the update of transition into |
For https://github.com/uber/deck.gl/blob/master/dev-docs/RFCs/v7.x/property-transition-rfc.md
Change List
elevationScaleandcoveragetransitions in HexagonLayerTODO