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

AssimpImporter: populate right texture indices #66

Closed
wants to merge 2 commits into from

Conversation

@ikalevatykh
Copy link
Contributor

ikalevatykh commented Sep 20, 2019

Ambient/diffuse/specular texture indices are not populated in the current version. They are always 0 even a material has several textures or a model has several materials.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 20, 2019

Hahah guess what -- I just pushed the same fix, twenty minutes ago.

I wonder how did this happen, am I your evil twin? :D

@ikalevatykh

This comment has been minimized.

Copy link
Contributor Author

ikalevatykh commented Sep 20, 2019

Hhaha, should I already be scared? 8)

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 20, 2019

No, seriously, are we working on the same projects? :D Because your other bugfix, few days ago, got reported to me recently as well and there I could say "oh, it got just fixed, pull latest master".

@mosra mosra added this to the 2019.0b milestone Sep 20, 2019
@mosra mosra added this to TODO in Asset management via automation Sep 20, 2019
@ikalevatykh

This comment has been minimized.

Copy link
Contributor Author

ikalevatykh commented Sep 20, 2019

Heh, in any case, it is nice that it is already fixed, thank you for your titanic work)

Sorry for off-topic, have you plan to do something with this:

/** @todo: Support multiple meshes per node */
. Maybe add dummy child nodes for each of meshes? On another case, I will propose some fix.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 20, 2019

The fixes I'm doing are for various 3D-scanned datasets used by AI Habitat and I guess that's where we overlap :)

Sorry for off-topic

Not a problem -- I actually wanted to propose that next time you discover some bug or missing feature in the Assimp importer, let me know before you start implementing. In case I'm already on it, it'll save you time, if I'm not yet, then you'll save time me :)

Multiple meshes per node -- I did a similar fix for TinyGltfImporter some time ago in da58734, and I think it could be done very similarly here -- see the overview here. I did it this way to have animations attached to particular nodes still work properly, inserting dummy children would break those.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 20, 2019

So, my fix is in 503a6d9 and I had to give you a mention as well (8e6f499) :) Both pushed to master now.

Closing this one as resolved, but feel free to reuse this PR for further questions about the multi-mesh support (and thanks in advance for looking into it).

@mosra mosra closed this Sep 20, 2019
Asset management automation moved this from TODO to Done Sep 20, 2019
@ikalevatykh

This comment has been minimized.

Copy link
Contributor Author

ikalevatykh commented Sep 20, 2019

Ok, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.