Skip to content

Conversation

Pessimistress
Copy link
Collaborator

polygonOffset parameter was not applied.


// Call subclass lifecycle method
this.draw({moduleParameters, uniforms, settings});
withParameters(this.context.gl,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The original idea was that we would forward the settings to the layer's draw function, but we can certainly apply them here.

We wanted to support props.settings, this is a nice way to do it. Won't allow overriding the layers hardcoded draw settings though, but maybe that is a good thing.

Not sure if we are actually supplying props.settings somewhere, maybe they are already supplied by layer-manager or draw-and-pick?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

polygonOffset was a feature that layers used to get for free. Do we have a good reason to ask layers to do more work in their draw methods now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also settings is not documented - do we have a good use case for it? If yes, maybe should call it glParameters to be clear?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be many use cases for a layer settings prop (props.parameters or props.glParameters). An editing app that I work on now wants to control blend modes per layer, and this parameter would handle that nicely with our stock layers.

Exposing this gives a ton of flexibility to apps, at a very low API cost for deck.gl (luma.gl already does the heavy lifting of allowing parameters to be specified as keys in objects).

No objections to changing to glParameters although it would be (partly) inconsistent with luma.gl, where the the new Model.draw({uniforms, ..., settings}) function takes a settings field. That said, the global luma.gl function is called setParameters.

I'd probably vote for parameters and if so, maybe update Model.draw({settings}) to take Model.draw({parameters}) as well?

@1chandu @shaojingli

@ibgreen ibgreen merged commit c26bf7d into master Jul 11, 2017
@ibgreen ibgreen deleted the offset branch July 11, 2017 03:13
gnavvy pushed a commit that referenced this pull request Jul 12, 2017
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

Successfully merging this pull request may close these issues.

2 participants