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

infra: upgrade three.js to r104 from r77 #2326

Merged
merged 2 commits into from
Jun 7, 2019

Conversation

stephanwlee
Copy link
Contributor

@stephanwlee stephanwlee commented Jun 6, 2019

Notable changes:

  • Vertex shader require related glsl code block
  • other API changes in between
  • WebGLRenderer.render no longer takes 4 inputs. Instead, you must set
    renderTarget manually.
  • BufferAttribute.prototype.count is important and attribute.array is
    discourage in favor of setArray

Notable no changes:

  • Types are not updated.
    • DefinitelyType no longer provide the latest
      typing of three in favor TypeScript in three.js repo. For the time
      being, we have decided not to pursue this as downloading the archive
      would fetch unnecessarily large file (examples dir is 300+MB).
    • Types in newer DefinitelyTyped and three.js seem to be incompatible
      with current build system. stephanwlee@daa35e4

Detailed steps to verify changes work correctly (as executed by you)

Poked around projector, what-if, and mesh plugins thoroughly.

Notable changes:
- Vertice shader require related glsl code block
- other API changes in between
- WebGLRenderer.render no longer takes 4 inputs. Instead, you must set
  renderTarget manually.
- BufferAttribute.prototype.count is important and attribute.array is
  discourage in favor of setArray

Notable no changes:
- Types are not updated. DefinitelyType no longer provide the latest
  typing of three in favor TypeScript in three.js repo. For the time
  being, we have decided not to pursue this as downloading the archive
  would fetch unnecessarily large file (examples dir is 300+MB).
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Wonderful! Thanks. One inline.

downloading the archive would fetch unnecessarily large file (examples
dir is 300+MB)

But https://registry.npmjs.org/three/-/three-0.104.0.tgz is 4.2MB and
includes individual .d.ts files, so that’s an option. It might require
a bit of BUILD work (à la Plottable), though, so no action required
in this commit.

@stephanwlee
Copy link
Contributor Author

stephanwlee commented Jun 7, 2019

@wchargin updated the PR message. Another typing issue that I faced is captured in this branch: stephanwlee@daa35e4. Both projector code and facets code need to be updated to work with the new type changes which I think should be done in a separate PR.

(I didn't know how to share my code in more permanent way but I will keep my branch around for awhile)

@stephanwlee stephanwlee requested a review from wchargin June 7, 2019 04:33
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Types in newer DefinitelyTyped and three.js seem to be incompatible
with current build system.

We should be able to fix this by upgrading TypeScript from 2.7.2 to 3.5,
and then passing the --allowUmdGlobalAccess compiler flag:

Again, no action required in this commit.

(I didn't know how to share my code in more permanent way but I will
keep my branch around for awhile)

I generally copy the diff or patch (git format-patch -1 HEAD) into a
Gist, which you can store indefinitely.

@stephanwlee
Copy link
Contributor Author

Didn't know about the TypeScript 3.5 changes. Thanks for that. I will consider upgrading that in the future.

@stephanwlee stephanwlee merged commit 1f56953 into tensorflow:master Jun 7, 2019
@stephanwlee stephanwlee deleted the threer branch June 7, 2019 18:15
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