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

Tweak the editor environment defaults and its interface #49736

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

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 19, 2021

  • Use ACES tonemapping instead of filmic tonemapping to provide a better appearance for bright lights.
  • Set the default tonemap whitepoint to 6 to make filmic tonemapping less bright and blown out. Linear tonemapping is unaffected.
  • Add tooltips to post-process buttons to explain what they do and how they work.
  • Rename Tonemap button to ACES Tonemap to better explain its effect when enabled.
  • Reorder post-process buttons from least to most expensive.
  • Decrease the maximum shadow distance for the default sun to improve shadow texel density. It's now the same as the DirectionalLight3D node's default value (100).

This closes godotengine/godot-proposals#3274 and addresses #54717 (will be split into a separate PR).

Preview

SSAO is now disabled by default (not shown on the preview).

image

@Calinou Calinou requested a review from a team as a code owner June 19, 2021 09:38
@Calinou Calinou added this to the 4.0 milestone Jun 19, 2021
@Calinou Calinou force-pushed the tweak-editor-environment-defaults branch 3 times, most recently from 4d3037a to 6c84024 Compare June 19, 2021 09:48
@trollodel
Copy link
Contributor

I understand why this is needed, but using the same settings of this PR I reach 13 fps in an empty scene with a UHD 620 🙁.
Before have this PR merged, please consider adding an editor setting for the editor environment or preserve that environment in the editor session and not per scene.

@Calinou
Copy link
Member Author

Calinou commented Jun 19, 2021

I understand why this is needed, but using the same settings of this PR I reach 13 fps in an empty scene with a UHD 620 slightly_frowning_face.
Before have this PR merged, please consider adding an editor setting for the editor environment or preserve that environment in the editor session and not per scene.

We should eventually detect whether integrated graphics are in use and disable specific features automatically. We could also enable half-resolution viewport rendering by default on IGPs, as it makes for a much smoother experience overall.

Also, remember that this PR doesn't tweak SSAO to render at half resolution by default, which makes it significantly slower than it could be.

@trollodel
Copy link
Contributor

trollodel commented Jun 19, 2021

Also, remember that this PR doesn't tweak SSAO to render at half resolution by default, which makes it significantly slower than it could be.

Reducing SSAO quality allows me to reach ~35-40 fps, enough for opening the environment preview settings and disable it entirely 🙂.

@reduz
Copy link
Member

reduz commented Aug 24, 2021

Indeed this kills performance on IGP, should definitely not be enabled by default.

@Calinou Calinou force-pushed the tweak-editor-environment-defaults branch from 6c84024 to 58da310 Compare August 25, 2021 05:57
@Calinou
Copy link
Member Author

Calinou commented Aug 25, 2021

Indeed this kills performance on IGP, should definitely not be enabled by default.

I amended the pull request to keep SSAO disabled by default. (Nonetheless, if high performance on IGPs is our goal, we should also disable glow by default.)

@Calinou
Copy link
Member Author

Calinou commented Nov 8, 2021

As per #54717, this PR now disables glow by default to improve performance on low-end GPUs.

@akien-mga
Copy link
Member

Disable glow by default to improve performance on low-end GPUs.

This might be better to split into its own PR. @reduz said that it's surprising that glow would have such a performance impact and this needs further research. But the editor tweaks are good.

@Calinou Calinou force-pushed the tweak-editor-environment-defaults branch from 8ef2680 to f5a53a6 Compare February 10, 2022 18:22
@Calinou
Copy link
Member Author

Calinou commented Feb 10, 2022

As discussed on the PR meeting, I amended this pull request to use ACES tonemapping by default. It makes bright highlights look more realistic. I also restored glow by default.

@Zireael07
Copy link
Contributor

Bumpity bumpity. Having spent an hour tweaking stuff to work with ACES, better defaults are definitely needed.

@Ansraer
Copy link
Contributor

Ansraer commented Jul 7, 2022

Could you post an updated screenshot? The description and code appear reasonable, but the outdated screenshot makes it look like godot-proposals/issues/348 needs to be reopened.
Once it has been confirmed that this doesn't reintroduce the blue tint this has my tentative approval.

@Calinou
Copy link
Member Author

Calinou commented Jul 7, 2022

Could you post an updated screenshot? The description and code appear reasonable, but the outdated screenshot makes it look like godot-proposals/issues/348 needs to be reopened. Once it has been confirmed that this doesn't reintroduce the blue tint this has my tentative approval.

Rebased and tested again, it works as expected:

2022-07-08_00 03 50

@reduz
Copy link
Member

reduz commented Jul 30, 2022

I think for the time being nothing should be done with this until we do proper optimization of the rendering engine.

@Calinou
Copy link
Member Author

Calinou commented Jul 30, 2022

I think for the time being nothing should be done with this until we do proper optimization of the rendering engine.

This PR no longer changes any performance-related options, just default visuals. Reducing the default maximum distance of the directional shadow is especially important to improve its texel density. I've seen multiple complaints about this on Rocket.Chat and Discord in the last few weeks.

@Calinou Calinou force-pushed the tweak-editor-environment-defaults branch from 9256960 to 5d513c9 Compare August 1, 2022 16:42
@TokisanGames
Copy link
Contributor

The current Godot 4 a13 ground color is too saturated and unattractive. I recommend a default of (.25, .22, .2) #403933, which will look good under filmic or ACES.

Examples in context. Original on left, new color under filmic, new under ACES:

image

The sky is too desaturated and the horizon is washed out. In reality, the sky gets darker the closer towards the zenith one looks, and the horizon is sharp. I recommend these colors which look good under filmic or ACES:
Color: 47577d
Sky Horizon: 939ba3
Ground Horizon: 737980

Current vs these numbers on filmic vs ACES:

image

Compare against these reference photos:

reference-sky-clouds

More:
https://user-images.githubusercontent.com/632766/71625352-4ff22180-2c22-11ea-9fbc-da364b5e389f.jpg
https://user-images.githubusercontent.com/632766/71616066-52875380-2bef-11ea-8d85-5eca462fe2eb.jpg
https://c1.staticflickr.com/3/2553/3933147568_d9454fe60c_b.jpg
https://get.pxhere.com/photo/landscape-horizon-field-ground-prairie-hill-dirt-road-natural-soil-agriculture-plain-grassland-wetland-plateau-habitat-ecosystem-green-grass-steppe-spting-natural-environment-grass-family-land-lot-649904.jpg
https://get.pxhere.com/photo/landscape-nature-path-grass-outdoor-horizon-sky-sun-track-road-trail-field-prairie-countryside-hill-view-country-summer-travel-dirt-road-cliff-walk-rural-green-scenic-dirt-natural-park-scenery-soil-agriculture-terrain-ridge-plain-dirt-path-grassland-wetland-plateau-day-scene-habitat-ecosystem-mound-steppe-rural-area-natural-environment-land-lot-coronado-heights-748642.jpg

@Calinou
Copy link
Member Author

Calinou commented Aug 7, 2022

The sky is too desaturated and the horizon is washed out. In reality, the sky gets darker the closer towards the zenith one looks, and the horizon is sharp. I recommend these colors which look good under filmic or ACES:

This kind of realistic sky doesn't necessarily look good, especially due to lighting looking too harsh.

@TokisanGames
Copy link
Contributor

I suppose it's in the eye of the beholder what looks good. However, I trust my eyes, since I've spent years training them in visual media.

IMO, the ground color is really ugly. It's poo brown, not dirt brown. This is the primary reason I spent the time to test different colors and make recommendations. Please don't dismiss them so casually.

The sky color is only mildly unattractive. It's just flat and unrealistic. These shots aren't considering lighting, only color. You know ACES adds contrast and makes lighting harsh. The default color makes the horizon look like the ground is alpha blending with the sky. The real horizon doesn't turn transparent.

@Calinou
Copy link
Member Author

Calinou commented Aug 7, 2022

@tinmanjuggernaut Either way, this is unrelated to this PR (which doesn't intend to change sky defaults as to keep its scope small). Please open a proposal for this.

@YuriSizov
Copy link
Contributor

Discussed in a editor team meeting and while some of the changes would need to be reviewed by the rendering team, we've agreed that the shadow distance can be adjusted now. Needs to be moved to a dedicated PR.

- Use ACES tonemapping instead of filmic tonemapping to provide a
  better appearance for bright lights.
- Set the default tonemap whitepoint to 6 to make filmic tonemapping
  less bright and blown out. Linear tonemapping is unaffected.
- Add tooltips to post-process buttons to explain what they do and
  how they work.
- Rename Tonemap button to ACES Tonemap to better explain its effect
  when enabled.
- Reorder post-process buttons from least to most expensive.
@Calinou
Copy link
Member Author

Calinou commented Aug 18, 2022

Discussed in a editor team meeting and while some of the changes would need to be reviewed by the rendering team, we've agreed that the shadow distance can be adjusted now. Needs to be moved to a dedicated PR.

Done: #64587

@atirut-w
Copy link
Contributor

  • Use ACES tonemapping instead of filmic tonemapping to provide a better appearance for bright lights.

How did it become the current default in the first place instead of ACES?

@Calinou
Copy link
Member Author

Calinou commented Sep 30, 2022

How did it become the current default in the first place instead of ACES?

ACES was not "fitted" when it was first added in Godot 3.0 (then ported to 4.0), so highlights didn't become less saturated. Also, ACES has a tendency to increase contrast and darken parts of the scene that are already dark (which can harm readability). This can make it difficult to use without a color correction curve to balance this out.

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.

Decrease the preview environment's directional light shadow maximum distance for sharper shadows
9 participants