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 optional UV2 logic for lightmapping to primitive shapes #67975

Merged

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Oct 28, 2022

This PR introduces optionally adding UV2 texture coordinates to primitive shapes including support for padding along the seams of the mesh. It will also set the lightmap size hint based on a texel size that can be configured in project settings:

image

The default values of this is 0.2.

The Primitive Meshes related to particles have not been changed.

This PR supersedes #49932

@BastiaanOlij BastiaanOlij added this to the 4.0 milestone Oct 28, 2022
@BastiaanOlij BastiaanOlij self-assigned this Oct 28, 2022
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner October 28, 2022 14:28
@jcostello
Copy link
Contributor

For what I can recall, emissive materials were working on baked lightmaps

@BastiaanOlij BastiaanOlij force-pushed the implement_uv2_on_primitives branch 2 times, most recently from c4a4b16 to 672415b Compare October 28, 2022 15:46
@Calinou
Copy link
Member

Calinou commented Oct 28, 2022

For what I can recall, emissive materials were working on baked lightmaps

In Godot 4, emissive materials require the mesh with the emissive material to have UV2, and have its GI mode set to Static. In Godot 3, meshes that don't have UV2 but had Use In Baked Light enabled could still contribute to lightmaps.

@BastiaanOlij
Copy link
Contributor Author

Before this gets merged, had some more thoughts about improving the UV algorithm.
The current calculation for UV1 is based on texture maps that have even sides and should remain as is for backwards compatibility.
But it would be easy to adjust the calculations for UV2 to have the UVs proportional to the actual size of the faces which is much better suited for lightmapping or texture painting. So I want to make that change before we merge.

Also on chat there was a suggestion to automatically assign the lightmap size hint to something appropriate, we need to decide what this calculation is (basically we need to design on a pixel per meter metric).

@Calinou
Copy link
Member

Calinou commented Oct 29, 2022

Also on chat there was a suggestion to automatically assign the lightmap size hint to something appropriate, we need to decide what this calculation is (basically we need to design on a pixel per meter metric).

I wonder how XAtlas determines this. It uses a Lightmap Texel Size property that is defined in the 3D scene's import options:

image

Since PrimitiveMesh doesn't have import options, we can add such a property to the PrimitiveMesh resource instead of making it an import option. Adjusting this property will cause the lightmap size hint to be updated.

@BastiaanOlij
Copy link
Contributor Author

@Calinou hmm, I wonder if it doesn't make more sense to make it a project setting, else we'll start getting questions of people wanting it to calculate backwards when they update the lightmap size and we'll need to do some sort of aspect ratio fixation.

@BastiaanOlij BastiaanOlij force-pushed the implement_uv2_on_primitives branch from 672415b to 3f2563f Compare October 30, 2022 12:52
@BastiaanOlij
Copy link
Contributor Author

Ok adjusted most UV2 calculations so they are based on actual sizes of the objects, just need to do BoxMesh and PrismMesh.

Also still need to add lightmap size hint calculation.

@Calinou the texelsize mentioned, if it's 0.2 does that mean that 1 pixel is 0.2 x 0.2 sized? So a square meter would have a 5x5 pixel grid for lighting data?

@Calinou
Copy link
Member

Calinou commented Oct 31, 2022

The project setting approach sounds fine to me. Just make sure the default matches what imported meshes use by default.

@Calinou the texelsize mentioned, if it's 0.2 does that mean that 1 pixel is 0.2 x 0.2 sized? So a square meter would have a 5x5 pixel grid for lighting data?

It means each texel square is 0.2×0.2 units. Larger values result in less detailed lightmapping, as the object will have fewer texels on the lightmap texture.

For reference, the default was increased from 0.1 to 0.2 in #58071 to speed up baking significantly. When baking indirect lighting only (which is the default setup in Light3D), it ends up looking very similar in practice.

@BastiaanOlij BastiaanOlij marked this pull request as draft November 1, 2022 12:29
@BastiaanOlij BastiaanOlij force-pushed the implement_uv2_on_primitives branch from 3f2563f to fd0fe85 Compare November 1, 2022 13:43
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Nov 1, 2022

I've changed this back to draft as I put the final things to it. It's been a bit of puzzling. I'm changing the UV2 calculation to be based on actual dimensions of the object and to update the lightmap size hint based on a new project setting.

So far I've adjusted:

  • BoxMesh
  • CylinderMesh
  • CapsuleMesh
  • PlaneMesh (/QuadMesh)
  • SphereMesh

image

@BastiaanOlij BastiaanOlij force-pushed the implement_uv2_on_primitives branch from fd0fe85 to f7ca122 Compare November 1, 2022 14:47
@BastiaanOlij BastiaanOlij marked this pull request as ready for review November 2, 2022 00:19
@BastiaanOlij BastiaanOlij force-pushed the implement_uv2_on_primitives branch from f7ca122 to 9a4e968 Compare November 2, 2022 00:19
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner November 2, 2022 00:19
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Nov 2, 2022

Finished prism and torus, I don't think the meshes for particles/trails require the UV2 treatment so I've left those to the default logic.

Prism and Box meshes could use some improvement to better pack the faces and prioritize sizes for smaller textures but I'll leave that for someone with more time on their hands :P

image

@BastiaanOlij BastiaanOlij force-pushed the implement_uv2_on_primitives branch 3 times, most recently from 8dcf0bc to 69e6add Compare November 7, 2022 08:41
@BastiaanOlij BastiaanOlij force-pushed the implement_uv2_on_primitives branch from 69e6add to 6c19950 Compare November 7, 2022 12:18
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.

Implementation looks fine to me, we need @reduz to approve the API additions though

scene/resources/primitive_meshes.h Outdated Show resolved Hide resolved
scene/resources/primitive_meshes.cpp Outdated Show resolved Hide resolved
@clayjohn clayjohn requested a review from reduz November 7, 2022 17:30
@BastiaanOlij BastiaanOlij force-pushed the implement_uv2_on_primitives branch from 6c19950 to 7658dc6 Compare November 13, 2022 08:28
@BastiaanOlij
Copy link
Contributor Author

Android build failure seems to be an issue with the CI, not the code, @akien-mga ?

@akien-mga akien-mga merged commit 038ee04 into godotengine:master Nov 14, 2022
@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.

5 participants