-
Notifications
You must be signed in to change notification settings - Fork 330
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
Add drag & drop loading to Web Viewer #1482
Add drag & drop loading to Web Viewer #1482
Conversation
Hi @hybridherbst , |
Hi @kwokcb, there's no changes needed to make paths work, files and references that work locally should also work here. Complexity of drag-drop as implemented here loading just comes from the fact that there are multiple different web APIs and they work slightly different. There may be more efficient ways of doing this (e.g. implementing a custom manager that is able to resolve paths on-the-fly instead of loading all at once) but I think this is a good start. Some examples:
|
Thanks for the explanation. I'll admit I'm no |
Thanks for the clear explanation. I'll take a closer look at the code for review.
Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Just some minor comments, mostly about comments and docs.
import * as THREE from 'three'; | ||
import * as fflate from 'three/examples/jsm/libs/fflate.module.js'; | ||
|
||
const debugFileHandling = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general comment, can you add some function header comments for this new file.
You can follow the convention user for the other files.
{ | ||
const allEntries = []; | ||
|
||
let haveGetAsEntry = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this to the top along with the other debug flag?
|
||
async function handleFilesystemEntries(entries) { | ||
const allFiles = []; | ||
const fileIgnoreList = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to not include these, but curious as to why these specific files are specified ?
// put all files in three' Cache | ||
for (const fileEntry of allFiles) { | ||
|
||
const allowedFileTypes = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why only these file types ? Is this all the three.js loader can handle?
Shouldn't this just try and load and catch if it fails.
Also should the extension check be case insensitive?
dirReader.readEntries( | ||
async (results) => { | ||
if (results.length) { | ||
// entries = entries.concat(results); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Should this line be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valuable enough feature that I'd like to take this in stages, first merging the initial proposal from @hybridherbst, and then iterating on this feature to resolve the issues that @kwokcb brings up.
Great work, and thanks for this contribution!
00417d7
into
AcademySoftwareFoundation:main
This PR implements drag-drop file loading for - .mtlx files - .mtlx files + textures or folders with textures - folders that contain at least one single .mtlx and textures - .zip files containing the above Currently, only the first found .mtlx file from any of the above will be loaded; future improvements could include - showing a list of dropped .mtlx files to choose from - hot reload when a .mtlx file on disk or a referenced texture changes again - this would turn the web viewer into a pretty nice MaterialX hand editor without having any kind of MaterialX software installed.
This PR implements drag-drop file loading for
Currently, only the first found .mtlx file from any of the above will be loaded; future improvements could include