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

Ensure that externally created contexts are polyfilled, tracked #954

Merged
merged 5 commits into from Mar 9, 2019

Conversation

tsherif
Copy link
Contributor

@tsherif tsherif commented Mar 7, 2019

Ensure that context polyfills always occur, even if an external context is provided. This was only happening accidentally, if withParameters happened to be called while rendering a scene.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Ensure that context polyfills always occur, even if an external context is provided. This was only happening accidentally, if withParameters happened to be called while rendering a scene

To polyfill the context we just need to call trackContext I don't see that happening?

createGLContext is a public API if we change its behavior we need to update docs.

I must admit that as a I user I would find it a little unexpected that createGLContext accepts a context prop and returns it if supplied instead of creating a context, I would not have expected that.

@tsherif
Copy link
Contributor Author

tsherif commented Mar 7, 2019

@ibgreen it happens here: https://github.com/uber/luma.gl/blob/2a17e425ed99a64112f3e60cb3f1611d9a4c355d/modules/core/src/webgl/context/context.js#L109-L114

The problem is that if a GL context is provided it won't get the polyfills or tracking unless some incidental call to trackContext is made elsewhere, e.g. via a call to withParameters. This ensures the context we end up with is always usable.

Semantically, I think you could think of it as a subtle change to the semantics of createGLContext that "provides a context with the tooling required by Luma", whether that context is created internally or provided as an argument.

@ibgreen
Copy link
Collaborator

ibgreen commented Mar 7, 2019

The problem is that if a GL context is provided it won't get the polyfills or tracking unless some incidental call to trackContext is made elsewhere, e.g. via a call to withParameters. This ensures the context we end up with is always usable.

But couldn't we just call trackContext in the AnimationLoop if the context is provided.

At least right now, I don't think people are passing contexts they already created and want to use with luma.gl through createGLContext, so I don't think adding this option will solve the problem in any other case.

We could of course update the docs and tell people that they must do so, but that would seem equivalent to just asking them to call trackContext - which has the benefit of seeming more natural to me.

@tsherif
Copy link
Contributor Author

tsherif commented Mar 8, 2019

@ibgreen How about this approach? Keeps createGLContext functionality the same and just pulls out the tooling logic so it can be used separately.

@ibgreen
Copy link
Collaborator

ibgreen commented Mar 8, 2019

@ibgreen How about this approach? Keeps createGLContext functionality the same and just pulls out the tooling logic so it can be used separately.

OK I see. It both tracks context and adds debug instrumentation...

Where does that leave trackContextState? In that case I guess we would stop exporting that function and recommend this method instead?


Shouldn't this function be a member of "the GLContext family of functions"? Currently, from src/index.js:

  createGLContext,
  destroyGLContext,
  resizeGLContext,
  setContextDefaults

Maybe

  • getTooledContext => instrumentGLContext or addToolingToGLContext?
  • setContextDefaults => setGLContextDefaults?

@tsherif
Copy link
Contributor Author

tsherif commented Mar 9, 2019

Ok updated the name. @Pessimistress this should remove the need for that Deck.gl update you made to address this problem.

@tsherif tsherif merged commit 8bf6cd8 into master Mar 9, 2019
@tsherif tsherif deleted the always-polyfill branch March 11, 2019 23:39
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.

None yet

2 participants