Opened 2 years ago
Closed 2 years ago
#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 )
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)
Change History (12)
comment:1 Changed 2 years ago by
- Description modified (diff)
comment:2 Changed 2 years ago by
- Branch set to u/paulmasson/threejs-r117
Changed 2 years ago by
comment:3 Changed 2 years ago by
- Commit set to 375a187b0c9f83c4296cfad6b0797018e900534f
- Status changed from new to needs_review
comment:4 Changed 2 years ago by
- Cc fbissey added
comment:5 follow-up: ↓ 6 Changed 2 years ago by
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 2 years ago by
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.
comment:7 Changed 2 years ago by
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 2 years ago by
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: ↓ 10 Changed 2 years ago by
- 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 2 years ago by
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 2 years ago by
- Branch changed from u/paulmasson/threejs-r117 to 375a187b0c9f83c4296cfad6b0797018e900534f
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Upgrade to Three.js r117