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

Add PBR properties #137

Closed

Conversation

melikebatihan
Copy link
Contributor

Hi @mosra, @Squareys,
This is my first commit on the task of integrating PBR materials.

I have trouble understanding/discerning the boundaries of textures and materials, especially in the code. I added PBR properties in texture related properties but I am not sure if my approach is relevant at all for the solution of the task. Besides, I am quite confused about the definitions in assimp/material.h and not sure if I used these definitions correctly as property keys in AssimpImporter. An clarification would be great.

Another question I want to ask is the usage of the function _str(). I wonder why it is used in only some of the equality comparisons of property keys, e.g:

key == _str(AI_MATKEY_SHININESS) vs.
key == _AI_MATKEY_TEXTURE_BASE

@mosra
Copy link
Owner

mosra commented Mar 15, 2023

Hi,

thank you, and sorry for totally neglecting your other PR, #129 😅

What is the intended file format this would be used for? If it's FBX or OBJ then I'd say a less painful path to success would be for me to finally merge #136 (sorry on that front as well), because the UfbxImporter has proper and non-buggy support for PBR already.

If it's not FBX/OBJ, there's #91 where I attempted to do something similar, before @pezcode went ahead and integrated most of it for custom material import in #116. Would some code or at least notes from there be salvageable? I vaguely remember the PBR support got reworked in Assimp 5.1 (?), and this PR was made before, so maybe what's there is mostly obsolete now.

@mosra mosra added the scrapped label Oct 10, 2024
@mosra
Copy link
Owner

mosra commented Oct 10, 2024

Closing due to the reasons stated above -- for glTF, FBX and OBJ there's now GltfImporter / UfbxImporter with a much more robust PBR support than what Assimp can offer. And for Assimp itself, there's the barrage of issues listed in #91, so it's unfortunately not just about recognizing new texture types but also adding a pile of format-specific workarounds to be able to know whether a diffuse texture is indeed diffuse or it's PBR base color, etc.

@mosra mosra closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants