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

Vulkan: Sky process mode can be set to realtime even when it makes no sense to do so #53171

Open
Zireael07 opened this issue Sep 28, 2021 · 5 comments

Comments

@Zireael07
Copy link
Contributor

Godot version

v4.0.dev.calinou [de91700]

System information

Linux Manjaro, Vulkan, Intel Kaby Lake

Issue description

According to the docs https://docs.godotengine.org/en/latest/classes/class_sky.html#class-sky-property-process-mode Realtime process mode only works for cubemaps of size 256.

Therefore it makes no sense to display that option in the UI if cubemap/radiance size is set to something else. Same goes for scripting.

Tangentially related, automatic process setting is NOT documented at all.

Steps to reproduce

Open up the editor, play around with the default world environment.

Minimal reproduction project

N/A

@clayjohn
Copy link
Member

I don't agree with hiding a setting like that. Right now of cubemap size is anything other than 256 and a user sets the mode to "realtime" the user gets a helpful warning message saying that the size was set to 256 internally.

I don't think hiding an important setting benefits users in any way.

@Calinou Calinou changed the title [Vulkan] Sky process mode can be set to realtime even when it makes no sense to do so Vulkan: Sky process mode can be set to realtime even when it makes no sense to do so Sep 28, 2021
@Calinou
Copy link
Member

Calinou commented Sep 28, 2021

I agree the cubemap size should be hidden when the process mode is set to RealTime. We generally use this approach of hiding irrelevant properties elsewhere. For instance, many properties are hidden in StandardMaterial3D if you use vertex shading (at least, it works this way with 3.x's SpatialMaterial).

@Zireael07
Copy link
Contributor Author

@clayjohn: Didn't see that warning message at all! Probably because I was writing a shader at the time. Can Godot please change to an output tab automatically in such situations?

@clayjohn
Copy link
Member

Groud is working on adding toasts to the editor which will allow warnings like that to popup #52940

I agree the cubemap size should be hidden when the process mode is set to RealTime. We generally use this approach of hiding irrelevant properties elsewhere. For instance, many properties are hidden in StandardMaterial3D if you use vertex shading (at least, it works this way with 3.x's SpatialMaterial).

This is how it worked originally, but then the only way to get rid of the warning would be to change your radiance update mode to something other than "realtime", change size, and then change it back to "realtime".

@Calinou
Copy link
Member

Calinou commented Apr 12, 2023

This is how it worked originally, but then the only way to get rid of the warning would be to change your radiance update mode to something other than "realtime", change size, and then change it back to "realtime".

On the other hand, should we have this run-time warning in the first place? We generally prefer to use a different approach in other nodes/resources:

  • Make a note in the class reference.
  • Hide the property in the inspector when it has no effect.

While this doesn't cover the situation where someone sets the radiance map size at runtime in a script, this is an unlikely scenario in the first place. Also, we should try to follow the same warning logic in all nodes whenever possible for consistency.

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

3 participants