godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.08k stars 69 forks source link

Add normal generation options to the OBJ mesh importer #220

Open RobertBColton opened 4 years ago

RobertBColton commented 4 years ago

Describe the project you are working on: Subdivision surfaces which require accurate vertex and face adjacency lookup with MeshDataTool or custom half-edge data structure. The forced normal generation for objs without normals produces flat shaded faces leading to inaccurate adjacency lookup in MeshDataTool. Describe the problem or limitation you are having in your project: I would like to disable normal generation for some of my imported models so I could use the builtin obj reader. Describe how this feature / enhancement will help you overcome this problem or limitation: I can dump my custom simple obj reader that uses SurfaceTool in favor of the builtin obj mesh importer. Show a mock up screenshots/video or a flow diagram explaining how your proposal will work: Generate Normals Mockup Describe implementation detail for your proposal (in code), if possible: Well, I may just jump into my first pull request to Godot and do this myself. I can already see where it needs done in the obj resource importer.

First the setting needs added: https://github.com/godotengine/godot/blob/dec10dd776fca2994277faa3a97b13e70317f784/editor/import/resource_importer_obj.cpp#L492 Then the setting needs to be actually checked: https://github.com/godotengine/godot/blob/dec10dd776fca2994277faa3a97b13e70317f784/editor/import/resource_importer_obj.cpp#L337 If this enhancement will not be used often, can it be worked around with a few lines of script?: This problem can not easily be worked around, no. It would be a total waste to write a script that dumps the normals and reloads the model for no reason. It's also not always sensible to write your own obj reader. Is there a reason why this should be core and not an add-on in the asset library?: I see this feature as universally necessary for practically all types of 3D games. First, not all 3D games actually use lighting, in which case the normal computation is a waste both in terms of editor processing time and GPU upload time. Even 3D games which do use lighting don't always need normals for every model. Finally, I'd feel remiss if I did not mention that Blender, which heavily inspires Godot, also has this on its export options.

hammeron-art commented 4 years ago

I see this feature as universally necessary for practically all types of 3D games. First, not all 3D games actually use lighting, in which case the normal computation is a waste both in terms of editor processing time and GPU upload time. Even 3D games which do use lighting don't always need normals for every model. Finally, I'd feel remiss if I did not mention that Blender, which heavily inspires Godot, also has this on its export options.

I think the SpatialMaterial requires normal data to work (as well as many other things like tangents, uvs, vertex_colors, etc). It's a common practice because not doing so is more work for almost no advantage in most cases. If you don't write normals to the mesh, the engine will still upload normal data as an array of zero vectors. Godot is even more forgiving than Blender, in Godot a mesh without normals renders fines because the engine takes care of sending the correct data to the shader while in Blender all meshes are required to have predefined normals for correct rendering and it always calculates them on import or edit.

I agree with the proposal to add the option to disable normal generation but if your goal is performance, you need to create a custom material that doesn't use normals or any other vertex data you don't need. I wouldn't recommend it though unless you are dealing with many high-density meshes.

RobertBColton commented 4 years ago

@hammeron-art as an addendum or alternative, let me propose that there be a setting to also control whether generated normals are smooth. Smooth normals are not a problem for adjacency lookup. I've actually completed my subdivision surfaces, and I use smooth normals just fine.

If just this option was provided, I wouldn't have needed my custom obj reader for the initial part of what I was doing. I also think it seems odd because adding a smooth group is so easy with SurfaceTool, that I can't believe it's not offered in the importer which uses the same interface.

https://github.com/RobertBColton/SubdivisionSurfaces#subdivision-surfaces

Smooth Subdivided Mesh Godot

Calinou commented 4 years ago

@RobertBColton If I understand correctly, does this mean the "Generate Normals" import setting should be an enum with the values "Disabled", "Flat" and "Smooth"?

RobertBColton commented 4 years ago

Ideally, yes. Otherwise a single checkbox for one or the other, if everybody is really that opposed to my initial idea of allowing to disable the normal generation completely.

And also, that's assuming there is no other purpose of adding a smoothing group if a mesh will not contain normals.

hammeron-art commented 4 years ago

@Calinou Smooth group is a setting for the calculation of normals and is only relevant when the .obj file doesn't have normal data. The .obj specifies if smoothing is on or off for a specific vertex group. The same object can have multiple on or off smooth vertex groups.

When an "Use Smooth Groups" option is on in the importer and there are no normals data in the file, the application is supposed to generate normals based on these smooth groups.

When there is normal data written in the .obj, smooth groups are ignored because the application will read the normals from the file instead of generating them.

In short, .obj is a painfull format! I think this exists only to save space with normal data back in the days.

Finally, the main issue is that normal data isn't required for unlit shading or when you will customize the mesh by code but Godot always calculates normals when there is no normal data in the file. An option to disable normal generating could be useful.

Calinou commented 4 years ago

@hammeron-art So, what should we do in the end? My original suggestion was to add a "Generate Normals" import setting that can have the values "Disabled", "Flat" or "Smooth", but maybe it should be named "Generate Missing Normals" instead as it shouldn't impact OBJ files that already contain normals.

RobertBColton commented 4 years ago

I thought your idea was fine, would "Disabled" not mean use the normals already in the OBJ? Perhaps you misunderstood me, in my case, my OBJ models didn't have any normals. That's exactly what I wanted, to just load my model as usual the way that it was.

Calinou commented 4 years ago

@RobertBColton In my proposal, "Disabled" would disable normal generation entirely, even if normals are missing.

RobertBColton commented 4 years ago

Ok, in my opinion, that would be perfect.

RobertBColton commented 4 years ago

For reference, here is the shading options in Blender's import dialog: Blender Import Shading Options