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

wish: multi-primitive loading for GLTF #48

Closed
xqms opened this Issue Aug 21, 2018 · 5 comments

Comments

2 participants
@xqms

xqms commented Aug 21, 2018

First of all, nice work on the engine! I'm trying to use magnum for generating renderings in a deep learning pipeline.

I currently get this one a lot and end up with parts of my meshes missing:

Trade::TinyGltfImporter::mesh3D(): more than one primitive per mesh is not supported at the moment, only the first will be imported

How involved would it be to implement this properly? I'm new both to magnum and GLTF, so I'm not sure I understand all the implications (Does magnum support multiple primitive types for one mesh? ...).

If you give me some roadmap of what needs to be done, I could probably work on this :-)

Here is an example mesh file: https://uni-bonn.sciebo.de/s/psUuQOVBmxAfwXU

@mosra

This comment has been minimized.

Owner

mosra commented Aug 22, 2018

Hi, thanks for the appreciation!

The hardest thing about adding such support is preserving backwards compatibility. But I just got an idea how this could be implemented in an almost-backwards-compatible manner and could be pretty straighforward to do.

Basically now there's Trade::ObjectData3D::instance() that has 1:1 mapping to Trade::MeshData3D. After the change each multi-primitive mesh will be multiple Trade::MeshData3D instances and there would be a new ObjectData3D::instances() function, returning a list of meshes that belong to given object/node. Would this fit your use case?

At first I thought about adding support for "multiple primitives" directly to Trade::MeshData3D, but that doesn't really make sense I think, as the "primitives" are usually completely independent.

There are two options: either you can work on this -- if you want to -- and I'll happily give you pointers what should go where, what needs to be updated, how the tests need to be extended, what to do about backwards compatibility etc.... Or if you can wait a bit, I can add the support myself, as I have the design and scope pretty clear in my head. But not immediately, I'm now working on other things such as HiDPI support and wanted to have these done as soon as possible. So probably next week earliest. Is that okay?

@mosra mosra added this to TODO in Asset management via automation Aug 22, 2018

@xqms

This comment has been minimized.

xqms commented Aug 23, 2018

Wow, thanks for the detailed answer. It seems a bit too involved for me as a newcomer ;-)

In the meanwhile I managed to solve my immediate issues by importing the meshes as OBJ through the AssimpImporter, which works nicely. So proper GLTF support (while it would be nice) is not high priority for me anymore.

@xqms

This comment has been minimized.

xqms commented Aug 31, 2018

A further note on this topic: Importing glTF via AssimpImporter seems to work fine as well. I haven't tested it thoroughly yet, but at first glance everything works.

Feel free to close the ticket (unless you want the TinyGLTFImporter to handle this as well).

@mosra mosra added this to the 2018.0c milestone Aug 31, 2018

@mosra mosra referenced this issue Aug 31, 2018

Closed

2018.10 release #265

56 of 56 tasks complete
@mosra

This comment has been minimized.

Owner

mosra commented Aug 31, 2018

Sorry for not being able to get to this yet. I'm in a process of finishing the 2018.08 release and this is definitely among the things that would be really nice to have in, but there is still a lot to do in other areas as well. I'll see what I can do.

Using AssimpImporter should work in most cases and I tried to make both importers give consistent output, but one thing that Assimp can't handle (and TinyGltf can) is animations. Not sure if you need them or not.

@mosra

This comment has been minimized.

Owner

mosra commented Sep 9, 2018

Hi, I finally got to implement this in da58734. I tested with various models and it seems to work well. It's done a bit differently than I outlined above (no code changes on your side are required), see the docs for details.

Please comment here if you find any issues ;)

@mosra mosra closed this Sep 9, 2018

Asset management automation moved this from TODO to Done Sep 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment