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 latest Magnum with complete MeshData Python bindings #1999

Merged
merged 4 commits into from
Feb 8, 2023

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Feb 8, 2023

Motivation and Context

This brings another ~2 months of Magnum-side updates. A good part of the following list is a side-product of working on Habitat-unrelated things recently, nevertheless I feel it's good to mention everything for exposure:

  • All Corrade containers (ArrayView, StridedArrayView etc.) now have debug assertions checking out-of-bounds conditions. This should lead to much safer behavior in debug builds without explicitly having to wrap access in range-checking helpers, and since the asserts are compiled out in release builds, no perf degradation in deployed binaries. If some Habitat code starts asserting in debug builds after this upgrade, it means it had accidental OOB access before, not that a regression was introduced by this PR.
  • Updates to the BitArray and BitArrayView APIs and a new StridedBitArrayView container for multi-dimensional ranges of bits. Meant to be used for representing boolean fields in SceneData as well as various algorithms in the batch renderer and ECS-like data processing.
  • Convenient reading and writing of boolean and string arrays in Utility::Json and Utility::JsonWriter
  • Various improvements to efficiency of Utility::copy(), especially on debug builds. This call was showing up high in profiler for the batch renderer when loading composite files with ~200k meshes. The actual root cause of Utility::copy() being called too much during load will get fixed properly in the next round of batch renderer updates.
  • Improved and more efficient internal handling of static plugins. Apart from reducing binary and startup overhead they're no longer scheduled for recompilation after every CMake run, which can be a relatively significant cost in incremental builds.
  • Better preserving the global bit in StringView which helps when passing string literals to APIs that would otherwise have to make a copy of them.
  • Memory use of the TestSuite library with many test cases was reduced 14 times. (Not that it would be memory-inefficient before, it's now just even more efficient :P)
  • Ongoing cleanup of std::pair from the APIs, leading to more efficient data copies as Containers::Pair is properly trivially copyable.
  • String-related optimizations in the GL library, especially in shader compilation, avoiding unnecessary copies in many places that relied on std::string before. As a related change, I updated the compiled-in Habitat shader sources to be null-terminated strings for zero-copy compilation.
  • Updates to magnum-sceneconverter, exposing a new --phong-to-pbr option. This is however already available in the prebuilt binaries I'm providing.
  • A new UfbxImporter plugin supporting FBX and OBJ files, with proper PBR material support. Not enabled at the moment, but could eventually replace Assimp for these formats -- let me know if you want to have it used by default. It could also eventually mean not including Assimp by default (and reducing build times / binary sizes), as it would now be only needed for COLLADA files, with OBJ, FBX, PLY, STL and glTF already handled by other plugins.

But maybe the fanciest new thing is typed mesh attribute and index data access in Python, requested by @aclegg3. It's just a quick half-a-day feature, which among other things means you can now do this with a trade.MeshData:

>>> from magnum import trade
>>> importer = trade.ImporterManager().load_and_instantiate('AnySceneImporter')
>>> importer.open_file('Horse.glb')
>>> mesh = importer.mesh(0)
>>> list(mesh.indices[:10])
[0, 1, 2, 2, 1, 3, 4, 5, 6, 3]
>>> list(mesh.attribute(trade.MeshAttribute.POSITION)[:3])
[Vector(27.7, 86, -3.4), Vector(27.2, 116.9, -17.9), Vector(27.1, 103.1, 18.4)]
>>> list(mesh.attribute(trade.MeshAttribute.TEXTURE_COORDINATES)[:3])
[Vector(0.412052, 0.409609), Vector(0.336939, 0.515533), Vector(0.44862, 0.470601)]

The indices and attribute() APIs return a containers.StridedArrayView1D with a dynamic type, which implements a buffer protocol and thus provides an efficient zero-copy access to the underlying data, compatible with numpy arrays. It works for "usual" attribute types as well as various packed types like 16-bit texture coordinates or 8-bit vertex colors, with Python nicely abstracting over the differences to access everything through a single API. There's also mutable_indices and mutable_attribute() for those who need full power, and of course all other MeshData properties are exposed as well. Except for helpers like positionsAsArray(), which we don't need there, because Python is amazing and can do that all in a single API.

The same will be eventually exposed for trade.SceneData, allowing you to inspect actual scene hierarchies, node mesh/material assignments etc.

I also have more changes pending for SceneData, expanding glTF import/export capabilities, new MaterialTools utilities, as well as GL API additions for #1997. Those will be in the next batch.

How Has This Been Tested

🟢 on my side, not sure what's up with the Python and JS failures here.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 8, 2023
Copy link
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

Looks good to me. All the changes are in submodules of course though. :)

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

Thank you!

@0mdc
Copy link
Contributor

0mdc commented Feb 8, 2023

#2000 should fix the python_lint CI issue seen here.

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.

Thanks for implementing these additional features so quickly.
For others' context, we're starting to want asset loading in Habitat-lab python and we don't want a ton of new dependencies, so it is ideal to use Magnum for this.
Planning to integrate these changes with habitat-lab1108 for receptacle PLY mesh import.

@aclegg3 aclegg3 merged commit 2b98cff into main Feb 8, 2023
@aclegg3 aclegg3 deleted the update-magnum20 branch February 8, 2023 22:08
aclegg3 added a commit that referenced this pull request Feb 10, 2023
* Update Magnum submodules.

* Bundle shaders as null-terminated strings to avoid needless copies.

* tests: size_type is some internal Bullet typedef, don't use it.

---------

Co-authored-by: aclegg3 <alexanderwclegg@gmail.com>
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.

5 participants