godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.98k stars 21.16k forks source link

HDR skybox colors are too dark/bright with compatibility renderer #83788

Open discapes opened 1 year ago

discapes commented 1 year ago

Godot version

v4.2.beta2.official [f8818f85e], v4.1.2.stable.fedora [399c9dc39]

System information

Fedora 40 (Rawhide) - OpenGL API 4.6 (Core Profile) Mesa 23.2.1 - Compatibility - Using Device: Intel - Mesa Intel(R) Xe Graphics (TGL GT2)

Issue description

When I add a skybox with a .hdr or .exr texture, it looks good on the Forward+ and Mobile renderers, but not with the Compatibility renderer. See images:

Expected (rendered with Forward+): image Actual (rendered with Compatibility): image

Steps to reproduce

  1. Create project
  2. Add camera
  3. Add WorldEnvironment
  4. Set the background to Sky, and set the Sky Material to a PanoramicSky, and set the Panorama to a HDR image
  5. Observe how it looks good in Forward+ and Mobile, but not in Compatibility

Minimal reproduction project

N/A

discapes commented 1 year ago

Btw, this issue isn't present in v3.5.2.stable.fedora [170ba337a], so I assume the regression tag is approprate

AThousandShips commented 1 year ago

We don't normally add regression tag between 3.x and 4.x, it mainly matters between 4.x version to track things that have done wrong

jsjtxietian commented 1 year ago

From https://docs.godotengine.org/en/stable/contributing/development/core_and_modules/internal_rendering_architecture.html, "Unlike the mobile renderer, colors are tonemapped and stored in sRGB format so there is no HDR support."

discapes commented 1 year ago

Is it planned? I would be really nice since skyboxes are a big part of many games.

clayjohn commented 1 year ago

In the compatibility renderer it looks the same as the GLES2 renderer in 3.x which is kind of expected

Screenshot from 3.5.1 GLES2 renderer image

The underlying issue here is that the sky shader assumes that it is going to receive a texture encoding in sRGB space (0-1 range). But instead it receives a linear texture. So the color space is all wonky.

We can fix this, but any fix comes with tradeoffs.

Option 1:

Force the Compatibility renderer to always use sRGB textures. This would result in us compressing the image to a lower precision format and compressing to the 0-1 range. This would make the HDRI look closer to the other renderers and also slightly improve performance. But it would restrict users from reading accurate color data out of HDRIs.

This issue will also affect 3D shaders if users use HDRIs as input. This option would fix the problem for both sky shaders and 3D shaders.

Option 2:

Detect if an HDRI is being used and provide a slightly different shader path for it. This would be worse for performance than option 1, but would look better. It would also allow users to continue user HDR textures in 3D shaders which would continue to be broken.

Edit: the tentative fix (option 1) looks like this:

image

Note how the bright parts of the sky wash out towards white

ParsleighScumble commented 1 year ago

There's got to be a good way to do this... Maybe if we internally convert the hdri into RGBE and make the sky shader expect that format instead it'll be ok for bandwidth? Just speculating, I'm not too sure about this.

jane00 commented 8 months ago

I also got the dark one when I run the project on Android mobile.

vaunakiller commented 1 month ago

To anyone looking for a quick dirty hack:

  1. Convert HDR texture to PNG or JPEG. (there are plenty of converters online - just google it)
  2. This PNG/JPEG still will be too dark. Throw it into image editor of choice. Play with gamma values (around 2.0 worked fine for me). If either sky or ground are blown out you can try adjusting levels first. Ofcourse you will loose quality. But hey - that worked for me in "lower fidelity" graphics style 😊

P.S. Bonus points for getting compression artifacts - JPEG gets some muddy feeling, while PNG's remind me of dithering and banding.