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

webassembly/app.html: Fetch mesh from search-query url-param #1596

Merged
merged 12 commits into from
Sep 4, 2024

Conversation

jo-chemla
Copy link
Contributor

@jo-chemla jo-chemla commented Aug 28, 2024

PR Following feature request thread here #1595

webassembly/app.html Outdated Show resolved Hide resolved
Add extension to filename, whether passed as url-param, or from response header `content-disposition`
@Meakk
Copy link
Member

Meakk commented Aug 28, 2024

Just tried a local build and opening http://192.168.0.12:3000/?mesh=https://github.com/KhronosGroup/glTF-Sample-Models/raw/main/2.0/DamagedHelmet/glTF-Binary/DamagedHelmet.glb gives me the following error:

Access to fetch at 'https://github.com/KhronosGroup/glTF-Sample-Models/raw/main/2.0/DamagedHelmet/glTF-Binary/DamagedHelmet.glb' from origin 'http://192.168.0.12:3000' has been blocked by CORS policy: The 'Access-Control-Allow-Origin' header contains the invalid value ''. Have the server send the header with a valid value, or, if an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

Any idea?

@jo-chemla
Copy link
Contributor Author

True, CORS blocking is a standard error when fetching content that resides on a server, from an app that lives on another domain. You either need:

  • to use a url to a mesh file that has Access-Control-Allow-Origin: * headers, maybe these should work like teapot
  • or to use an Allow-CORS plugin (for example this plugin: firefox/chrome)

@Meakk
Copy link
Member

Meakk commented Aug 28, 2024

Using ?mesh=https://groups.csail.mit.edu/graphics/classes/6.837/F03/models/teapot.obj:

Uncaught (in promise) TypeError: Cannot read properties of null (reading 'split')
    at ?mesh=https://groups.csail.mit.edu/graphics/classes/6.837/F03/models/teapot.obj:241:45

Using ?mesh=https://groups.csail.mit.edu/graphics/classes/6.837/F03/models/teapot.obj&extension=.obj:
No error is reported by the window is empty (a filename meshurl_param.obj is set in the open button)

image

@snoyer
Copy link
Contributor

snoyer commented Aug 28, 2024

How do we feel about using the hash part of the URL (https://f3d.app/web/#http://example.com/model.glb) instead of the search params?
Also (and independently of the hash vs search question) appending the extension as a hash to the model URL looks like it could be a simpler solution than adding a seperate parameter

        const mesh_url = window.location.hash.substring(1);
        const j = mesh_url.lastIndexOf('/');
        const mesh_name = (j>0? mesh_url.substring(j+1):mesh_url);
        const sanitized_mesh_name = mesh_name.replace(/[^a-zA-Z0-9._-]/,'_');
        // ...
            Module.FS.writeFile(sanitized_mesh_name, new Uint8Array(buffer));
            openFile(sanitized_mesh_name);
  • with http://localhost:8000/app.html#http://localhost:8000/Buggy.glb:

    • mesh_url: http://localhost:8000/Buggy.glb
    • mesh_name: Buggy.glb
    • sanitized_mesh_name: Buggy.glb
  • with http://localhost:8000/app.html#http://localhost:8000/cube#.glb:

    • mesh_url: http://localhost:8000/cube#.glb
    • mesh_name: cube#.glb
    • sanitized_mesh_name: cube_.glb

@jo-chemla
Copy link
Contributor Author

jo-chemla commented Aug 28, 2024

Thanks for the feedback. I just found a way to test my changes using the existing f3d.js, wasm and data build files live on https://f3d.app/web/f3d.js. I've reworked the way the url is constructed and tried to investigate why the mesh is not loading, probably should be easy.

Hash part of the url vs search query can probably be done as soon as the POC works ok. Regarding the sanitization, it can probably also rely on existing encodeURI browser APIs. Will report if I find anything!

The error I'm getting at the moment: ChunkLoadError: Loading chunk 5 failed.(missing: undefined)

@jo-chemla
Copy link
Contributor Author

jo-chemla commented Aug 28, 2024

Finally got it to work! 🚀
There are logs to better understand how the filename is constructed, this should work with most use-cases. Hope this is useful!

Tested with

Note if the window location hash is used instead of a search query, it might be more difficult to allow the user to pass both a mesh url and an extension in case the extension is not included within the url or returned as response header content-disposition, or hashes are present in the url. A way to circumvent this would be to encodeURI the mesh-url passed as a hash, and decode it before fetching the request. One could then simply define the url formatting as a search query where the question mark is replaced by a hash, something like https://f3d.app/web#mesh=${encodeURI(https://groups.csail.mit.edu/graphics/classes/6.837/F03/models/teapot.obj)}&extension=.obj

Following direction by f3d team, see PR discussion thread
@jo-chemla
Copy link
Contributor Author

jo-chemla commented Aug 28, 2024

Final edit: 🚀 Last commit replaces the search-query with a hash as requested by @snoyer URLs are parsed exactly like search queries, but with a # rather than ? character. mesh URLs parsed are decodeURI'ed, so one can pass either a standard url, or an encoded one in case it includes special characters like ?, # etc. I've also updated the above list of mesh files I could test it against, and all formats do work! From my point-of-view, it is now up to the f3d team whether or not to merge!

Final format looks like this:

Copy link
Member

@Meakk Meakk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I confirm it works, nice job :)
Can you address the few cosmetic comments here, but it looks good to me.
@snoyer can you also take a quick look at the changes?

webassembly/app.html Outdated Show resolved Hide resolved
webassembly/app.html Outdated Show resolved Hide resolved
webassembly/app.html Outdated Show resolved Hide resolved
webassembly/app.html Outdated Show resolved Hide resolved
@jo-chemla
Copy link
Contributor Author

Alright, just did include the suggested changes, thanks for the feedback!
And I realized I forgot to push the ? -> # change (search query to hash) suggested by snoyer, now it is included in this final commit.

webassembly/app.html Outdated Show resolved Hide resolved
@snoyer
Copy link
Contributor

snoyer commented Aug 29, 2024

Nice feature! Here's a few improvements:

It'd be nice to clarify the order of priority used to get the filename between the download URL, response header, and extension parameter. I think it should be: from the header if available, otherwise from the url, and either way force the extension if the parameter is set. Maybe refactor to a filename_for_model_url(model_url, format, header) function so we can cover it with tests in the future.

Using the hash is nice and local, it doesn't get sent to the server and doesn't refresh the page when changed, but it therefore needs a listener to work on subsequent changes.

I'd say overall it would need to look something like:

  function load_from_url(){
    const params = new URLSearchParams(window.location.hash.replace('#', '?'));
    // ...
    fetch(model_url).then((response) => {
         // ...
         const filename = filename_for_model_url(model_url, extension, response.headers.get("content-disposition")
         // ...
         openFile(filename);
    }
  }
  addEventListener("hashchange", (event) => {load_from_url()}); // do it whenever the hash changes
  load_from_url(); // do it the first time when page is loaded

@jo-chemla
Copy link
Contributor Author

Thanks, just refactored per your instructions, should be good to go!

I think it should be: from the header if available, otherwise from the url, and either way force the extension if the parameter is set.

This is how it works at the moment - rephrased, if extension provided, enforce it, otherwise, get type from header if available, otherwise from url.

if (extension_parsed) {
  filename = `model.${extension_parsed}`;
} else {
  // If extension is not provided by user, try to get it auto from content-disposition header of url extension 
  if (contentDisposition) {
    filename = contentDisposition.split('filename=')[1].split(';')[0];
  } else {
    try {
      filename = model_url.split('/').pop();

webassembly/app.html Outdated Show resolved Hide resolved
webassembly/app.html Outdated Show resolved Hide resolved
webassembly/app.html Outdated Show resolved Hide resolved
webassembly/app.html Outdated Show resolved Hide resolved
@jo-chemla
Copy link
Contributor Author

Thanks for the last suggestions, I included them and just pushed the final commit!

@Meakk Meakk merged commit f002d29 into f3d-app:master Sep 4, 2024
39 checks passed
jo-chemla added a commit to jo-chemla/stac-browser that referenced this pull request Sep 4, 2024
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.

4 participants