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

[3.x] Add vertex color support to OBJ importer #76671

Merged
merged 1 commit into from May 15, 2023

Conversation

dioptryk
Copy link
Contributor

@dioptryk dioptryk commented May 2, 2023

Backport of #71033 to 3.x, with one notable difference.

I believe the original PR is implemented correctly, according to the specification, but does not actually work with programs like TreeIt or SpeedTree. This is because they output seven components after v, instead of six, like the original implementation assumes. Therefore, the 4.x version does not import the colors from OBJs created in the above applications, while my 3.x version does. The rest of the code is the same (only changes necessary for 4 -> 3 Godot version).

@dioptryk dioptryk requested a review from a team as a code owner May 2, 2023 10:09
@AThousandShips
Copy link
Member

AThousandShips commented May 2, 2023

I wouldn't necessarily call it a bug that we can't handle OBJ files that don't follow this format, but I'd suggest opening a bug for this for 4.x as it's not tracked anywhere as I can see

@Calinou
Copy link
Member

Calinou commented May 2, 2023

Reading the code, I thought the last component was the vertex color's alpha channel? What is the 7th component exported by TreeIt or SpeedTree?

@Calinou Calinou added this to the 3.6 milestone May 2, 2023
@dioptryk
Copy link
Contributor Author

dioptryk commented May 2, 2023

What is the 7th component exported by TreeIt or SpeedTree?

@Calinou I actually have no idea. I tested both options for color, I mean components 4-6 versus 5-7, and the former resulted in green vertices for the exported trees, so I would assume this version is correct :-) I do not think it's alpha, though. Why would SpeedTree export alpha values for leaves, if they're supposed to be alpha blended/alpha cut through the albedo texture anyway? Both apps are closed source and there seems to be no documentation, so I could not check the implementation, unfortunately.

The imported meshes seem fine - tree trunks are of singular color for both SpeedTree and TreeIt, so leaf shaders should work correctly.

@fire
Copy link
Member

fire commented May 2, 2023

It was mentioned that this diverges from 4.0? We typically merge to master before backporting enhancements.

@clayjohn
Copy link
Member

clayjohn commented May 2, 2023

I wonder if the reason there are 7 components is because speed tree is exporting in xyzwrgb format instead of xyzrgb as we currently assume.

It is valid for a .obj file to have xyz or xyzw. Color is a weird extension that everyone seems to support, but isn't technically specified anywhere. It is just tacked onto the end of the vertex line. So in theory, it should be possible to have 6 or 7 components for each vertex.

That being said, you mention that using 456 for color looks more correct than 567, so perhaps speedtree is storing something else per vertex

edit: Speedtree seems to indicate it is only passing 3-channel colors https://docs8.speedtree.com/modeler/doku.php?id=vertex_colors

Edit2: Its out of date, but this UE3 post references the alpha value from a SpeedTree vertex color https://docs.unrealengine.com/udk/Three/SpeedTree.html

@dioptryk
Copy link
Contributor Author

dioptryk commented May 3, 2023

Below you'll find the vertex color view from TreeIt and on the right Godot with vertex color as albedo with this PR enabled (components 4-6). So, it seems this is correct. As for the SpeedTree I apologize, but I must have mistaken one of my TreeIt trees for a SpeedTree tree, because I just checked all OBJ export presets in SpeedTree and none of them include vertex colors. It's just for FBX, it seems. So, if one needs OBJ from SpeedTree, they can only depend on having a separate material for leaves to animate them with a shader.

Trees

@clayjohn
Copy link
Member

clayjohn commented May 3, 2023

Ah okay. Thanks for updating. I took a look at TreeIt and it seems to use 456 for color and 7 for baked ambient occlusion. So for now it seems like using 456 and ignoring 7 is the way to go

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great!

Could you also make a PR targeting master changing the check to v.size() >= 7 so that 4.0 can work with TreeIt assets as well?

@dioptryk
Copy link
Contributor Author

dioptryk commented May 3, 2023

@clayjohn sure, I will. Thank you for finding my first contribution useful 😃

@akien-mga
Copy link
Member

Could you also make a PR targeting master changing the check to v.size() >= 7 so that 4.0 can work with TreeIt assets as well?

Bump :) I'd like to get the change merged in master before introducing a different in 3.x - otherwise there's a risk of never changing it in master, and since that's the branch which will be supported forever, that would be a shame :)

@dioptryk
Copy link
Contributor Author

PR for master ready (#77042) 😄

@akien-mga akien-mga changed the title Add vertex color support to OBJ importer [3.6] Add vertex color support to OBJ importer May 15, 2023
@akien-mga akien-mga changed the title [3.6] Add vertex color support to OBJ importer [3.x] Add vertex color support to OBJ importer May 15, 2023
@akien-mga akien-mga merged commit 567128b into godotengine:3.x May 15, 2023
13 checks passed
@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.

None yet

6 participants