needle-tools / usd-viewer

USD Web Viewer based on Autodesk's WASM USD bindings and a three.js Hydra render delegate
Other
59 stars 6 forks source link

feat: clean error messages #15

Closed beersandrew closed 1 month ago

beersandrew commented 1 month ago

wasm changes here: https://github.com/beersandrew/USD/commit/ff1b0a4e593ea87d17096009cdbdb5d5a90b8073

hybridherbst commented 1 month ago

Would it be possible to go back to the formatted version of the file?

Like this I can't really understand the changes: https://github.com/needle-tools/usd-viewer/pull/15/files#diff-eaca4b0557887de7c727d9ff3fee5dbb57c40a95267fc1277f2f792dadd1d905

beersandrew commented 1 month ago

Would it be possible to go back to the formatted version of the file?

Like this I can't really understand the changes: https://github.com/needle-tools/usd-viewer/pull/15/files#diff-eaca4b0557887de7c727d9ff3fee5dbb57c40a95267fc1277f2f792dadd1d905

I will try. there are two things that make that awkward for now:

  1. Finding a good formatter: the best i can find is a deprecated js formatter in visual studio code that breaks index.html when i save it so I don't really want that plugin on all of the time, and sometimes it requires me to put in my own manual new lines for it to format new sections of the file.
  2. Knowing when new c++/cmake changes require emHdBindings.js changes: the majority of these changes all of the changes are within the wasm file but everything gets built at the same time and it's not obvious to me when the wasm (c++ / cmake) changes depend on emHdBindings.js and when they dont, in the case that there are new required emHdBindings.js changes, they will not include the new args change that was made or other custom changes, it was great to be able to find a way to include the custom memory code at build time but i don't think there will always be an option to do that

I will:

  1. look for some commandline lint function to run, unless you have something you know about or have specific formatting rules you want
  2. find the change you made in the past with the emHdBindings.js file and format the existing file
  3. see if the new changes i make actually need to be merged with the existing file or not, hoping they don't but if they do then i'll need some good way of being able to patch or format or something at build time
beersandrew commented 1 month ago

@hybridherbst is there that you added other than the 2 parts below.

  1. adding this part to the arguments:
moduleArg = {
    // module overrides can be supplied here
    locateFile: (path, prefix) => {
      if (!prefix) prefix = _scriptDir.substr(0, _scriptDir.lastIndexOf('/') + 1);
      return prefix + path; 
    },
    ...args
}
  1. Including readdir & analyzePath at the end
    Module["FS_readdir"]=FS.readdir;
    Module["FS_analyzePath"]=FS.analyzePath;
beersandrew commented 1 month ago

Unfortunately i guess this first diff is not going to be very useful, but now future diffs should be better. sorry about that

hybridherbst commented 1 month ago

Yup I think it's 29329896c9335c4a3ba6b8e1092b79bfebaad5b7 and a combination of f50f71707760f099eea14c15ddd0388503453076 and what we did while in the call to get the package to work with a bundler

hybridherbst commented 1 month ago

I think if formatting is always the same it should also be relatively easy to spot what has changed in emHdBindings.js; in the past I also just rebased my changes on the new (formatted) version and that usually works fine (git can figure it out)

hybridherbst commented 1 month ago

Also I think when I first formatted it it was just the regular prettier package that comes with VS Code I believe b63c75307b41ce248ed7415ec65113fbf4f053d1

beersandrew commented 1 month ago

Cool I think this branch is ready then

hybridherbst commented 1 month ago

Thank you, merged!

There is one follow-up issue: Currently loading a file with an error leads to an Abort and subsequently I can't load other files anymore. Is this something that we can avoid? Steps:

image