#29809 closed enhancement (fixed)

Three.js: Upgrade to r117

Reported by: paulmasson Owned by:
Priority: major Milestone: sage-9.2
Component: graphics Keywords: threejs
Cc: ​egourgoulhon, gh-jcamp0x2a, fbissey Merged in:
Authors: Paul Masson Reviewers: Joshua Campbell
Report Upstream: N/A Work issues:
Branch: 375a187 (Commits, GitHub, GitLab) Commit: 375a187b0c9f83c4296cfad6b0797018e900534f
Dependencies: Stopgaps:

Status badges

Description (last modified by paulmasson)

The change to the template avoids a console warning that is new for this version.

The other console warning about OrbitControls cannot be removed because it represents a major change to the Three.js library that will take some effort to work around, but that does not need to be addressed until the beginning of next year.

Upgraded library is in attachments to this ticket.

Attachments (1)

threejs-r117.tar.gz (156.4 KB) - added by paulmasson 18 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 18 months ago by paulmasson

  • Description modified (diff)

comment:2 Changed 18 months ago by paulmasson

  • Branch set to u/paulmasson/threejs-r117

Changed 18 months ago by paulmasson

comment:3 Changed 18 months ago by paulmasson

  • Commit set to 375a187b0c9f83c4296cfad6b0797018e900534f
  • Status changed from new to needs_review

New commits:

375a187Upgrade to Three.js r117

comment:4 Changed 18 months ago by fbissey

  • Cc fbissey added

comment:5 follow-up: Changed 18 months ago by chapoton

maybe you should add the new "upstream_url" field in checksums.ini ?

Something like this, adapted :

upstream_url=https://github.com/numpy/numpy/releases/download/vVERSION/numpy-VERSION.tar.gz

comment:6 in reply to: ↑ 5 Changed 18 months ago by paulmasson

Replying to chapoton:

maybe you should add the new "upstream_url" field in checksums.ini ?

Something like this, adapted :

upstream_url=https://github.com/numpy/numpy/releases/download/vVERSION/numpy-VERSION.tar.gz

The upstream zip file includes the entire project. We are only using a small fraction of it, so the numbers wouldn't match.

Version 0, edited 18 months ago by paulmasson (next)

comment:7 Changed 18 months ago by gh-jcamp0x2a

Please forgive my ignorance of the process of updating a sage package, but I ran into build errors due to the zip file not existing on the mirrors (like the patch bot is currently reporting). Is this expected until the ticket gets merged or some other step is taken to sync them up? I was able to get it to build correctly by manually moving the zip into the upstream/ directory.

I verified that the two files within the zip (three.min.js and OrbitControls.js) are indeed from the r117 release and that the listed checksums match.

I had a collection of Jupyter notebooks on hand from prior work containing all of the examples in the plot3d reference documentation, so I did a quick side-by-side comparison with the old and new Three.js versions. I didn't observe any visual discrepancies, and the camera controls function the same as before.

I also noticed the new console warning regarding OrbitControls.js as expected.

Was the other console warning you mentioned due to Three.js expecting a boolean value but json.useFlatShading being null/undefined? I think !!json.useFlatShading would also work to coerce it to be boolean, but that's a minor nitpick.

comment:8 Changed 18 months ago by paulmasson

Joshua, copying the zip file to upstream is exactly what is needed in this circumstance, since it won't be copied to the mirrors until the ticket is approved. If you don't notice any problems in your notebooks then we're good to go.

comment:9 follow-up: Changed 18 months ago by gh-jcamp0x2a

  • Reviewers set to Joshua Campbell
  • Status changed from needs_review to positive_review

Ok, thank you. Just wanted to make sure that was expected. I've given a positive review.

comment:10 in reply to: ↑ 9 Changed 18 months ago by paulmasson

Replying to gh-jcamp0x2a:

Ok, thank you. Just wanted to make sure that was expected. I've given a positive review.

Thanks!

comment:11 Changed 17 months ago by vbraun

  • Branch changed from u/paulmasson/threejs-r117 to 375a187b0c9f83c4296cfad6b0797018e900534f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.