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

Replace "Invert" import option with more useful "Normal Map Invert Y" #39202

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented May 31, 2020

master version of #48693.

This can be used to invert a normal map's direction.

The "Invert" import option is no longer useful in Godot 4.0 since it uses height maps instead of depth maps in StandardMaterial3D. If you still need to use a depth map, SpatialMaterial3D features several properties to invert the height:

image

This closes godotengine/godot-proposals#785. This partially addresses #18299 (needs further testing).

Preview

Left: inverted
Right: not inverted

image

test_normal_map_direction.zip (build Godot with this PR before opening the project)

@clayjohn
Copy link
Member

Wouldn't it be more consistent to move this option to SpatialMaterial3D? Seeing as we have the other flip options there already.

We need to keep in mind that Godot users upgrading from 3.2 to 4.0 may have designed their assets with Godot in mind.

@Xrayez
Copy link
Contributor

Xrayez commented Aug 26, 2020

This can be used to invert a normal map's direction.

If this change is welcome, I'll make a 3.2 version of this PR that keeps the "invert" import option for compatibility.

I'm not sure if it's safe to assume that this option is not used for something else, because as of now it's quite general-purpose. Even if it's doesn't make sense to have this in the engine as of now, I'm sure other modules/plugins could possibly benefit from the existing logic, as this could cover more than just ability to invert green channel, I'd personally keep invert option and add whatever is necessary for normal map processing.

@fire
Copy link
Member

fire commented Jan 13, 2021

Is this pr considered rejected? @Xrayez makes an accurate point that normal fixing is different from invert.

@Calinou
Copy link
Member Author

Calinou commented Jan 13, 2021

@fire I don't think we've gotten enough opinions about this yet. Also, this pull request is only for 4.0; I can make a PR for 3.x that keeps the current "Invert" import option while adding a "Invert green channel" option.

@fire
Copy link
Member

fire commented Apr 26, 2021

I don't think more opinions will arrive. What is our consensus?

@akien-mga
Copy link
Member

For the reference, we discussed this in a PR review meeting today and agreed with the change. We agreed that referencing Y instead of green would likely be better, though there was some concern about potential confusing with flipping the texture vertically on the Y axis. We suggested using normal_map_invert_y or similar to make it clear that it's in the context of normal maps where inverting Y is a common concept.

This can be used to invert a normal map's direction.

The "Invert" import option is no longer useful in Godot 4.0 since
it uses height maps instead of depth maps in StandardMaterial3D.

This closes godotengine/godot-proposals#785.
@Calinou Calinou force-pushed the editor-import-invert-green-channel branch from 3998478 to 4ef71d7 Compare June 4, 2021 16:09
@Calinou Calinou changed the title Replace "Invert" import option with more useful "Invert green channel" Replace "Invert" import option with more useful "Normal Map Invert Y" Jun 4, 2021
@akien-mga akien-mga merged commit 7085c0d into godotengine:master Jun 5, 2021
@akien-mga
Copy link
Member

Thanks!

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.

Add an import option to make Godot read a normal map with inverted Y axis
5 participants