-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
base: master
Are you sure you want to change the base?
Conversation
b5027c4
to
882f6bd
Compare
Thanks for opening a pull request, and going through the effort of implementing this 🙂
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.
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
|
I don't quite understand how this idea of user-provided LUT would work. Here is my understanding of this:
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 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 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. |
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. |
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.
@JoNax97 The LUT itself isn't generated at build time, just the 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. |
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… |
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. |
This comment was marked as resolved.
This comment was marked as resolved.
@Calinou How about the second option mentioned? The LUT could only be embedded in the editor (using |
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. |
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. |
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 |
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. |
The original author responded to a question on their repo and basically said that you can't go smaller without sacrificing quality significantly. |
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? |
Edit: Sorry, I made a number of edits to this comment 😅
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 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? |
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 The |
I did a very quick check of
6681f25 882f6bd 314,368 bytes saved with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
882f6bd
to
d63c94e
Compare
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.
|
@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) |
Tonemapping, like with Tony McMapface, can be used in 2D games when |
Ah, good point |
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? |
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.
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.
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.
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. |
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
My main concern with a build option is that it's only for power users. We have two options:
So IMO it's not a good solution.
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:
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. |
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. |
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. |
d63c94e
to
9a58e84
Compare
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. |
A couple of things to reiterate and note about this approach:
About HDR output... Maybe relevant, maybe not, depending on how things get implemented:
(Feel free to copy these notes to the new custom tone mapping proposal when it comes around) |
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 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
):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:
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.