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 a global enable/disable shadows #326

Closed
Janders1800 opened this issue Dec 21, 2019 · 9 comments
Closed

Add a global enable/disable shadows #326

Janders1800 opened this issue Dec 21, 2019 · 9 comments

Comments

@Janders1800
Copy link

Describe the project you are working on:
A 3D exploration/puzzle game for mobile. I want to be able to enable or disable shadows on a settings menu.
It would be nice to have a bool variable that you can tweak via Project Settings or script.

Describe the problem or limitation you are having in your project:
Currently there is no way to enable/disable all shadows on a project, you have to setup a script that enables/disables shadows for every light source, a nightmare if you have dozens of lights per level.

Describe how this feature / enhancement will help you overcome this problem or limitation:
Not really for me right now, since I'm stuck with 3.2 until I finish this project, but for other users and future projects it will save a lot of work and setup.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:
Maybe an 'Enabled' option under Quality->Shadows
Shadows
Or something like this by script:
ProjectSettings.set("Shadows/Enabled", true)

Describe implementation detail for your proposal (in code), if possible:
Add an if before calculating shadows?

If this enhancement will not be used often, can it be worked around with a few lines of script?:
It can be worked around with a singleton and a line of code, the pain is in to setup every light to get to work.

Is there a reason why this should be core and not an add-on in the asset library?:
I don't think its possible to make it efficient in a plugin

@clayjohn
Copy link
Member

Not sure if it's worth it when this can be done with so few lines of code.

If I were you I'd add every light to a group and then just iterate over that group when you need to.

Adding a global enable/disable would require adding logic to very performance sensitive parts of the engine and I just don't see the benefit. My personal position is that the core rendering stuff should be kept simple and lightweight as much as possible.

@Janders1800
Copy link
Author

Yeah well, this is more a UX problem that a code one. As I said, the pain is to set up every light not the code.

I don't know Godot guts that well but it will really surprise me if this is such a drastic change or cost performant.

@Jummit
Copy link

Jummit commented Dec 21, 2019

Add all your lights to a group and do SceneTree.call_group("Lights", "set_shadow", false),
Or do get_tree().root.propagate_call("set_shadow", [true / false]).

I don't think its possible to make it efficient in a plugin

Here you go:
shadow_toggler.zip

Just set ShadowToggler.shadows_enabled to true or false.

@Janders1800
Copy link
Author

Janders1800 commented Dec 21, 2019

Thanks @Jummit didn't knew of propagate_call. In my game I went for the groups solution, since I want some lights to have shadows and some to don't.

Still, the point of my feature request is not that it can be done by code. The point is that it's a pain/confusing setup for new users, and can be avoided by adding a global shadow controller. It is a UX problem.

@Jummit
Copy link

Jummit commented Dec 21, 2019

image

@Calinou
Copy link
Member

Calinou commented Dec 21, 2019

Disabling shadows for all lights will likely lead to a broken appearance in most games. Instead, you want to only disable shadows for smaller/far away lights or use baked lighting.

We might add a LOD system of sorts to lights in 4.0 to fade their visibility and shadows based on their radius/distance. This can already be achieved by scripting, but I figure a C++ implementation would perform faster if you have hundreds of lights in your scene.

Edit: This was implemented by godotengine/godot#58512. Also, you can globally disable positional shadows in a viewport by setting the shadow atlas size to 0 (see https://github.com/godotengine/godot-demo-projects/tree/master/3d/graphics_settings). The project setting doesn't allow you to do this due to its property hint, but this can be achieved using a script.

@Janders1800
Copy link
Author

Janders1800 commented Dec 21, 2019

Nice, Its nice to see there are improvements planned.

Disabling shadows for all lights will likely lead to a broken appearance in most games.

I disagree there, if the game is designed for such eventuality there is no problem.

Instead, you want to only disable shadows for smaller/far away lights or use baked lighting.

Unfortunately that is not an option, bakedlightmap is broken on mobile, and even if it worked, there is no point in using it since lights baked on 'All' mode don't lit 'non baked' objects (but that is another discussion).

@clayjohn
Copy link
Member

I think the point that Calinou is trying to make is that turning off shadows for all lights is a very specific use case and is likely to only be useful for your game.

In my opinion, since this can be done more efficiently in GDScript and in a single line of code it is better to leave it as a plugin or a shareable line of code.

@akien-mga
Copy link
Member

I agree with most comments here that:

  • This can be achieved already with a couple lines of code
  • This is a feature is very game-specific, and is unlikely to be useful for the vast majority of users

As per our best practices for engine contributions defined in https://docs.godotengine.org/en/latest/community/contributing/best_practices_for_engine_contributors.html#the-problem-has-to-be-complex-or-frequent, this is typically something that we would not accept in the engine.

Moreover, the concerns expressed by @clayjohn for the performance cost of having a boolean toggle in the core rendering is another reason not to implement such a convenience for a limited use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants