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

Fix calculation of skybox rotation #17476

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

Conversation

hukasu
Copy link

@hukasu hukasu commented Jan 21, 2025

Objective

Fixes #16628

Solution

Matrices were being applied in the wrong order.

Testing

Ran skybox example with rotations applied to the Skybox on the x, y, and z axis (one at a time).

e.g.

Skybox {
    image: skybox_handle.clone(),
    brightness: 1000.0,
    rotation: Quat::from_rotation_y(-45.0_f32.to_radians()),
}

Showcase

Screencast_20250121_151232.webm

@alice-i-cecile alice-i-cecile added this to the 0.15.2 milestone Jan 21, 2025
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 21, 2025
@BenjaminBrienen BenjaminBrienen added the D-Shaders This code uses GPU shader languages label Jan 22, 2025
Copy link

@mate-h mate-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I also noticed this issue in main while trying to align a skybox and directional light. Thanks for fixing.

Copy link
Contributor

@greeble-dev greeble-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes seem good and I've tested with the skybox example. But left a suggestion to update comments and variable names.

Comment on lines 34 to +43
// Transforming the view space ray direction by the skybox transform matrix, it is
// equivalent to rotating the skybox itself.
var view_ray_direction = view_position_homogeneous.xyz / view_position_homogeneous.w;
view_ray_direction = (uniforms.transform * vec4(view_ray_direction, 1.0)).xyz;
view_ray_direction = (view.world_from_view * vec4(view_ray_direction, 0.0)).xyz;

// Transforming the view space ray direction by the view matrix, transforms the
// direction to world space. Note that the w element is set to 0.0, as this is a
// vector direction, not a position, That causes the matrix multiplication to ignore
// the translations from the view matrix.
let ray_direction = (view.world_from_view * vec4(view_ray_direction, 0.0)).xyz;
let ray_direction = (uniforms.transform * vec4(view_ray_direction, 0.0)).xyz;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comments and variable names are now misleading? Should be something like:

Suggested change
// Transforming the view space ray direction by the skybox transform matrix, it is
// equivalent to rotating the skybox itself.
var view_ray_direction = view_position_homogeneous.xyz / view_position_homogeneous.w;
view_ray_direction = (uniforms.transform * vec4(view_ray_direction, 1.0)).xyz;
view_ray_direction = (view.world_from_view * vec4(view_ray_direction, 0.0)).xyz;
// Transforming the view space ray direction by the view matrix, transforms the
// direction to world space. Note that the w element is set to 0.0, as this is a
// vector direction, not a position, That causes the matrix multiplication to ignore
// the translations from the view matrix.
let ray_direction = (view.world_from_view * vec4(view_ray_direction, 0.0)).xyz;
let ray_direction = (uniforms.transform * vec4(view_ray_direction, 0.0)).xyz;
// Transforming the view space ray direction by the view matrix, transforms the
// direction to world space. Note that the w element is set to 0.0, as this is a
// vector direction, not a position, That causes the matrix multiplication to ignore
// the translations from the view matrix.
let view_ray_direction = view_position_homogeneous.xyz / view_position_homogeneous.w;
let world_ray_direction = (view.world_from_view * vec4(view_ray_direction, 0.0)).xyz;
// Transforming the world space ray direction by the skybox transform matrix, it is
// equivalent to rotating the skybox itself.
let ray_direction = (uniforms.transform * vec4(world_ray_direction, 0.0)).xyz;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Shaders This code uses GPU shader languages D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Skybox rotation issue
5 participants