-
Notifications
You must be signed in to change notification settings - Fork 505
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
Add support for glTF multi-receptacle assets #1249
Conversation
…antiating a unique Receptacle.
# Import mesh data from a scene format (e.g. glTF) | ||
scene_ix = importer.default_scene | ||
if scene_ix != -1: | ||
scene_data = importer.scene(scene_ix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With facebookresearch/habitat-sim#2066 this whole code could be replaced with something like this, assuming the goal is to get back a list of meshes that have their positions transformed according to the scene hierarchy. Basically it's about porting this example snippet to Python:
meshes = [importer.mesh(i) for i in range(importer.mesh_count)]
# Mesh referenced by mesh_assignments[i] has a corresponding transform in
# mesh_transformations[i]. Association to a particular node ID is stored in
# scene.mapping(mn.trade.SceneField.MESH)[i], but it's not needed for anything
# here.
mesh_assignments: cr.containers.StridedArrayView1D = \
scene.field(mn.trade.SceneField.MESH)
mesh_transformations: List[mn.Matrix4] = \
mn.scenetools.flatten_transformation_hierarchy3d(scene, mn.trade.SceneField.MESH)
assert len(mesh_assignments) == len(mesh_transformations)
# A mesh can be referenced by multiple nodes, so this can't operate in-place.
flattened_meshes: List[mn.trade.MeshData] = []
for mesh_id, transformation in zip(mesh_assignments, mesh_transformations):
flattened_meshes += [mn.meshtools.transform3d(mesh_id, transformation)]
The flattened_meshes
is then very close to the mesh_data
array here. You may however still want to store the MeshData
instances or (references to) the position arrays from them instead of a List[mn.Vector3]
, as that's significantly more memory-efficient (as far as I can see, each mn.Vector3
instance stored in a list is 64 bytes, a lot more than the three floats per position when stored inside MeshData
).
If you'd want to reduce the memory use even more, you could discard also all unused attributes except positions. I.e. do this in the mesh loading code:
mesh = ...
# This operation affects only the metadata -- the remaining attributes stay in
# the vertex buffer but are no longer referenced and thus treated like padding
mesh = mn.meshtools.filter_only_attributes(mesh, [mn.trade.MeshAttribute.POSITION])
# This forces a repack of the mesh, throwing away all unused data (by default
# it keeps the original layout including all padding if it's already interleaved)
mesh = mn.meshtools.interleave(mesh, mn.meshtools.InterleaveFlag.NONE)
(Wrote this off the top of my head, can't guarantee it compiles or works. But should be pretty close to a working state.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the API updates and suggestions here.
I refactored the code to use the new APIs and took your suggestion to store the MeshData
directly instead of lists.
However, I ran into some issues with the memory improvement suggestions. I think these could be added in a later PR, but for reference:
mesh = mn.meshtools.filter_only_attributes(mesh, [mn.trade.MeshAttribute.POSITION])
Seems to corrupt theindices
:
len(self.mesh_data.indices) = 6
len(self.mesh_data.attribute(mn.trade.MeshAttribute.POSITION)) = 4
index = 0
self.mesh_data.indices[index] = 32349
mesh = mn.meshtools.interleave(mesh, mn.meshtools.InterleaveFlag.NONE)
maybe need some additional plugin or compile flags?
AttributeError: module '_magnum.meshtools' has no attribute 'InterleaveFlag'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm getting the following terminal output even with export MAGNUM_LOG=quiet
:
PluginManager::Manager: duplicate static plugin AssimpImporter, ignoring
PluginManager::Manager: duplicate static plugin AnySceneImporter, ignoring
PluginManager::Manager: duplicate static plugin AnyImageImporter, ignoring
Trade::GltfImporter::scene(): node 0 extras modelNormal property is Utility::JsonToken::Type::Object, skipping
Trade::GltfImporter::scene(): node 0 extras modelObb property is Utility::JsonToken::Type::Object, skipping
Trade::GltfImporter::scene(): node 1 extras modelNormal property is Utility::JsonToken::Type::Object, skipping
Trade::GltfImporter::scene(): node 1 extras modelObb property is Utility::JsonToken::Type::Object, skipping
Trade::GltfImporter::scene(): node 2 extras modelNormal property is Utility::JsonToken::Type::Object, skipping
Trade::GltfImporter::scene(): node 2 extras modelObb property is Utility::JsonToken::Type::Object, skipping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export MAGNUM_LOG=quiet
Unlike with HABITAT_LOG
, this affects only the startup log when creating a GL context, not everything. Implementing that would either mean getenv()
on every print (which is very slow), or need some global "logging context" singleton which -- while completely fine in Habitat -- is a rather nasty thing to have in a library / middleware setting and would introduce more problems than it would solve.
From the application perspective, silencing the output is possible with scoped redirection, such as this in C++ (relevant docs):
{
Mn::Warning redirectWarnings{nullptr};
// all warnings coming from Magnum are silenced now
}
// all warnings coming from Magnum are printed again
Hm, so maybe I could expose something similar to Python as well. I'm thinking it could be doable with a context manager and a with
statement, although I'd need to look up how to do that with pybind. Would this make sense?
import magnum as mn
with mn.redirect_warnings(None):
# all warnings coming from Magnum are silenced now
# all warnings coming from Magnum are printed again
Another idea I had was to introduce Flag::Quiet
on the importer interface, and make the plugins respect it. There's Flag::Verbose
already. I'll add that, this seems good to have in any case.
Trade::GltfImporter::scene(): node 0 extras modelNormal property is Utility::JsonToken::Type::Object, skipping
I was asking about these on Slack a while ago: https://cvmlp.slack.com/archives/C048W359BFC/p1677178392534799?thread_ts=1677014872.077569&cid=C048W359BFC
My goal is to make them all easily available through Python, so you could for example get them as scene.field(importer.scene_field_for_name('modelNormal'))
, which also means having them implicitly preserved though all possible scene transformations and exported back when you save a file again. There's two options as I said in that thread -- either those get changed to not be in nested objects so the existing importer code can fetch them, or I update the glTF import/export code to handle them, somehow. I didn't get to the latter yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module '_magnum.meshtools' has no attribute 'InterleaveFlag'
It's InterleaveFlags
, sorry. (I just typed that code snippet out from memory.)
Seems to corrupt the
indices
Uh oh. Working on getting that fixed 😉 The root cause is that meshtools.filter_*_attributes()
modify the metadata but reference the original mesh, and Python wasn't made aware of the original still being referenced. I'm making some additional changes to have these accidents caught automatically to prevent this from happening again.
I think this should work for now, I'll ping you once I have the overal robustness improved:
mesh = ...
# `mesh_filtered` now refers to data in `mesh`
# (`mesh_filtered.vertex_data_flags` isn't `OWNED`)
# until i make a proper fix, it's important to use a different variable
# which should tell Python's GC to not delete the original
mesh_filtered = mn.meshtools.filter_only_attributes(mesh, [mn.trade.MeshAttribute.POSITION])
# makes the mesh standalone / self-owned again
# (`mesh.vertex_data_flags` is `OWNED`)
mesh = mn.meshtools.interleave(mesh_filtered, mn.meshtools.InterleaveFlags.NONE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this appears to be working now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, so maybe I could expose something similar to Python as well. ...
with mn.redirect_warnings(None): # all warnings coming from Magnum are silenced now...
Another idea I had was to introduceFlag::Quiet
on the importer interface, and make the plugins respect it. ...
Either of these should functionally solve the problem from my perspective. In general a solution to quieting logs would be useful once we get to scaled training or running batched processes. 👍
Trade::GltfImporter::scene(): node 0 extras modelNormal property is Utility::JsonToken::Type::Object, skipping
I was asking about these on Slack a while ago: https://cvmlp.slack.com/archives/C048W359BFC/p1677178392534799?thread_ts=1677014872.077569&cid=C048W359BFC
I generally agree that accessing these nested objects would be valuable in the long run, but currently I just want to suppress the noise. 🙂
…e singletons instead of function variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I would prefer if @mosra gave it an approval before merging.
…ilter_interleave_mesh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* add support for glTF multi-receptacle assets with each sub-mesh instantiating a unique Receptacle. * update receptacle names in test for new code * refactor to use Meshdata internally * convert triangle-like primitives to triangles.
* add support for glTF multi-receptacle assets with each sub-mesh instantiating a unique Receptacle. * update receptacle names in test for new code * refactor to use Meshdata internally * convert triangle-like primitives to triangles.
* TriangleMeshReceptacles and RearrangeGenerator Improvements (#1108) * updates to rearrange episode generation to support mesh receptacles and improved debugging * docstrings and typing for DebugVisualizer util * mesh receptacle debug_draw with global transforms * add some tests for sampling accuracy * bugfix - use default_sensor_uid instead of hardcoded "rgb" in DebugVisualizer * adjust debug peek target to bb center to better capture objects not centered at COM * add debug circles to the peek API * add peek_scene variant for easliy getting images of a full scene or stage. * RearrangeGenerator cleanup and unstable object culling (#1248) * fix unstable object culling compatibility with target sampling and change default to use this feature * add comments and typing * fix bug where only the first of a target sampler's object samplers was used * Add support for glTF multi-receptacle assets (#1249) * add support for glTF multi-receptacle assets with each sub-mesh instantiating a unique Receptacle. * update receptacle names in test for new code * refactor to use Meshdata internally * convert triangle-like primitives to triangles. * refactor to use Receptacle unique_name for sample filtering. Add support for filter files configured in scene instance config user_defined fields. * refactor snapping functionality to improve stability by estimating consistency of support surface from bb corners projected in gravity direction * Update habitat-lab/habitat/sims/habitat_simulator/sim_utilities.py Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com> * turn off additional stability culling for now * bugfix * fix margin issues * Add TODO comment * Misc changes to work with merge * Add missing import * Fix target sampling bug * Remove unneeded code * Add changes to test previously missed --------- Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com> Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com>
* add support for glTF multi-receptacle assets with each sub-mesh instantiating a unique Receptacle. * update receptacle names in test for new code * refactor to use Meshdata internally * convert triangle-like primitives to triangles.
* add support for glTF multi-receptacle assets with each sub-mesh instantiating a unique Receptacle. * update receptacle names in test for new code * refactor to use Meshdata internally * convert triangle-like primitives to triangles.
Motivation and Context
Current
TriangleMeshReceptacle
parsing supports only single mesh PLY files. This PR adds support for multi-mesh glTF files.In the multi-mesh case, a unique
TriangleMeshReceptacle
instance is created for each sub-mesh.For example, if 3 sub-meshes exist in the
1234.glb
file referenced from receptacle metadata like:The parser will produce 3 TriangleMeshReceptacles named:
NOTE some un-quiet-able logging is expected from receptacle loading and import initialization.
This will be fixed soon in a follow-up change to Magnum APIs introducing log quieting functionality.
E.g.:
How Has This Been Tested
Locally with .glb and .ply files.
Should update test_assets in habitat-sim to include new .glb receptacles.
Types of changes
Checklist