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

Add drag & drop loading to Web Viewer #1482

Conversation

hybridherbst
Copy link
Contributor

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.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@kwokcb
Copy link
Contributor

kwokcb commented Aug 21, 2023

Hi @hybridherbst ,
This has been requested for some time :) Thanks for putting this together.
I'm curious how are dependent images handled? i.e. what should the URI be to specify the location of the image?

@jstone-lucasfilm jstone-lucasfilm changed the title Feature: drag & drop loading Add drag & drop loading to Web Viewer Aug 21, 2023
@hybridherbst
Copy link
Contributor Author

hybridherbst commented Aug 21, 2023

Hi @kwokcb, there's no changes needed to make paths work, files and references that work locally should also work here.
The important part is THREE.Cache.enabled = true; which allows pre-seeding the cache with the dropped binary files (textures) and text files (mtlx files). So basically everything is loaded into the cache with relative paths to the dropped thing (e.g. a folder or, if multiple files were selected, /) and things just work :)

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:

@kwokcb
Copy link
Contributor

kwokcb commented Aug 21, 2023

Thanks for the explanation. I'll admit I'm no three.js expert by a long shot.
Indeed just having this opens up the viewers usefulness a lot.

@hybridherbst
Copy link
Contributor Author

If you're curious, the interesting parts are

@kwokcb
Copy link
Contributor

kwokcb commented Aug 22, 2023

Thanks for the clear explanation. I'll take a closer look at the code for review.

  • For posterity If it's not in the code, could you add these annotations.
  • For users, could you update the docs here to denote the new functionality to outline what is supported (basically what you mentioned in the ASWF Slack). It may not be obvious for instance that folks can drop zip file from the GPUOpen library.

Thanks.

Copy link
Contributor

@kwokcb kwokcb left a 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;
Copy link
Contributor

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;
Copy link
Contributor

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 = [
Copy link
Contributor

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 = [
Copy link
Contributor

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);
Copy link
Contributor

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?

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a 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!

@jstone-lucasfilm jstone-lucasfilm merged commit 00417d7 into AcademySoftwareFoundation:main Aug 25, 2023
13 checks passed
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
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.
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.

None yet

3 participants