Skip to content

Conversation

nnethercote
Copy link
Contributor

It's supposed to produce a landscape with a small white sun, blue sky, and yellow ground. Instead it produces a small white sun and everything else is black. The problem is with the
sun_intensity_extra_spec_const_factor argument to sky_shader::fs.

  • When running on the GPU, sky_shader::fs is called from main_fs, which has a spec_constant with a default value of 100.

  • When running on the CPU, sky_shader::fs is called directly from the CPU shader's main, and it passes 1.

In other words, the CPU shader's sun intensity is 100x too small, which explains why it's so dark. This commit changes the value to 100, which makes the CPU shader produce the expected result.

(The shader later divides the intensity value by 100. There are comments about integration testing for specialization constants that I don't understand.)

@nnethercote
Copy link
Contributor Author

It looks like the CPU sky shader was broken in 4c6cf0d, which added the 100x intensity value in order to implement adjustable sun brightness in the ash runner. The CPU runner wasn't updated accordingly (it needed to be because it doesn't go via main_fs, unlike the GPU runners) and I guess there are no tests for the CPU runner.

It's supposed to produce a landscape with a small white sun, blue sky,
and yellow ground. Instead it produces a small white sun and everything
else is black. The problem is with the
`sun_intensity_extra_spec_const_factor` argument to `sky_shader::fs`.

- When running on the GPU, `sky_shader::fs` is called from `main_fs`,
  which has a spec_constant with a default value of 100.

- When running on the CPU, `sky_shader::fs` is called directly from the
  CPU shader's `main`, and it passes 1.

In other words, the CPU shader's sun intensity is 100x too small, which
explains why it's so dark. This commit changes the value to 100, which
makes the CPU shader produce the expected result.

(The shader later divides the intensity value by 100. There are comments
about integration testing for specialization constants that I don't
understand.)
@Firestar99 Firestar99 added this pull request to the merge queue Sep 18, 2025
Merged via the queue into Rust-GPU:main with commit 19d23e5 Sep 18, 2025
13 checks passed
@nnethercote nnethercote deleted the fix-cpu-sky-shader branch September 18, 2025 23:59
@eddyb
Copy link
Member

eddyb commented Sep 22, 2025

My bad - I never put this up as a PR for whatever reason:

$ git show 97c0e5be
commit 97c0e5be87dab30b0ea26495845858d5adbb882c
Author: Eduard-Mihai Burtescu <[email protected]>
Date:   Sat Nov 25 09:43:57 2023 +0200

    examples/runners/cpu: fix intensity factor, and increase resolution.

diff --git a/examples/runners/cpu/src/main.rs b/examples/runners/cpu/src/main.rs
index 30c12a9420d..90dc4da78af 100644
--- a/examples/runners/cpu/src/main.rs
+++ b/examples/runners/cpu/src/main.rs
@@ -97,8 +97,8 @@ fn color_u32_from_vec4(v: Vec4) -> u32 {
 }
 
 fn main() {
-    const WIDTH: usize = 1280;
-    const HEIGHT: usize = 720;
+    const WIDTH: usize = HEIGHT * 16 / 9;
+    const HEIGHT: usize = 1080;
 
     let mut window = Window::new(
         "Rust GPU - CPU shader evaluation",
@@ -141,7 +141,7 @@ fn main() {
                 * vec2(WIDTH as f32, HEIGHT as f32);
 
             // evaluate the fragment shader for the specific pixel
-            let color = shader_module::fs(&push_constants, frag_coord, 1);
+            let color = shader_module::fs(&push_constants, frag_coord, 100);
 
             color_u32_from_vec4(color)
         })

(ironically, because I keep rebasing everything on top of main, this commit now makes no sense, because it has the resolution but not the intensity factor change)

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

Successfully merging this pull request may close these issues.

3 participants