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

Update to Magnum 2019.10 #334

Merged
merged 4 commits into from
Nov 8, 2019
Merged

Update to Magnum 2019.10 #334

merged 4 commits into from
Nov 8, 2019

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Nov 1, 2019

Motivation and Context

This updates Habitat to use Magnum 2019.10. Read the release announcement here.

Highlights of the release and this PR:

  • magnum submodules updated to the v2019.10 tag, bundled cmake Find modules updated as well
  • all remaining primitives exposed to python (hi, @aclegg3!)
  • bundles Basis Universal transcoder (because the repo is too huge to be a git submodule)
  • enables the BasisImporter plugin and sets up a driver-dependent target format (yeah, there's a huge pile of ifdefs... welcome to the GL extension world)
  • makes import of compressed images work
  • the engine is now ready for multi-threaded asset import
  • the engine implements device selection in WindowlessEglApplication (tho not CUDA device selection at the moment, so it can't fully replace the habitat code yet)
  • the engine improves other things that mostly benefit Windows users on non-excellent drivers (sorry, didn't have any access to a suitable hardware to figure out the macOS buffer texture crash, @bigbike)

How Has This Been Tested

On my side (Linux) things build, viewer views.

On your side, can you please confirm that the viewer works with the *.basis.glb files I uploaded on Slack yesterday? Ideally also on all web browsers you can access. Thanks! 🙏

To be clear, the v2019.10 tag in all repos. Also updating all Find
modules to latest to make system-magnum builds work.
https://github.com/BinomialLLC/basis_universal/tree/2f43afcc97d0a5dafdb73b4e24e123cf9687a418

Unfortunately the repo has >200 MB history due to huge files added in
the past, so it's not feasible to add it as a Git submodule. Disabling
the (huge) BC7m6 format and a few others that Magnum has no support for,
the BC7m6 file is empty because the sources include it unconditionally.
Fix in BinomialLLC/basis_universal#91.
It was used internally, but never exposed to CMake GUI / ccmake.
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 1, 2019
Copy link
Contributor

@erikwijmans erikwijmans left a comment

Choose a reason for hiding this comment

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

Works on MacOS for me (can't test linux myself ATM tho).

Bringing this up here also, what are people's thoughts on just auto-loading the basis mesh if it exists? I changed the logic in ResourceManager::loadGeneralMeshData to be

 const std::pair<std::string, std::string> splitFilename =
        Cr::Utility::Directory::splitExtension(filename);
    const std::string basisFilename = Cr::Utility::String::join(
        {splitFilename.first, ".basis", splitFilename.second}, "");
    const std::string filenameToLoad =
        Cr::Utility::Directory::exists(basisFilename) ? basisFilename
                                                      : filename;

    if (!importer->openFile(filenameToLoad)) {
      LOG(ERROR) << "Cannot open file " << filenameToLoad;
      return false;
    }

and that seems to be pretty nice :)

Basis meshes also load much faster, which is awesome!

@mosra
Copy link
Collaborator Author

mosra commented Nov 3, 2019

Like I mentioned on Slack, I'm not happy about having two sets of glb files, one with JPEGs and one with Basis, as that unnecessarily duplicates all vertex data. Alternative options (glTF is pretty flexible in this regard):

  • have the glb files with JPEGs, and have basis files referenced externally (basically a bunch of *.basis files lying around next to the *.glb)
  • have the glb files with basis and JPEGs referenced externally (inverse of the above -- that makes it self-contained and smaller for 90% users, and those who feel like they need the JPEGs for quality or whatever, still have the possibility to use those)
  • either of the above, but instead of having 300 separate files, all external images packed into a single *.bin that's then referenced from the glb as an additional buffer
  • bundle both JPEGs and basis into the same glb (unfortunately that makes file size about 1.5x larger, bad for downloads)

Basis meshes also load much faster, which is awesome!

It might be related to having to upload only 1/4 of the memory, but I have a hunch it's mostly because for JPEGs the code generates mips on-the-fly, while for Basis currently not (I want to have mips encoded in the files directly, but need to finish mosra/magnum#369 for that first).

Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

LGTM. Everything is running fine on local tests. 👍

@@ -53,6 +57,7 @@
#endif

namespace Cr = Corrade;
namespace Mn = Magnum;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this! 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updating the code as I go :) But only as long as it doesn't add unrelated changes to the diff.

@erikwijmans
Copy link
Contributor

We could distributed just the basis versions and maintain an option to the old jpeg versions? I am also fine with packaging both jpeg and basis into a single mesh (for reference, CV datasets taking up terabytes is common, so going from 10GB of data to 20GB of data probably won't hurt anyone too much :))

@mosra
Copy link
Collaborator Author

mosra commented Nov 5, 2019

@erikwijmans okay, then I'll update the gltf2basis script to keep both JPEGs and Basis files (and also update it with the Gibson fixes). Will ping you back once that's done -- the final repack shouldn't need any additional processing on the cluster, only reusing the already converted basis files.

@msavva
Copy link
Collaborator

msavva commented Nov 5, 2019

@erikwijmans @mosra regarding packaging both basis and jpeg into gltf: do we anticipate a non-trivial number of people needing to fall back to jpeg? If not, I think it's better to opt for basis only in the main packages and have jpeg in a parallel bin file (i.e. option 3 above). That way we get the benefit of smaller file sizes for the most common use case... It is true that many datasets are large, but we can do better, right? :-)

@erikwijmans
Copy link
Contributor

True true. Maybe we distribute basis-only meshes and then have an option for people to get the original versions if they want them for some reason but heavily recommend the basis versions?

My primary concern is fragmentation, i.e. people thinking that the basis version of mp3d/gibson/etc is somehow "different" and that you can't compare results between the "original" versions and the basis versions.

@msbaines
Copy link
Contributor

msbaines commented Nov 5, 2019

Size is important for WebGL. For WebGL, it really makes sense to have a single container format. For the project in general, we may want to convert and have all datasets available in a single container format, gltf2 w/basis textures, but also support natives formats in order to reduce friction.

@@ -20,7 +20,7 @@ mkdir -p build_js
cd build_js

cmake ../src \
-DCORRADE_RC_EXECUTABLE=../build_corrade-rc/deps/corrade/src/Corrade/Utility/corrade-rc \
-DCORRADE_RC_EXECUTABLE=../build_corrade-rc/RelWithDebInfo/bin/corrade-rc \
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unrelated and assumes the build-type of corrade-rc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With 2019.10 magnum is by default putting all binaries and libraries into a single location to make it easier to load dynamic plugins from the build dir (and also avoid DLL hell for Windows users). So this is a direct result of that. I could override that with CMAKE_RUNTIME_OUTPUT_LOCATION for the CI script if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, so binaries are no longer being built in the usual location of CMAKE_CURRENT_BINARY_DIR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not, unless you set one of the CMAKE_{RUNTIME,LIBRARY,ARCHIVE}_OUTPUT_LOCATION, in which case that setting is preserved. This also affects only Magnum used as a subproject, in case of a system-wide installed Magnum it's assumed PATH is set up properly and plugins are in expected locations, so there's less reasons to mess with this. Docs for reference.

This makes the workflow easier for majority of users and so it's enabled by default. Habitat builds everything including plugins as static, so there it doesn't make much difference (although once I got used to binaries being all in one place, I have to admit it's much more convenient). If you wish to preserve the original behavior, I can override this.

Copy link
Contributor

@msbaines msbaines Nov 5, 2019

Choose a reason for hiding this comment

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

I thought Release was the default build. Maybe set CMAKE_BUILD_TYPE explicitly above when corrade-rc is built. That way, we aren't relying on a default.

Copy link
Collaborator Author

@mosra mosra Nov 5, 2019

Choose a reason for hiding this comment

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

RelWithDebInfo is set as default here in CMakeLists.txt and here as the opposite of Debug in setup.py. It was done like this before my time so I don't know the reasons behind, but I'd say it's a pretty strong default :)

(AFAIK, CMake's default build type is an empty string, so neither Debug, Release, nor RelWithDebInfo.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.

@mosra
Copy link
Collaborator Author

mosra commented Nov 5, 2019

Size is important for WebGL.

One related thing that's on the filesystem abstraction roadmap is partial downloading -- if the *.glb would be constructed in a way that puts (colored) vertex data first, basis textures second and JPEGs last, the viewer could first show just colored vertices, show Basis textures, once they get downloaded, and stop the download before the JPEGs appear in the file.

KTX2 aims to push that further (I hope it didn't get removed from the draft since I looked last time) -- small mips are put early in the file and larger mips later, so progressing the download would basically cause a "progressive refinement", similarly to progressive JPEGs if you remember, but here over the whole glTF file.

@bigbike
Copy link
Contributor

bigbike commented Nov 5, 2019

I would pick option 2 + 3 in @mosra's comment.
Bake the basis images in the glb while packing all the jpegs in one binary, which is referenced externally.

After we make a decision, I wonder who will create the basis mesh?

  • Users use the magnum conversion tool to do it by themselves.

  • When user load a traditional glb file for the 1st time, our simulator can convert it on the fly, and store it in the same folder with the name "modelname.basis.glb". Next time, our simulator by default load this "new" basis glb instead, unless user enforces using the old one.

  • Other options (We preprocess them? For example, we process all the replica models, and put the files in the habitat sub-folder. But I am not sure if we would do it for MP3D dataset since FB does not own it.)

@msavva
Copy link
Collaborator

msavva commented Nov 5, 2019

I would pick option 2 + 3 in @mosra's comment.
Bake the basis images in the glb while packing all the jpegs in one binary, which is referenced externally.

After we make a decision, I wonder who will create the basis mesh?

  • Users use the magnum conversion tool to do it by themselves.
  • When user load a traditional glb file for the 1st time, our simulator can convert it on the fly, and store it in the same folder with the name "modelname.basis.glb". Next time, our simulator by default load this "new" basis glb instead, unless user enforces using the old one.
  • Other options (We preprocess them? For example, we process all the replica models, and put the files in the habitat sub-folder. But I am not sure if we would do it for MP3D dataset since FB does not own it.)

Agreed. I also vote for going with glb containing basis textures + optional single bin file with jpeg textures.

Likely the best option from above is to do preprocessing on our end so that users have a smooth out-of-the-box experience. Just to clarify: we do not own or distribute any of the datasets. We can coordinate with the original authors so that they can distribute updated files from their end along with the original data.

@bigbike
Copy link
Contributor

bigbike commented Nov 5, 2019

@msavva : I see your point (to give the user smooth "out-of-box" experience). Then that means we need to work with the original authors on it. Also another concern is the scalability. What if the mesh authors create and add new meshes in the future? Then we need to work with them again.

What do you think of the idea, which is to save the basis mesh at the 1st time, and use them by default after that? There is no extra work on user side, which means the same "out-of-box" experience. And such a solution does not have the aforementioned concerns.

@msavva
Copy link
Collaborator

msavva commented Nov 6, 2019

@msavva : I see your point (to give the user smooth "out-of-box" experience). Then that means we need to work with the original authors on it. Also another concern is the scalability. What if the mesh authors create and add new meshes in the future? Then we need to work with them again.

We know the authors well and can work with them closely 🙂

What do you think of the idea, which is to save the basis mesh at the 1st time, and use them by default after that? There is no extra work on user side, which means the same "out-of-box" experience. And such a solution does not have the aforementioned concerns.

I don't think this is feasible given that we do not include the entirety of the basis library dependency (only the transcoder part).

@bigbike
Copy link
Contributor

bigbike commented Nov 6, 2019

We know the authors well and can work with them closely 🙂

This is awesome!

I don't think this is feasible given that we do not include the entirety of the basis library dependency (only the transcoder part).

Oh, I see. This is a good point.

@erikwijmans
Copy link
Contributor

erikwijmans commented Nov 6, 2019

Creating the basis meshes is definitely not something we want users to have to do. It took about 12 hours on 800 CPU cores to create all the basis meshes for MP3D :)

@mosra
Copy link
Collaborator Author

mosra commented Nov 8, 2019

To unblock (and merge) this -- I think the decision about what to pack and how is pretty orthogonal to this upgrade (here the code is only updated to recognize Basis textures, not to prefer one format over the other), so I would propose merging this PR as-is and then work on the Basis/JPEG logic/preference in another PR.

Opinions? :)

@bigbike
Copy link
Contributor

bigbike commented Nov 8, 2019

It seems reasonable, and it will also unblock #338.
Thanks a lot for the amazing work.

@mosra mosra merged commit 461003a into master Nov 8, 2019
@mosra mosra deleted the magnum-2019-10 branch November 8, 2019 20:38
eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020
…search#334)

Renaming in  docs: pointgoal -> pointgoal_with_gps_compass
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
* Update to Magnum 2019.10.

To be clear, the v2019.10 tag in all repos. Also updating all Find
modules to latest to make system-magnum builds work.

* Bundle Basis Universal transcoder and enable the BasisImporter plugin.

https://github.com/BinomialLLC/basis_universal/tree/2f43afcc97d0a5dafdb73b4e24e123cf9687a418

Unfortunately the repo has >200 MB history due to huge files added in
the past, so it's not feasible to add it as a Git submodule. Disabling
the (huge) BC7m6 format and a few others that Magnum has no support for,
the BC7m6 file is empty because the sources include it unconditionally.
Fix in BinomialLLC/basis_universal#91.

* assets: setup for Basis target format, compressed image import.

* List a forgotten USE_SYSTEM_GLFW option.

It was used internally, but never exposed to CMake GUI / ccmake.
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.

7 participants