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

[esp/bindings_js] add back support for semantic information #245

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

msbaines
Copy link
Contributor

My earlier change where I allowed specifying the scene broke
semantic mesh support. Adding back semantic mesh support as
a URL parameter.

Motivation and Context

Get semantic mesh working again with WebGL port.

How Has This Been Tested

Tested via http://0.0.0.0:8000/esp/bindings_js/bindings.html?scene=17DRP5sb8fy.glb&semantic=mp3d

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 25, 2019
FS.createPreloadedFile("/", scene, scene, true, false);
Module.scene = scene;
const navmesh = scene.substr(0, scene.lastIndexOf(".")) + ".navmesh";
FS.createPreloadedFile("/", navmesh, navmesh, true, false);
if (config.semantic === "mp3d") {
const house = scene.substr(0, scene.lastIndexOf(".")) + ".house";
Copy link
Contributor

Choose a reason for hiding this comment

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

Convert extension replacement code and preload into a function now?

@@ -5,17 +5,25 @@ import VRDemo from "./modules/vr_demo";
import "./bindings.css";

Module.preRun.push(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, it will be better to move this to its own class which has functions like preloadMp3d etc.

@@ -5,17 +5,25 @@ import VRDemo from "./modules/vr_demo";
import "./bindings.css";

Module.preRun.push(() => {
let scene = "skokloster-castle.glb";
let config = {};
config.scene = "skokloster-castle.glb";
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, use defaults.js file here to export and load defaults (forgot in last PR).

FS.createPreloadedFile("/", house, house, true, false);
const semantic = scene.substr(0, scene.lastIndexOf(".")) + "_semantic.ply";
FS.createPreloadedFile("/", semantic, semantic, true, false);
}
Copy link
Collaborator

@mosra mosra Sep 25, 2019

Choose a reason for hiding this comment

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

Just checking we're all still on the same page -- once I'm done with the filesystem abstraction APIs (mosra/corrade#39), everything that's added here and in #239 could be moved back to C++, right?

Or, saying it differently -- what's done here is delaying the app startup (execution of main() is waiting on those files being loaded). If the same would be done via emscripten_async_wget*() APIs from C++ (which is what the filesystem abstraction will ... abstract), the loading could be done asynchronously, with a visual progress feedback directly in the app.

I guess it boils down to how much of this we want to do in pure JS and how much in C++ (and you know what I personally prefer :)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we are on the same page. I am looking forward to deleting this code once (mosra/corrade#39) lands. I'd much rather just use the C++ filesystem API then have to preload files into the virtual filesystem. This is definitely a temporary hack.

My earlier change where I allowed specifying the scene broke
semantic mesh support. Adding back semantic mesh support as
a URL parameter.
@msbaines msbaines merged commit 5524f5f into master Sep 25, 2019
@msbaines msbaines deleted the semantic branch September 25, 2019 20:28
eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants