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

Improve Lightmapper Workflow #6544

Open
WickedInsignia opened this issue Mar 21, 2023 · 8 comments
Open

Improve Lightmapper Workflow #6544

WickedInsignia opened this issue Mar 21, 2023 · 8 comments

Comments

@WickedInsignia
Copy link

WickedInsignia commented Mar 21, 2023

Describe the project you are working on

Environments for graphics testing and personal projects.

Describe the problem or limitation you are having in your project

Godot's lightmapper workflow is right now a bit lackluster, with some arbitrary values and the need for more user fine-tuning to get the best possible results.
Although I don't recommend that Godot mimics Unity as a global standard, Unity has a very refined lightmapping workflow that could provide some great inspiration. Below I list some of the key differences in their working methods:

Unity:

The option to unwrap a mesh's UV2 is provided in the import dialogue just like Godot, however more options are provided to fine-tune the unwrap such as pack margin and Hard Angle. Hard Angle is especially important, since it allows the user to tell the unwrapper to place UV seams at harsh angular changes and avoid placing them in places that are meant to appear smooth. Having manual control of this is essential for some low-poly styles, especially when matching the seams to smoothing groups.

LightmapWorkflow_UnityExample01

Unlike Godot, the user defines the scene-wide Texel Density in the Lightmapping panel. This is measured in Texels Per Unit (texture pixels per meter). You may also provide scene-wide Padding here, as well as the Max Lightmap Size to aim for. Unity keeps to these size limitations more reliably than Godot, with Godot sometimes opting for 8x 512sq maps when the defined max size is much larger.

LightmapWorkflow_UnityExample02

Unity also provides a more cohesive view of the final lightmaps, allowing for large-scale viewing to check for lighting errors and highlighting where the selected mesh's UVs are located.
The option to define each mesh's scale in the lightmap is also available as a decimal percentage, rather than a multiplier like Godot. This allows for far more fine-tuned sizing, and extremely small sizes such as 1% (0.01).

LightmapWorkflow_UnityExample03

Godot:

Godot provides the option to unwrap a mesh's UV2 for lightmapping on import, just like Unity. However it also asks that Texel Density is provided in this same dialogue. This is provided as an arbitrary number, and requires the user to reduce the already very small starting value to get finer coverage. If the user wants to change the Texel Density for the whole scene, they must change it per mesh into a completely arbitrary value. Too much guesswork and effort involved.

LightmapWorkflow_GodotExample01

Max Texture Size is provided in the Lighhtmap GI settings, but no other scene-wide settings are offered. Godot also does not abide by Max Texture Size reliably, regularly making maps much smaller than necessary and not packing them efficiently into larger sizes where possible.

LightmapWorkflow_GodotExample02

A basic view of the lightmaps is also available, but the information is nearly functionally useless when the maps cannot be inspected beyond a thumbnail with no per-mesh UV information.

LightmapWorkflow_GodotExample03

Describe the feature / enhancement and how it helps to overcome the problem or limitation

  • Use a non-arbitrary value for defining Texel Density. I recommend Texture Pixels Per Meter as Unity does, since Godot's units are metric.
  • Make Texel Density a scene-wide option in the LightmapGI properties.
  • Add an option for Lightmap Padding in the LightmapGI properties, defined in texels.
  • Make Godot utilize Max Lightmap Texture Size more reliably (if this is set to 4096, Godot should not be generating 12x1024sq maps for example).
  • Add more lightmap UV options to mesh importing. Hard Angle at the very least would be extremely useful.
  • Change per-mesh lightmap sizing to a custom decimal value (0.01 = 1%) instead of only 4 different multipliers.
  • Allow for lightmaps to be inspected at a larger size. Instead of a scene-wide mesh highlighting like Unity (which isn't possible since most meshes are actually instanced scenes, with LightmapGI only present in the main scene) add a subpanel to each MeshInstance that shows the lightmap texture it belongs to, with the UVs for that particular MeshInstance highlighted.
  • Allow for both the MeshInstance and LightmapGI lightmap texture displays to be opened up in a floating window or expanded for inspection.
  • Add a viewport option to check lightmap texel density via a checkerboard texture, as shown below:

GIVis9

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

See above.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, this is core Godot functionality.

Is there a reason why this should be core and not an add-on in the asset library?

The lightmapping workflow is a core component of Godot's GI solutions.

@Calinou
Copy link
Member

Calinou commented Mar 21, 2023

  • Use a non-arbitrary value for defining Texel Density. I recommend Texture Pixels Per Meter as Unity does, since Godot's units are metric.

I think this makes sense, but this is a breaking change. If this is implemented, conversion code needs to be added for scene resources that have the old property.

  • Make Texel Density a scene-wide option in the LightmapGI properties.

This is already being tracked in #3893.

  • Add an option for Lightmap Padding in the LightmapGI properties, defined in texels.

LightmapGI padding is currently hardcoded to 2×2 pixels (1 pixel on each edge for each atlas slice). These lines are defining this padding:

Out of curiosity, why does this need an option? Lightmaps are sampled without mipmaps (they don't even have mipmaps generated), so 2×2 pixels should suffice to prevent lightmap bleeding.

  • Make Godot utilize Max Lightmap Texture Size more reliably (if this is set to 4096, Godot should not be generating 12x1024sq maps for example).

LightmapGI size is determined by this function: https://github.com/godotengine/godot/blob/a008a06fcb3a65d5fc3694d3cb215bfb60397fe6/modules/lightmapper_rd/lightmapper_rd.cpp#L162

I don't know how it could be made to follow the defined maximum texture size more strictly.

  • Add a viewport option to check lightmap texel density via a checkerboard texture, as shown below:

This is being tracked in #3213.

  • Add more lightmap UV options to mesh importing. Hard Angle at the very least would be extremely useful.

Godot uses xatlas for generating UV2; I don't know if xatlas exposes this.

  • Change per-mesh lightmap sizing to a custom decimal value (0.01 = 1%) instead of only 4 different multipliers.

This is being tracked in #3233.

@WickedInsignia
Copy link
Author

Thanks for all that! Just to answer those:

  • Great to see all the proposals that are being tracked already!
  • The arbitrary value is a real pain-point at the moment. As an artist, working with proper texel density values gives predictable results, and the current default arbitrary value (0.2) yields a very low texel density. Most times you're going down into 0.1 or 0.05 rather than up. If this can be changed, it should be. Even if that default value is simply adjusted to a higher value with the same density result (such as 10), it would improve things greatly alongside being a scene-wide value.
  • I'm having a LOT of leaking issues with Godot's lightmapper, and it would have been easier to check for errors with custom padding values. Unity has issues at 2 texels padding with a considerably more capable mapper, so it makes sense that Godot would too. Doesn't have anything to do with mipmapping since denoising, smoothing and bicubic interpolation could introduce bleeding depending on geometry. Will inspect these leaks more closely with a custom UV setup though.
  • I'm not sure how to make it follow defined maximum texture size more closely either, but if I'm defining a max size of 2048 and it's giving me 5 individual 1024x512 maps (as it has done), something's up. Might be worth bug reporting instead.

@Calinou
Copy link
Member

Calinou commented Mar 21, 2023

  • The arbitrary value is a real pain-point at the moment. As an artist, working with proper texel density values gives predictable results, and the current default arbitrary value (0.2) yields a very low texel density. Most times you're going down into 0.1 or 0.05 rather than up. If this can be changed, it should be. Even if that default value is simply adjusted to a higher value with the same density result (such as 10), it would improve things greatly alongside being a scene-wide value.

The default lightmap texel size was changed from 0.1 to 0.2 in Godot 4.0, as the prior value led to much larger lightmap sizes and longer bake times. If you bake indirect light only (which is the default configuration), you don't need lightmaps to be very dense as indirect lighting is low-frequency data.

#3893 will let you use a multiplier to adjust this density globally more easily. The default is intended to provide quick bakes and relatively small file sizes, as many people use lightmaps to target mobile devices or HTML5.

Also note that Bicubic sampling hasn't been reimplemented in 4.x yet, but once it's readded, lightmaps will look as good as they did in 3.x.

  • I'm having a LOT of leaking issues with Godot's lightmapper, and it would have been easier to check for errors with custom padding values.

This is likely related to godotengine/godot#69126. It might be worth checking if increasing the hardcoded padding to 16 pixels improves the situation, but I think the issue is somewhere else.

Edit: I've checked this locally and can confirm increasing padding to 16 pixels does not fix the issue with environment light leaking. (The lightmap does show that padding between meshes is increased, but the leaking issue occurs even with a single mesh in the scene.)

@WickedInsignia
Copy link
Author

What I mean is changing that value to a more digestible number by default, while maintaining the exact same resolution as the old default.
So if we make that default 20 instead, it gives the same texel density as 0.2 in the old setting. 10 gives the same density as 0.1, 5 the same as 0.05, etc.
It just makes the number easier to handle, rather than always working with decimals. This should be relatively simple to implement if the new value is just a multiplication of the old one (such as 0.2x100)
The current situation isn't great for lighting that's entirely baked. There's absolutely no loss to increasing that value to something more general-purpose for either case since there are very few use-cases that require anything higher than 1.

Also I think my leaking issue is different to that since it's related to direct and bounce light. Will investigate and make a report if appropriate.

@Calinou
Copy link
Member

Calinou commented Mar 21, 2023

What I mean is changing that value to a more digestible number by default, while maintaining the exact same resolution as the old default.
So if we make that default 20 instead, it gives the same texel density as 0.2 in the old setting. 10 gives the same density as 0.1, 5 the same as 0.05, etc.

This change would also break compatibility. If we make a compatibility-breaking change, I'd prefer going the texels-per-unit route which is more commonly used in other engines nowadays.

It should be possible to create a compatibility handler by renaming the import option to something else, then checking if the old import option still exists and apply its value to the new import option as 1.0 / old_value.

@WickedInsignia
Copy link
Author

I'd agree, definitely prefer the texels-per-unit approach. This was just offered in the instance it wouldn't break compatibility.

@jcostello
Copy link

Maybe we can merge both proposals (this one and #7900)

@WickedInsignia
Copy link
Author

Maybe we can merge both proposals (this one and #7900)

That proposal almost entirely addresses baking/saving, this one is more of a mixed bag of texel editing and preview options. Probably best to keep them separate but roll them into "4.x milestones" as reference so they don't get lost.
Definitely worth bumping this proposal while there's some movement on the lightmapper front lately too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants