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 Tony McMapface as a tonemapping mode #97095

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jirisvd
Copy link
Contributor

@jirisvd jirisvd commented Sep 16, 2024

This PR adds Tony McMapface as a tonemapping mode based on godotengine/godot-proposals#7263. All tonemapping options currently available in Godot have various limitations described by @Calinou in the proposal. For example, Linear, Reinhard and Filmic suffer from oversaturated bright lights, whereas ACES steers bright blues towards purple and can significantly darken scenes. Regardless of which option users choose, extensive tweaking is needed to achieve good results.

Tony McMapface is an artist-frendly tonemapper that tries to stay close to the input color and doesn't increase contrast or saturation, leading to a pleasant neutral look that can work as a great easy-to-use alternative to the options currently available. Take a look at the following screenshot from a simple scene with colorful spheres illuminated by the sun:

tony

Tony manages to tone down the saturation of the spheres on the left without harming the rest of the image, resulting in a balanced look. Compare that to Reinhard, Filmic or ACES (all screenshots are configured with whitepoint 6.0):

Tonemap tony_mc_mapface.zip Bistro demo scene
Reinhard reinhard_6 bistro_reinhard_6
Filmic filmic_6 bistro_filmic_6
ACES aces_6 bistro_aces_6
Tony McMapface tony bistro_tony

The ability to preserve color hues can be seen in the following two test scenes made by @allenwp. Notice how Tony preserves green and blue, while Reinhard and Filmic shift green towards yellow and ACES shifts blue towards purple:

Tonemap tonemapping_comparison.zip tonemapping_test_bars.zip
Reinhard hue_reinhard_6 bars_reinhard_6
Filmic hue_filmic_6 bars_filmic_6
ACES hue_aces_6 bars_aces_6
Tony McMapface hue_tony bars_tony

For Tony McMapface to work correctly, it needs to be integrated into the engine, it wouldn't be possible to integrate it as a custom LUT that the user provides, see @Calinou's investigation and the following discussion in the proposal.

Tony's shader samples its own custom LUT as a final step, so this LUT needs to be embedded into the engine. It's a 48x48x48 3d texture in RGBE5999 format and this PR embeds it DEFLATE compressed, which ultimately results in a 306 KB binary size increase. To alleviate this, the PR also adds a disable_tony_mc_mapface=yes build option that disables it and removes the embedded LUT, so that mobile/web users can minimize binary size just like they can with other parts of the engine.

The embedded header file with the LUT's binary data is dynamically generated at build time by a build script. The tonemapping mode has been added to all three rendering methods.

Thanks to @allenwp for advice, testing and reviewing earlier versions.

@Calinou
Copy link
Member

Calinou commented Sep 16, 2024

Thanks for opening a pull request, and going through the effort of implementing this 🙂

which ultimately results in a 306 KB binary size increase

This is a lot for mobile/web platforms, which makes me reconsider whether this should be in core. Not every user targeting mobile/web platform compiles their own export templates, as the process can be time-consuming (and daunting for users not familiar with the command line).

It's made worse by the fact that this 306 KB is for a texture that is already compressed, so the usual compression techniques (Android APK, gzip/Brotli for web export) won't reduce its size. The web export's loading speed is directly determined by the size of the WebAssembly blob.

We could also disable the feature by default in web builds, but this will result in projects looking different once exported to the web platform if they use Tony McMapface. This can be difficult for users to troubleshoot, even if a warning is printed to the console in this case (it's not exactly obvious when this happens, as the HTML shell currently does not have a visible notification when DevTools isn't open).

Alternatively, we could consider using a smaller LUT texture as an approximation of the tonemapper. This could make the size increase much more acceptable – ideally below 100 KB. Would a 24×24×24 texture look bad (or perhaps 24×48×24 to increase precision on the green channel, which is the most visible to the human eye1)? If this is a viable approach, it might be worth trying multiple sizes and comparing them against a reference image using dssim to find a sweet spot between accuracy and binary size.

For Tony McMapface to work correctly, it needs to be integrated into the engine, it wouldn't be possible to integrate it as a custom LUT that the user provides, see @Calinou's investigation and the following discussion godotengine/godot-proposals#7263.

Allowing users to use their own tonemapper textures would allow implementing this feature with a much smaller binary size increase (only a few KB at most), so that you only pay for the features you actually use in a project. This should be technically feasible, it's just that the current color correction feature is not meant to be used as a custom tonemapper (since it runs after tonemapping).

It should be straightforward to make the PR use a new user-supplied texture as an Environment property, similar to the existing color correction adjustments feature. The difference is that adjustments run after tonemapping, while we want the custom tonemapper texture to run before tonemapping. I suggest making it enabled using a new Custom tonemapper option at the end of the existing enum.

We can provide a set of recommended textures in the documentation, or even in the asset library. This way, we could even expand this to other up-and-coming tonemappers such as Khronos' PBR Neutral (if a LUT version of it is ever made available). As I understand it, in theory, any tonemapper can be represented as a LUT.

I still think AgX should be core though, considering a lot of apps are starting to standardize around it (and abandoning ACES in the process). While Tony McMapface looks good on its own, it isn't as standardized in comparison.

Footnotes

  1. On second thought, this may not be a great idea as it can result in quantization issues biasing the color towards magenta, like it happens with DXT1 compression. Using a perfect cube size may still be preferable.

@allenwp
Copy link
Contributor

allenwp commented Sep 17, 2024

Allowing users to use their own tonemapper textures would allow implementing this feature with a much smaller binary size increase (only a few KB at most), so that you only pay for the features you actually use in a project. This should be technically feasible, it's just that the current color correction feature is not meant to be used as a custom tonemapper (since it runs after tonemapping).

It should be straightforward to make the PR use a new user-supplied texture as an Environment property, similar to the existing color correction adjustments feature. The difference is that adjustments run after tonemapping, while we want the custom tonemapper texture to run before tonemapping. I suggest making it enabled using a new Custom tonemapper option at the end of the existing enum.

We can provide a set of recommended textures in the documentation, or even in the asset library. This way, we could even expand this to other up-and-coming tonemappers such as Khronos' PBR Neutral (if a LUT version of it is ever made available). As I understand it, in theory, any tonemapper can be represented as a LUT.

I don't quite understand how this idea of user-provided LUT would work. Here is my understanding of this:

  • When decomposed, Tony McMapface is two parts:
    1. A basic Reinhard tone mapper (color = color / (1 + color)). This is mathematically equivalent to the corrected Reinhard tone mapper if the White parameter was set to infinity.
    2. A specially designed floating point color correction LUT which further transforms the [0.0, 1.0] Reinhard floating point output into the final color value. This is where the magic happens, as this corrects the hue shift that happens in the Reinhard tone mapping.

If the user was able to swap the LUT in this scenario, they would only be able to provide LUTs that are designed to be applied after the Reinhard color = color / (1 + color) tone mapping has been applied. This is where I become confused because I don't know that this sort of custom tone mapping LUT would be expected by most users.

If custom tonemapping LUTs were a feature, I would expect them to behave a bit more like this, where the input is either [0.0, 2.0], [0.0, 3.0], [0.0, 4.0], etc. based on the needs of the project.

That said, I might be wrong! Maybe it makes sense for the user to supply a tone mapping LUT that is intended to be applied after color = color / (1 + color)! This would allow for Tony McMapface and also whatever the user is able to generate that works with the post-Reinhard [0.0, 1.0] range. I don't know how I would create a LUT like this, though, personally, so I'd like to hear of some examples.

Of course, the other option would be for the user to supply both a custom LUT texture and a custom tone mapping shader that applies the LUT. This would possibly be a more complex solution to custom tone mapping LUTs, but would allow for Tony McMapface and any other type of LUT that the user may want to use for tone mapping, including one with custom colour grading built-in as described in the previous link. This approach was discussed and decided against in the proposal.

@JoNax97
Copy link
Contributor

JoNax97 commented Sep 17, 2024

You said the LUT Is generated dynamically during the build process.

Would it be feasible to generate it on the fly in Godot itself and store it in-memory? This would only need to be done once at startup, and only for projects using the tonemapper.

@jirisvd
Copy link
Contributor Author

jirisvd commented Sep 17, 2024

I think @allenwp's comment gets at the heart of the problem. It doesn't seem to be enough to just allow the user to provide a custom tonemapping LUT because different LUTs can be expected to require different accompanying shader code. Tony McMapface needs Reinhard tonemapping to be performed first, whereas the LUT from the Naughty Dog presentation expects HDR [0.0-2.0] input. Other LUTs could have different requirements.

So it would seem that for tonemappers like this to be supported as a custom option with the LUT supplied by the user, Godot would have to make it possible to provide both the LUT and the custom shader code that samples it. Would it be possible to extend the compositor system to allow users to write their own tonemapping shaders? Now that I'm looking at godotengine/godot-proposals#7916, it seems that this has already been proposed. This would make it possible for users interested in using Tony McMapface or any other custom tonemapping LUT in Godot to write their own compositor effect and provide both the shader and the LUT themselves without the need to fork the engine, which is the only way to do it currently.

You said the LUT Is generated dynamically during the build process.

Would it be feasible to generate it on the fly in Godot itself and store it in-memory? This would only need to be done once at startup, and only for projects using the tonemapper.

@JoNax97 The LUT itself isn't generated at build time, just the tony_mc_mapface_lut.gen.h header file. The build script copies the raw pixel data of the LUT from the .dds file in thirdparty/tony-mc-mapface, compresses it and writes it down as a uint8_t byte array into the header file so that it can be used in the engine at runtime. But the actual generation code for the LUT itself isn't public at the moment and is likely computationally expensive.

So there are only two ways to get the LUT into the engine, either by embedding it like in the PR or by the user providing it themselves in their project as proposed by @Calinou. We can't actually generate the LUT ourselves.

@allenwp
Copy link
Contributor

allenwp commented Sep 17, 2024

I feel like there are some other ways to get the LUT into the project, but it would definitely be more involved…

The export process could add in the LUT if TMMF was enabled in the project settings. If TMMF was disabled in the project settings, it would not be added during export. I don’t like this idea because I don’t think there is any sort of precedence for this sort of behaviour.

Or maybe enabling TMMF will trigger the editor to add the LUT texture to the user’s project. This would mean the LUT would exist in the project’s resources alongside other textures. Disabling TMMF could either delete this file or ask the user if they want to delete it. The link to this LUT texture could be presented in the environment node’s properties, but this might cause confusion that you could replace it with something else… which is technically true, but it’s unlikely that you would do such a thing.

Maybe there is another way that isn’t as janky and out-of-place for Godot’s way of doing things…

@Calinou
Copy link
Member

Calinou commented Sep 17, 2024

The export process could add in the LUT if TMMF was enabled in the project settings. If TMMF was disabled in the project settings, it would not be added during export. I don’t like this idea because I don’t think there is any sort of precedence for this sort of behaviour.

A similar approach is used for optional ICU data (for hyphenation and the like), but it's much larger data (usually 10+ MB). I feel adding dedicated support for a 300 KB file is a bit overkill in comparison.

@Mickeon

This comment was marked as resolved.

@jirisvd
Copy link
Contributor Author

jirisvd commented Sep 17, 2024

@Calinou How about the second option mentioned? The LUT could only be embedded in the editor (using #ifdef TOOLS_ENABLED) and when selected by the user, it would be saved as a file in res://. This is definitely a bit awkward, it would however solve the problem and would be quite simple to implement. I assume that a 306 KB size increase only on the editor side wouldn't be that bad, it's the exported game where it matters the most and that would be solved, only exported games that actually use the LUT would have it.

@Calinou
Copy link
Member

Calinou commented Sep 17, 2024

How about the second option mentioned?

This sounds reasonable, but at the same time, I wonder if we can just resize the LUT texture to be smaller as I mentioned in my comment above. If we can embed a smaller texture (ideally < 100 KB), this would avoid a lot of added complexity.

@allenwp
Copy link
Contributor

allenwp commented Sep 17, 2024

How about the second option mentioned?

This sounds reasonable, but at the same time, I wonder if we can just resize the LUT texture to be smaller as I mentioned in my comment above. This would avoid a lot of added complexity.

I think this question would be best answered by the original author of the LUT… they might be available to give an opinion? The current closed source nature of the LUT means that testing of a different resized LUT would need to be very thorough.

@jirisvd
Copy link
Contributor Author

jirisvd commented Sep 17, 2024

This sounds reasonable, but at the same time, I wonder if we can just resize the LUT texture to be smaller as I mentioned in my comment above. This would avoid a lot of added complexity.

I initially worked with an RGB8 48x48x48 version of the LUT instead of RGBE5999, which was smaller in size but less precise. It looked decent, but the binary was still quite large and the difference was visible when comparing screenshots side-by-side. I can experiment with making it much smaller, but I can only assume that the difference in quality would grow as the texture shrinks. Overall, I'm not sure how I'd feel about Godot offering Tony McMapface natively but with significantly lower quality than originally intended. I'm sure the author put a lot of work into fine-tuning the colors to be just right and a lot of that would be lost, at which point I would prefer some other solution, even if it would be slightly more awkward (like the res:// solution) or delayed (like waiting for custom compositor effect tonemapping).

@adamscott
Copy link
Member

For the web, we could make the .dds file available in the export files themselves. The engine could load the texture separately, or when needed. But it introduces a lot of moving pieces for essentially an optional feature.

@clayjohn
Copy link
Member

How about the second option mentioned?

This sounds reasonable, but at the same time, I wonder if we can just resize the LUT texture to be smaller as I mentioned in my comment above. If we can embed a smaller texture (ideally < 100 KB), this would avoid a lot of added complexity.

The original author responded to a question on their repo and basically said that you can't go smaller without sacrificing quality significantly.

@JoNax97
Copy link
Contributor

JoNax97 commented Sep 17, 2024

What do you think about shipping the LUT for now to get the feature to users, and look into alternative loading or size reduction in a follow-up?

@allenwp
Copy link
Contributor

allenwp commented Sep 18, 2024

Edit: Sorry, I made a number of edits to this comment 😅

The export process could add in the LUT if TMMF was enabled in the project settings. If TMMF was disabled in the project settings, it would not be added during export. I don’t like this idea because I don’t think there is any sort of precedence for this sort of behaviour.

A similar approach is used for optional ICU data (for hyphenation and the like), but it's much larger data (usually 10+ MB). I feel adding dedicated support for a 300 KB file is a bit overkill in comparison.

I haven’t looked into the ICU code, but if it’s a choice between this sort of solution or not including Tony McMapface in the engine, I think this becomes less of an “overkill” solution.

From my experience shipping mobile games, having an extra 300 kB of incompressible data added to game package is a big deal and I would very much like to have a switch to turn that off without compiling a custom build of the engine. As more features are added to the engine, I suspect many mobile developers would appreciate having more levers to reduce the size of their game, so maybe this system could be built out a bit more so it doesn’t feel so overkill?

From the perspective of discoverability and usability, it would obviously be ideal if it was fully automatic and behind the scenes, but I don't see that being possible because you can switch tonemappers at runtime.

To be true to its original design and purpose, I believe Tony McMapface needs to be discoverable as a first-class tone mapper in the Environment’s list. This is because it’s a tone mapper that is meant to be used by those who don’t want to fuss around with a bunch of settings (not even a white parameter), don’t want any film-like effects or curves, don’t want to write their own tone mapper, and want to have something simple and neutral that doesn’t have any surprises like hue shifts in brighter values.

All this said, if the PR is not integrated as-is, I think it would be best for Tony McMapface to be an advanced project setting that is turned on by default. When the project setting is turned off, the LUT texture would not be included in the exported project and Tony McMapface would be hidden in the Environment tonemapper list. I don't know exactly what would happen if you tried to switch to Tony McMapface at runtime or if you loaded a scene that used it when the feature is turned off in the project settings, but maybe there could be some solution for this? Or maybe it would be a "disable at your own risk", given it's an advanced project setting?

@allenwp
Copy link
Contributor

allenwp commented Sep 19, 2024

From my experience shipping mobile games, having an extra 300 kB of incompressible data added to game package is a big deal and I would very much like to have a switch to turn that off without compiling a custom build of the engine.

I should clarify that I have experience shipping mobile games with Unity, but not with Godot. After looking into this more, I suspect that most users who are exporting for mobile or web who want a smaller app package size are likely already building a custom export template to reduce size.

For this reason, I am of the opinion that this PR should be merged as-is. When better systems are in place to reduce package size through project or export settings, the Tony McMapface LUT should be adapted to work with these systems.

In addition, I think that a related docs PR should be merged to provide explanation of how to remove the Tony McMapface LUT texture from builds. This could likely just be a small modification to the existing docs page. The Godot build options generator should also be updated to include this new disable_tony_mc_mapface build option.

The Project -> Tools -> Edit Compilation Configuration Profile could also be updated to have disable_tony_mc_mapface added to the list... But this tool seems to be mostly for disabling classes and nodes, rather than minor features like this.

@allenwp
Copy link
Contributor

allenwp commented Sep 22, 2024

I did a very quick check of disable_tony_mc_mapface:

Using SCons-detected MSVC version 14.3, arch x86_64 on Windows.

6681f25
scons platform=windows target=template_release arch=x86_64: 61,828,096 bytes

882f6bd
scons platform=windows target=template_release arch=x86_64 disable_tony_mc_mapface=yes: 61,832,192 bytes
scons platform=windows target=template_release arch=x86_64: 62,146,560 bytes

314,368 bytes saved with disable_tony_mc_mapface=yes for this specific configuration.

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, it works as expected in all rendering methods.

Linear Reinhard Filmic
Screenshot_20240924_163806 png webp Screenshot_20240924_163810 png webp Screenshot_20240924_163815 png webp
ACES Tony McMapface
Screenshot_20240924_163820 png webp Screenshot_20240924_163824 png webp

SConstruct Outdated Show resolved Hide resolved
@akien-mga akien-mga requested review from clayjohn and a team September 24, 2024 16:20
@akien-mga
Copy link
Member

I haven't reviewed in depth yet, but I just want to raise a flag quickly that adding a build system option for this seems very highly specific to me. I'm not saying it's a bad idea, and thus not asking to change it for now, but I want to review a bit more thoroughly to confirm whether it's good or whether we should do it differently.

@allenwp
Copy link
Contributor

allenwp commented Sep 24, 2024

Posting here as well for posterity: I had some fun whipping up some more tools for comparing tonemaps, including Tony McMapface. I made these scenes as an extension of @Calinou's demo project. You can download the project here if you'd like to fiddle with them yourself; I find them most useful as interactive tools.

All images use Forward+ with whitepoint 6.0 unless otherwise mentioned.

Tonemap Gradients (Linear Scale, No Debanding) Hues (Exponential Scale, No Debanding)
Linear Gradients-Linear Hues-Linear
Reinhard (5efa6ba onward) Gradients-Corrected-Reinhard Hues-Corrected-Reinhard
Filmic Gradients-Filmic Hues-Filmic
ACES Gradients-ACES Hues-ACES
Tony McMapface (this PR) Gradients-TMMF Hues-TMMF
AgX PR #87260 Gradients-AgX Hues-AgX
AgX PR #87260 (whitepoint = 16.0) Gradients-AgX-16 Hues-AgX-16
AgX Punchy PR #87260 Gradients-AgX-Punchy Hues-AgX-Punchy

@clayjohn
Copy link
Member

I haven't reviewed in depth yet, but I just want to raise a flag quickly that adding a build system option for this seems very highly specific to me. I'm not saying it's a bad idea, and thus not asking to change it for now, but I want to review a bit more thoroughly to confirm whether it's good or whether we should do it differently.

@akien-mga The setting is added out of a concern for adding 300 kb to the engine binary (including export binary) that can't be effectively compressed.

Allen explains it well in his comment #97095 (comment)

I don't love the idea of adding 300 kb to the engine for a single small feature. But its quite clear that users are very excited at the prospect of having a new modern tonemapping operator in core and this is the only realistic way to add it that fits within Godot's design principles.

So overall, I would love your input on adding a new build system flag and what you think about adding more size.

With respect to the build system flag, I think that it might be fine to instead just ensure that Tony is not included when 3D is disabled. 300 kb is a lot when you are doing a minimal build (without physics or 3D). But any 3D build is going to be large enough that 300 kb won't make much of a difference (especially since presumably the game will have 3D assets)

@allenwp
Copy link
Contributor

allenwp commented Sep 24, 2024

With respect to the build system flag, I think that it might be fine to instead just ensure that Tony is not included when 3D is disabled. 300 kb is a lot when you are doing a minimal build (without physics or 3D). But any 3D build is going to be large enough that 300 kb won't make much of a difference (especially since presumably the game will have 3D assets)

Tonemapping, like with Tony McMapface, can be used in 2D games when HDR 2D is enabled in the project settings. So this will be another case when the Tony McMapface LUT texture will need to be included.

@clayjohn
Copy link
Member

Tonemapping, like with Tony McMapface, can be used in 2D games when HDR 2D is enabled in the project settings. So this will be another case when the Tony McMapface LUT texture will need to be included.

Ah, good point

@adamscott
Copy link
Member

adamscott commented Sep 26, 2024

Tested locally, it works as expected in all rendering methods.
[Link]

I'm a little bit torn because the Tony McMapface image seems to be the better looking one by far. But I really hate the idea to add 300kb to the already big Web platform export.

@allenwp
Copy link
Contributor

allenwp commented Sep 26, 2024

I'm a little bit torn because the Tony McMapface image seems to be the better looking one by far. But I really hate the idea to add 300kb to the already big Web platform export.

If I understand correctly, your concern is regarding the base transferred data size for users playing web games that use the default web export template. Your concern is not regarding the full package export size for web. Is this correct? For example, it would be fine to have the LUT texture as a separate file that is included in the export but never actually downloaded unless it's used.

As I see it, Godot is designed to give developers control of the export size through build parameters that restrict what features are included in their custom export template. If the developer is not concerned about the package export size, then they use the default export template. This works well for all platforms except for web because in these cases the developer is often the most concerned about export package size, so they can make the decision on whether a custom export template is needed. For example, many distribution platforms have strict requirements for package size, such as mobile and even some consoles: this puts the pressure on the developer to adhere to these requirements and makes the export size the developers' problem.

This approach of requiring the developer to build their own export template does not work well for web exports because there are common scenarios where the developer is not the entity who is most concerned about export size, but instead the player is. With web builds, the player will bounce off a game that takes too long to load or they might have a metered internet connection. This becomes especially problematic with game jam games, etc. where the developer does not have the time to build their own custom export template or simply does not have the interest to care about the amount of data transmitted to each player.

As it stands, web export has one primary solution to this web-specific problem: compress the wasm file before transferring, which brings the 35.38 MB export template down to only 6.78 MB over the wire (as of 4.3).

The problem with adding this sort of incompressible LUT texture data is that it would (theoretically, I haven' checked yet) bump up the 6.78 MB transferred to around 7.08 MB transferred. So this specific PR fights against the primary solution that currently exists for web exports.

Am I understanding all of this correctly?

@adamscott
Copy link
Member

If I understand correctly, your concern is regarding the base transferred data size for users playing web games that use the default web export template. Your concern is not regarding the full package export size for web. Is this correct?

It's a little more complicated than that. Currently, if no manual optimization is made, you actually have to load the entire engine + the entire game assets to even begin to start the game. (the game won't run before loading the entire single .pck file that contains the game assets).

So, whether the LUT texture is in the .wasm file or in the .pck file, it still adds to the waiting time to play the game.

 For example, it would be fine to have the LUT texture as a separate file that is included in the export but never actually downloaded unless it's used.

It's kinda difficult to do. The base engine needs to support that use-case first. And we need to create custom code in the web export to communicate that we need to download that file, and actually load the bytes into the engine once the download completed.

This approach of requiring the developer to build their own export template does not work well for web exports because there are common scenarios where the developer is not the entity who is most concerned about export size, but instead the player is. With web builds, the player will bounce off a game that takes too long to load or they might have a metered internet connection. This becomes especially problematic with game jam games, etc. where the developer does not have the time to build their own custom export template or simply does not have the interest to care about the amount of data transmitted to each player.

The problem is actually more on the stated fact that only one .pck is made for the web export.

@reduz had ideas to actually change the file system to allow streaming of individual files, but it's quite a big endeavor in my humble opinion.

The other solution is to create multiple pcks manually, but it's a tedious task, and the automatic dependency recognition is not perfect at discovering every dependency. And that method can actually lead to load the same asset multiple times if you're not careful enough.

As it stands, web export has one primary solution to this web-specific problem: compress the wasm file before transferring, which brings the 35.38 MB export template down to only 6.78 MB over the wire (as of 4.3).

The engine size is still super big at that size, over the wire with brolti compression. And the bigger problem, as stated, is the default single pck problem. And the fact that once you downloaded that 4-7MB file, you don't have yet a playable game.

@allenwp
Copy link
Contributor

allenwp commented Sep 26, 2024

I can see that there is a lot of nuance to this problem. Obviously, adding an extra 300 KB to the default web export template is a problem. But it also seems like a problem that the engine cannot grow without negatively affecting web exports that are made by developers who don't have an interest in building their own optimized export templates.

The only other thought I have on the subject is that web may need an official (and default) "compact" export template that has some features disabled (like Tony McMapface). The "full" export template would also be officially provided for those making games that make use of the more uncommon engine features. I can immediately see a number of challenges with this and this solution is way outside of the scope of this PR.

return tonemap_aces(max(vec3(0.0f), color), white);
} else { // TONEMAPPER_TONY_MC_MAPFACE
Copy link
Member

Choose a reason for hiding this comment

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

I would make it an explicit else if that checks the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I amended the commit to explicitly check the enum with an else if in both renderer_rd/shaders/effects/tonemap.glsl and gles3/shaders/tonemap_inc.glsl.

@akien-mga
Copy link
Member

My main concern with a build option is that it's only for power users. We have two options:

  • Either we include the LUT by default in official builds, so users can use it, and everyone pays the cost (especially Web users). Power web or mobile developers can compile it out, but that's a fringe of the userbase which has the time and technical skills to learn how to make custom export templates for their game (and definitely none of game jammers, which is a big use case for the web export).
  • Or we don't include the LUT by default because we don't want to include the size of official export templates, and then the feature is as good as not existing. It would require being both a power user and awareness that the feature even exists for people to make their custom editor and export template builds to enable the feature.

So IMO it's not a good solution.

@Calinou How about the second option mentioned? The LUT could only be embedded in the editor (using #ifdef TOOLS_ENABLED) and when selected by the user, it would be saved as a file in res://. This is definitely a bit awkward, it would however solve the problem and would be quite simple to implement. I assume that a 306 KB size increase only on the editor side wouldn't be that bad, it's the exported game where it matters the most and that would be solved, only exported games that actually use the LUT would have it.

I'd like to go back to this suggestion, which IMO makes the most sense. The LUT can be in the editor binary (which will grow for all users, but we're not really constrained there), and not in the export templates binary.

If, and only if, users enable the feature, the LUT would be written to the project folder.

If we think it's awkward for end users that a texture appears randomly in the root of their project (which they might also dislike as it might not fit the structure of their project files), we could consider:

  • Adding a project setting to configure where to write the file
  • Or write it to somewhere in the .godot/ (I guess .godot/imported/, it's not really imported but I don't see where else it would be semantically suited), and then it can even directly be in CompressedTexture2D format or similar

That last option would be the least intrusive for the end users, as it would all happen transparently. But I'm not sure it would be supported out of the box with the current import pipeline, so just writing the file to a configurable path if it's missing might be better.

@akien-mga
Copy link
Member

Actually there may also be a need for users to manually trigger writing the LUT somewhere in their project, if they're not using the Tony McMapface mode by default, but want to expose it as an option to their players.

Selecting that mode in the inspector could add a button to the inspector to write the LUT to disk (with a warning when running without it so users are made aware of that requirement).


Maybe we're overcomplicating too much though and the idea to have two sets of export templates for web, one more minimal than the other, is something I've been thinking about for a while too.

@adamscott
Copy link
Member

How about making it a plugin? It could even be an official plugin.

Like, if we make the engine accept dynamically new tonemapping modes, then people could include it voluntarily.

@Ansraer
Copy link
Contributor

Ansraer commented Sep 27, 2024

We talked about tonemappers in the rendering meeting a few hours ago. Ideally, we would like the shader template PR to support swapping out the tonemapper, which should make it easy to offer custom tone mappers on the assetlib without bloating the binary size.

If that turn out to not be viable without significant additional time investment, we have tentatively agreed that we would like to either merge AGX or Tony.
It is my understanding that in that case AGX would be the preferred option, due to there being a good analytical approximation being available and it also being Blender's default tonemapper. However, right now #87260 is not ready to be merged, the final image is too washed out and it also using the wrong variant of AGX, it is based on the original version of AGX while we would prefer to match the version in Blender, which was created by EaryChow.

@allenwp
Copy link
Contributor

allenwp commented Sep 27, 2024

We talked about tonemappers in the rendering meeting a few hours ago. Ideally, we would like the shader template PR to support swapping out the tonemapper, which should make it easy to offer custom tone mappers on the assetlib without bloating the binary size.

A couple of things to reiterate and note about this approach:

  • This allows developers to perform HDR colour grading as a part of the tone mapping process, creating one or a series of HDR LUT textures that perform all colour grading and tone mapping, similar to the Uncharted 4 approach. With this, swapping LUTs for different parts of the game at runtime is important.
  • Both a shader and the option of a LUT texture(s) of a user-specified HDR texture format will be needed to implement Tony McMapface using this approach.
  • Could be implemented so new tonemaps must be added as plugins (scripting interface), must be added through the editor's Environment inspector (editor interface), or a combination of both approaches; lots of options for how this is implemented.
  • At minimum, it should be possible to implement Tony McMapface to behave identically to this PR (in regards to how it interacts with exposure, glow, etc.)
  • Discoverability of these tonemappers will be very low. Might be good to think about ways to address this. In the docs, maybe?

About HDR output... Maybe relevant, maybe not, depending on how things get implemented:

  • Consider how this will work with HDR output, when that inevitably happens in the future. How will future colour grading (editor tools or user-provided HDR LUT, etc.), glow, etc. interact with this?
  • EA Frostbite's HDR colour grading and HDR tone mapping approach has a few points that are good to keep in mind regarding how a future of HDR output might be implemented.

(Feel free to copy these notes to the new custom tone mapping proposal when it comes around)

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.

9 participants