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 texel_scale property to LightmapGI #64908

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

marcinn
Copy link
Contributor

@marcinn marcinn commented Aug 25, 2022

This PR adds texel_scale property to LightmapGI.
Closes godotengine/godot-proposals#3893.


Side notes:

  1. This patch also adds BakeError:BAKE_ERROR_LIGHTMAP_TOO_SMALL together with a new warning message. I'm unable to generate editor/translations/editor.pot by using extract.py due to message duplicate error (editor.pot becomes empty). I have read the README files and contribution docs, but still don't know what exactly to do when adding new messages. As I'm not sure if the new BakeError (along with the new message) will be accepted, I decided to submit this potentially unfinished PR.

  2. The new BakeError should help in other lightbaking cases (errors or incorrect user input) that result in an incorrect image size (width or height equal to zero). In such cases the editor should display a warning instead of freezing.

@jcostello
Copy link
Contributor

This should affect #62987 once its ready. IDK which one will be merge first @techiepriyansh

@jcostello
Copy link
Contributor

BTW, works well

@marcinn
Copy link
Contributor Author

marcinn commented Aug 26, 2022

This should affect #62987 once its ready. IDK which one will be merge first @techiepriyansh

I'm not sure how related it is. If Texel Scale should affect visualisation of texel density, its value must be stored somewhere and must be accessible in runtime (as a copy in each baked mesh?). Currently it is stored only in LightmapGI node and affects lightmap_size only during offline baking.

@jcostello
Copy link
Contributor

IMO, it should affect the visualization of texel density in the debug view. Lets hear @techiepriyansh opinion

@Calinou
Copy link
Member

Calinou commented Aug 26, 2022

I'm unable to generate editor/translations/editor.pot by using extract.py due to message duplicate error (editor.pot becomes empty).

Don't worry – you don't need to generate those files yourself, as this is done periodically by Akien 🙂

@techiepriyansh
Copy link
Contributor

IMO, it should affect the visualization of texel density in the debug view. Lets hear @techiepriyansh opinion

Yes, technically, it should. But it won't be needed if the users only care about the relative texel density of different meshes in the visualization.

@jcostello
Copy link
Contributor

IMO, it should affect the visualization of texel density in the debug view. Lets hear @techiepriyansh opinion

Yes, technically, it should. But it won't be needed if the users only care about the relative texel density of different meshes in the visualization.

If it can be easily done I suggest to have it to have coherence with the lightmap size in the mesh attributes

@clayjohn
Copy link
Member

Please note, I have marked this as "for pr review", technically, we are in a feature freeze right now, so new features should not even be considered for merging. However, this is a relatively small change, so it may be able to sneak in.

@marcinn
Copy link
Contributor Author

marcinn commented Aug 28, 2022

@clayjohn Thanks.

@jcostello
Copy link
Contributor

@clayjohn I would appreciate it

@jcostello
Copy link
Contributor

@clayjohn please consider this to 4.0 🙏

@marcinn marcinn force-pushed the lightmap-gi-texel-scale branch 2 times, most recently from 6bd2977 to 6370cf3 Compare May 23, 2023 21:08
@akien-mga akien-mga requested a review from clayjohn May 23, 2023 21:23
@clayjohn clayjohn modified the milestones: 4.1, 4.2 Jun 9, 2023
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 4714e95), it works as expected.

Testing project: test_pr_64908.zip

I noticed a small issue in the editor however:

This message is spammed when you adjust the Texel Scale property to its minimum value in the LightmapGI inspector:

./scene/3d/lightmap_gi.cpp:1437 - Condition "p_multiplier < 0.01" is true.

I'm surprised this happens, considering the property hint disallows values lower than 0.01.

@akien-mga akien-mga changed the title Add texel_scale property to LightmapGI Add texel_scale property to LightmapGI Aug 11, 2023
@marcinn1
Copy link

marcinn1 commented Aug 11, 2023

I'm surprised this happens, considering the property hint disallows values lower than 0.01.

I bet this could be floating point precision issue, not related to my patch.

@Calinou
Copy link
Member

Calinou commented Aug 11, 2023

I bet this could be floating point precision issue, not related to my patch.

In this case, I'd suggest changing the check in the setter method to compare against 0.01 - CMP_EPSILON.

@marcinn1
Copy link

I bet this could be floating point precision issue, not related to my patch.

In this case, I'd suggest changing the check in the setter method to compare against 0.01 - CMP_EPSILON.

I've rebased and added this change. I see no spam messages at 0.01.

BTW: On my setup the longest part of generating lightmap with texel_scale=0.5 for this project is denoising.

@Calinou
Copy link
Member

Calinou commented Aug 12, 2023

BTW: On my setup the longest part of generating lightmap with texel_scale=0.5 for this project is denoising.

Denoising is performed on the CPU using OpenImageDenoise. Latest versions support GPU denoising, but we can't update to the latest version because of the new ISPC dependency which makes building OIDN from source very difficult.

Since lightmapping on the GPU is quite fast (especially with modern high-end GPUs), denoising will often take more time than baking lightmaps if you have a GPU that is fast compared to the CPU. This is the case even on top-end setups with "matching" CPU and GPU (such as i9-13900K + RTX 4090), as GPU processing speed is evolving much faster than CPU speed (even in multicore tasks).

Also, once this PR is merged, I could adapt #80518 to add a setting that automatically multiplies texel scale for preview bakes. You can choose to forcibly disable denoising in preview bakes too, although I've left it enabled by default because bakes look pretty bad with denoising disabled.

@marcinn1
Copy link

Denoising is performed on the CPU using OpenImageDenoise

Oh, it uses just 1 core/thread. There are threading limitations introduced for Godot, like this:

   // -- GODOT start --
   #endif
   numThreads = 1;
   // -- GODOT end --

@clayjohn
Copy link
Member

Denoising is performed on the CPU using OpenImageDenoise

Oh, it uses just 1 core/thread. There are threading limitations introduced for Godot, like this:

   // -- GODOT start --
   #endif
   numThreads = 1;
   // -- GODOT end --

See: #81659

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.

Seems fine to me! I haven't tested locally, but I trust the other reviewers

@AThousandShips
Copy link
Member

Needs a rebase

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 13, 2023
@akien-mga
Copy link
Member

@marcinn @marcinn1 Are you available to rebase this PR?

@marcinn1
Copy link

marcinn1 commented Jan 4, 2024

Yes, give me a moment please

@marcinn1
Copy link

marcinn1 commented Jan 4, 2024

@akien-mga done!

@akien-mga akien-mga requested a review from Calinou January 4, 2024 17:15
@akien-mga akien-mga merged commit 85e999d into godotengine:master Jan 5, 2024
15 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.

Add a global lightmap texel size multiplier in LightmapGI to speed up preview baking
9 participants