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 option to invert roughness maps. #101988

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rbwldrf
Copy link

@rbwldrf rbwldrf commented Jan 24, 2025

Some modelling workflows have smoothness maps instead of roughness maps. I thought it would make things easier if there was an option to invert it in editor.

Feel free to uncomment the SConstruct file, I couldn't build without commenting it out because I'm on SCons 4.8.1

@rbwldrf rbwldrf requested a review from a team as a code owner January 24, 2025 14:37
@AThousandShips AThousandShips changed the title Added option to invert roughness maps. Add option to invert roughness maps. Jan 24, 2025
SConstruct Outdated Show resolved Hide resolved
scene/resources/material.h Outdated Show resolved Hide resolved
@rbwldrf
Copy link
Author

rbwldrf commented Jan 24, 2025

OK, is there anything else I should fix?

@AThousandShips
Copy link
Member

Nope that's all resolved

@rbwldrf rbwldrf requested a review from a team as a code owner January 24, 2025 15:30
@Calinou
Copy link
Member

Calinou commented Jan 24, 2025

Similar to the Normal Map Invert Y import option, this could be handled at import time so you don't need different shader versions when different materials use different texture conventions. This in turn leads to better performance and faster load times.

In fact, Normal Map Invert Y can already achieve this! This is because it inverts the texture's green channel on import, regardless of what the texture actually contains. Either use an ORMMaterial3D (green is the roughness channel in an ORM map), or use a StandardMaterial3D and set the roughness channel to green instead of red.

@rbwldrf
Copy link
Author

rbwldrf commented Jan 24, 2025

Huh. That's a valid point. I just thought it would be a nice QOL feature similar to Heightmap Flip Texture.

@RedMser
Copy link
Contributor

RedMser commented Jan 24, 2025

#99676 should already do what you want (arbitrarily remap color channels of a texture at import-time).

@rbwldrf
Copy link
Author

rbwldrf commented Jan 24, 2025

That's a good implementation! I honestly hadn't noticed that, which is most likely the culprit of my proposal.

Keeping that in mind, and I'm playing devil's advocate here, I suppose this proposal could help prevent other users from missing that setting, marginally improving UX at the expense of whatever performance hit the addition of a per-material variable would add.

I get that bloating the BaseMaterial3D like this is not in the interest of most, and I totally understand if this doesn't get added. I'm just spitballing an (admittedly flawed) alternative solution to a problem caused by a lack of understanding of the engine and a want for a quick and easy tinker that I rushed without research towards existing fixes to the problem and was a little too eager to share.

@fire
Copy link
Member

fire commented Jan 24, 2025

The previous discussion rejected adding a boolean to invert roughness maps to the material system. I don't currently have the reference links to it.

The reasoning is it adds an unused feature to the material system.

However, a texture import setting that rewrites the texture might be okay.

@AThousandShips
Copy link
Member

For context one past rejected PR:

But the rejection was not in writing anywhere there

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

Successfully merging this pull request may close these issues.

5 participants