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

Break out @luma.gl/webgl module #983

Merged
merged 3 commits into from Mar 21, 2019

Conversation

Projects
None yet
3 participants
@ibgreen
Copy link
Contributor

ibgreen commented Mar 13, 2019

For #724

Background

Change List

  • @luma.gl/webgl2: This breaks out our webgl classes into a separate module.
    • Splits out half the code from the huge core module
    • Clarifies how the luma.gl code base is organized.
  • @luma.gl/core: reexport all webgl functions
    • so apps won't notice the split
    • just like previous shadertools and state-tracker splits
  • @luma.gl/webgl2-state-tracker:
    • just renames this module to webgl2-...
    • ensure all our webgl2... modules are consistently names
  • Notes:
    • This particular split does not give us any immediate bundle size benefits
    • since core will still depend on this module.
  • Challenges:
    • some utility functions needed to be duplicated or shared/reexported.

@ibgreen ibgreen force-pushed the ib/webgl-module branch from 7c990fb to 01c6c73 Mar 19, 2019

@ibgreen ibgreen requested review from tsherif and Pessimistress Mar 19, 2019

@ibgreen ibgreen changed the title Break out @luma.gl/webgl module Break out @luma.gl/webgl2 module Mar 19, 2019

@tsherif

This comment has been minimized.

Copy link
Member

tsherif commented Mar 19, 2019

Looks fine, but I'm just curious about the "WebGL 2" naming. Feel like it gives the impression that these are WebGL2-only.

@ibgreen

This comment has been minimized.

Copy link
Contributor Author

ibgreen commented Mar 19, 2019

Feel like it gives the impression that these are WebGL2-only.

Well, the impression I wanted to emphasize is that these libraries fully support WebGL2. I personally think that is what makes them stand out.

That said, @luma.gl/webgl2 will run on WebGL1, but only if webgl2-polyfill is installed, so in that sense I suppose the impression you have is partially true.

@ibgreen ibgreen force-pushed the ib/webgl-module branch from 01c6c73 to 0ecf2da Mar 20, 2019

@ibgreen

This comment has been minimized.

Copy link
Contributor Author

ibgreen commented Mar 20, 2019

Now rebased on top of the new import linter rules, which gives additional peace of mind when doing this type of splits...

just curious about the "WebGL 2" naming. Feel like it gives the impression that these are WebGL2-only.

I reverted this change for now. It does seems like something we should decide when we have a quorum.

@ibgreen ibgreen marked this pull request as ready for review Mar 20, 2019

@ibgreen ibgreen changed the title Break out @luma.gl/webgl2 module Break out @luma.gl/webgl module Mar 20, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 20, 2019

Coverage Status

Coverage increased (+0.2%) to 46.224% when pulling 0ecf2da on ib/webgl-module into fc5c605 on master.

@ibgreen ibgreen merged commit 2fe6b90 into master Mar 21, 2019

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 46.224%
Details
license/cla Contributor License Agreement is signed.
Details

@ibgreen ibgreen deleted the ib/webgl-module branch Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.