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 vertex color support to OBJ importer #71033

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Add vertex color support to OBJ importer #71033

merged 2 commits into from
Jan 12, 2023

Conversation

scurest
Copy link
Contributor

@scurest scurest commented Jan 7, 2023

Adds support for vertex colors to OBJ imports with the v x y z r g b extension. The Blender exporter got support for this last June. See http://paulbourke.net/dataformats/obj/colour.html for more about the extension.

Fixes #70982.

Bugsquad edit: This closes godotengine/godot-proposals#5973.

  • No change for OBJ files without vertex colors.
  • v lines without a vertex color are treated as white.
  • The vertex colors are normally in sRGB space (see small discussion on the Blender patch). Do I need to do anything for that? I know there's the "Is sRGB" checkbox in the material properties.
  • Once a v line with a vertex color has been seen, vertex colors are turned on for all subsequent surfaces/os in the file. If you want this to be done with finer granularity (per surface) it will need to be reworked.

Please let me know if I need to do anything else.

@scurest scurest requested a review from a team as a code owner January 7, 2023 17:59
@akien-mga akien-mga added this to the 4.0 milestone Jan 7, 2023
@akien-mga akien-mga requested a review from a team January 7, 2023 18:22
@akien-mga akien-mga modified the milestones: 4.0, 4.x Jan 7, 2023
@clayjohn
Copy link
Member

clayjohn commented Jan 7, 2023

The vertex colors are normally in sRGB space (see small discussion on the Blender patch). Do I need to do anything for that? I know there's the "Is sRGB" checkbox in the material properties.

Yes the "is sRGB" checkbox should be checked off for materials associated with the mesh.

@scurest
Copy link
Contributor Author

scurest commented Jan 7, 2023

I pushed a fix for when there's a mix of v lines with and without colors. The colors vector should now always be either empty or the same size as vertices.

@scurest
Copy link
Contributor Author

scurest commented Jan 7, 2023

@clayjohn Can I do it for all MTL materials or does it need to be only if it had vertex colors? (And you mean checked on right?)

@scurest
Copy link
Contributor Author

scurest commented Jan 7, 2023

I pushed setting "Is sRGB" to on only if the material is used with vertex colors (so no change importing OBJ files without them). Like before, this may overapproximate when it needs to be turned on if there's a mix of v with and without color.

@fracteed
Copy link

fracteed commented Jan 8, 2023

@scurest thanks so much for working on this! Just wanted to check that the colors are not being altered by the importer to an sRGB space? I am using vertex colors as masks so only need the pure R, G or B channels as is. I assume that the sRGB conversion is just the standard shader option, so the original colors are unaltered if the shader option is not used?

@scurest
Copy link
Contributor Author

scurest commented Jan 8, 2023

@fracteed There's no color conversion, no. The only thing it does is set the StandardMaterial3D's Vertex Color "Is sRGB" on. If you access the color in a ShaderMaterial you will get the value as it is in the OBJ file plus or minus some small error (about 0.003 from testing it) that probably has something to do with how the attribute is stored or something (not sure exactly how that works).

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Test Plan:

https://developer.blender.org/D15159 has objs and mrgb objs test cases. Show (like a screenshot / video) that each feature obj load correct. (or not corrupt.)

I'm in approval of a separate mrgb pr or if you want to edit this one.

@scurest
Copy link
Contributor Author

scurest commented Jan 8, 2023

I'm not interested in doing the #MRGB extension.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

obj Game Project.zip

MRP using the blender files.

  1. Blender import requires vertex color to be on
  2. In obj mesh import mode it imports the vertex colors, but is missing a material to be generated on the ArrayMesh.
  3. obj Scene import is not keeping the colors without an ArrayMesh material
  4. vertex srgb on or off? I can try to match the blender (glb) importer, but not sure if there's a correct variation.

It does work. Can be finished in other future prs with this as a base.

@fracteed
Copy link

fracteed commented Jan 9, 2023

@fire upon reading that blender developer link, I was surprised that blender does do a linear to sRGB conversion on export. Does that mean that Godot would reconvert to linear upon import? According to that link,, blender does the conversion to linear upon import.

From what I have read gltf reads and writes vertex color in linear.

@clayjohn
Copy link
Member

clayjohn commented Jan 11, 2023

With respect to the sRGB conversion, upon reviewing the code I realized that, since we are parsing one vertex at a time anyway, we can just use Color.srgb_to_linear to convert to linear. However, that leaves people like Fracteed in the lurch if they save their obj files with linear space colors and need them returned in linear space. Having the conversion in the shader allows users the option to leave the colors in the color space of their choice.

@fire upon reading that blender developer link, I was surprised that blender does do a linear to sRGB conversion on export. Does that mean that Godot would reconvert to linear upon import? According to that link,, blender does the conversion to linear upon import.

Right now this PR converts to linear in the shader after reading the vertex color (as long as the supplied material is used). If you implement your own shader, you could read the vertex color directly which would return it in whatever color space the obj file was created with (e.g. if the obj file comes from blender it will still be srgb).

From what I have read gltf reads and writes vertex color in linear.

Kind of. GLTF just doesn't specify what color space to use so if its linear it stays linear.

@fire Can you clarify your last comment. Are your four points issues with this PR, or general usability issues with obj that impact the test project you linked?

@fire
Copy link
Member

fire commented Jan 12, 2023

I think the pr can be accepted.

When imported the obj will still be white. I was referring to that work that needed to be done.

@clayjohn
Copy link
Member

I think the pr can be accepted.

When imported the obj will still be white. I was referring to that work that needed to be done.

Right, because this doesn't provide a material if there is no MTL file referenced by the .obj. It makes sense to me that the obj importer would match the GLTF importer and create a material with FLAG_ALBEDO_FROM_VERTEX_COLOR. But I think you are right and that is something for a future PR.

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.

I tested this PR locally with the MRP from #70982 and it appears to work fine

@akien-mga akien-mga merged commit 14cca21 into godotengine:master Jan 12, 2023
@akien-mga akien-mga modified the milestones: 4.x, 4.0 Jan 12, 2023
@akien-mga
Copy link
Member

Thanks!

@scurest scurest deleted the obj-vertex-color branch January 12, 2023 23:58
@dioptryk
Copy link
Contributor

Is there a chance for this to be backported into 3.5 or 3.6? I really like importing TreeIt/SpeedTree trees from OBJ, since it's the easiest way to get a tree immediately as a Mesh (without the .tscn) and then MultiMesh it. Vertex color is very important for the tree models, because of how wind shaders work (and stuff like simulating seasons). Besides that, the PR is quite small and it looks like there should not be any conflicts? Thank you.

@Zireael07
Copy link
Contributor

Seconding (as it happens, for exactly the same reason, i.e. SpeedTree OBJ trees and vertex color wind shaders)

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 28, 2023
@Calinou
Copy link
Member

Calinou commented Apr 28, 2023

A PR to backport this to 3.x is welcome, but this won't be backported to 3.5.x as per the release policy (patch releases only include bug fixes).

This PR's commits do not cleanly apply to 3.x, and SurfaceTool in 3.x lacks the set_color() method that is used in this PR. This makes this PR nontrivial to backport.

@dioptryk
Copy link
Contributor

@Calinou should it be a completely new PR for 3.x? Is there any merging going on between 3.x and 4.x, or just cherry picks? I may try to do this as my first contribution.
Btw, it seems SurfaceTool in 3.x has add_color() instead, but the docs are identical, so perhaps it will work out of the box with just the method renamed :-)

@Calinou
Copy link
Member

Calinou commented Apr 29, 2023

@Calinou should it be a completely new PR for 3.x? Is there any merging going on between 3.x and 4.x, or just cherry picks? I may try to do this as my first contribution.

Yes, someone needs to open a new PR on 3.x. Feel free to give it a try 🙂

@dioptryk
Copy link
Contributor

dioptryk commented May 2, 2023

PR ready: #76671.
Please check the description, because I believe a fixed a bug that is present in master.

@Calinou Calinou removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 2, 2023
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.

OBJ importer does not read vertex colors Implement Blender-style vertex colour support for OBJ import
8 participants