Feat/add gltf previewer #608
Conversation
ntarocco
force-pushed
the
feat/add-gltf-previewer
branch
from
e357ca6 to
88c94c0
Compare
| from invenio_previewer.utils import dotted_exts | ||
|
|
||
| previewable_extensions = ["gltf"] | ||
| previewable_extensions = ["glb", "gltf"] |
There was a problem hiding this comment.
We need to get all the possible extensions
There was a problem hiding this comment.
This is a necessary addition. The GLTFLoader of three.js handles both transparently.
|
One comment on the viewport and camera. What shows up by default in the previewer and how much of it is visible depends on both the viewport (what is shown here seems fine) and the camera position in the three.js previewer. The initial camera position for CMS events (like one shown here) works. However, for other gltf files there may geometries with different scales models may appear "too far" or "too close" or with suboptimal camera positions. Perhaps this isn't a big deal as one can zoom in and out with the previewer. Saying all this, it might be a good idea as a future feature to get sensible defaults from the 3D object bounding box. |
ntarocco
force-pushed
the
feat/add-gltf-previewer
branch
from
88c94c0 to
a80b876
Compare
ntarocco
force-pushed
the
feat/add-gltf-previewer
branch
from
a80b876 to
7ce156b
Compare
palkerecsenyi
left a comment
palkerecsenyi
left a comment
There was a problem hiding this comment.
Looks good! I haven't tested it locally but it follows the Three.js docs closely, I can't see any serious bugs. I just have a few small suggestions, but it would also be fine to merge as-is.
| "gltf_previewer_css": "./less/cds_rdm/previewers/gltf-previewer.less", | ||
| }, | ||
| dependencies={ | ||
| "three": "^0.180.0", |
There was a problem hiding this comment.
This was just updated to 0.181.0 a few minutes ago: https://github.com/mrdoob/three.js/releases/tag/r181
| }, | ||
| dependencies={ | ||
| "three": "^0.180.0", | ||
| "three-addons": "^1.2.0", |
There was a problem hiding this comment.
Not sure if there's any alternative but this was archived in 2021 and last publish 7 years ago. https://github.com/marcofugaro/three-addons.
There was a problem hiding this comment.
I saw it, however, I am a big fan of code that does not change and it works :)
| import { GLTFLoader } from "three/addons/loaders/GLTFLoader.js"; | ||
|
|
||
| document.addEventListener("DOMContentLoaded", () => { | ||
| class ThreeScene { |
There was a problem hiding this comment.
Maybe could we declare the class outside of the event handler and call just create + init the app inside it? The class not being top-level looks a little unusual
There was a problem hiding this comment.
We can, but I don't think it will make any difference: I really hope that DOMContentLoaded is fired only once! I will move it.
|
|
||
| startRendering() { | ||
| const renderLoop = () => { | ||
| requestAnimationFrame(renderLoop); |
There was a problem hiding this comment.
Should we move this to the end of renderLoop to avoid running the next update before the current one accidentally? It probably won't make a huge difference if the render is fast but I've usually seen this call at the end rather than the start.
| const progressBar = document.getElementById("progress"); | ||
| const percentText = document.getElementById("percent"); |
There was a problem hiding this comment.
This might be slightly precarious in case we happen to have other elements with these IDs on the page as they're quite generic. Maybe we could use more specific names, e.g. gltf-previewer-progress
| } | ||
| }, | ||
| (error) => { | ||
| console.error("An error occurred loading the GLB file:", error); |
There was a problem hiding this comment.
Will this show a visible error to the user? E.g. if the user is on an unstable network it's quite likely that the load would fail so we should make sure it's communicated clearly
| this.controls.update(); | ||
| this.renderer.render(this.scene, this.camera); | ||
| }; | ||
| renderLoop(); |
There was a problem hiding this comment.
Also the official recommendation is to use this.renderer.setAnimationLoop instead of manually calling requestAnimationFrame to ensure the loop timing works on all devices: https://threejs.org/docs/#WebGLRenderer.setAnimationLoop
There was a problem hiding this comment.
e.g. see the "Rendering the scene" example: https://threejs.org/manual/#en/creating-a-scene
3 participants
Preview:
