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.

Sign up for GitHub

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

Jump to bottom

Avoid setting PBR properties when they are not found on mtl file #4440

Merged
merged 6 commits into from Apr 13, 2022
Merged

Avoid setting PBR properties when they are not found on mtl file #4440

merged 6 commits into from Apr 13, 2022

Conversation

sacereda
Copy link
Contributor

This comes from #4424.

Metallic/roughness/sheen/clearcoat properties should only be added to the material if they appear on .mtl file.

Pinging @errissa here, feel free to suggest/change if you find anything odd.

@errissa
Copy link
Contributor

errissa commented Mar 14, 2022

I’ll leave it to the core developers to comment, if necessary, on the implementation details. However, the concept expressed with this PR to optionally add the PBR properties based on their presence in the MTL file makes sense to me and shouldn’t impact anyone using a PBR workflow with OBJ.

kimkulling
kimkulling requested changes Mar 16, 2022
View reviewed changes
Copy link
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

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

Just 2 small findings. Would you be able to solve these things? When done I will merge it. And maybe you can consider using std::optional?

Just a question from my side.

code/Common/Maybe.h
@@ -0,0 +1,29 @@
#pragma once
Copy link
Member

Choose a reason for hiding this comment

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

Copyright is missing

code/Common/Maybe.h
#pragma once
#include <assimp/ai_assert.h>

template <typename T>
Copy link
Member

Choose a reason for hiding this comment

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

Documentation is missing

@sacereda
Copy link
Contributor Author

Sure, I can check. I'm not really versed with std::optional, never used it before, and just tried to use something similar to STEPFile.h::Maybe, in case you were all used to this.

But yes, I can change it if you prefer to use std.

@turol
Copy link
Member

turol commented Mar 16, 2022

std::optional is C++17. What do we want our minimum standard requirement to be?

@sacereda
Copy link
Contributor Author

That might be a disadvantage, at least for this minimal use case.

@kimkulling
Copy link
Member

At the moment we have c++11 as our minimum. So I guess adding this class makes sense for me.

Thanks a lot for checking this out.

kimkulling
kimkulling approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

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

Adding the documentation will be added.

@kimkulling kimkulling merged commit b699d3a into assimp:master Apr 13, 2022
@kimkulling
Copy link
Member

Merged, thanks a lot for your contribution.

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

@kimkulling kimkulling kimkulling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sacereda @errissa @turol @kimkulling