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

Built-in skybox #8275

Merged
merged 31 commits into from
Apr 2, 2023
Merged

Built-in skybox #8275

merged 31 commits into from
Apr 2, 2023

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Mar 31, 2023

Objective

Solution

  • Add a skybox plugin that renders a fullscreen triangle, and then modifies the vertices in a vertex shader to enforce that it renders as a skybox background.
  • Skybox is run at the end of MainOpaquePass3dNode.
  • In the future, it would be nice to get something like bevy_atmosphere built-in, and have a default skybox+environment map light.

Changelog

  • Added Skybox.
  • EnvironmentMapLight now renders in the correct orientation.

Migration Guide

  • Flip EnvironmentMapLight maps if needed to match how they previously rendered (which was backwards).

@JMS55
Copy link
Contributor Author

JMS55 commented Mar 31, 2023

Currently this is not used in any examples. Should it be? Also, what should we do about the existing skybox example?

@JMS55 JMS55 requested review from superdump and robtfm March 31, 2023 15:17
@JMS55 JMS55 added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Mar 31, 2023
@JMS55 JMS55 added this to the 0.11 milestone Mar 31, 2023
@alice-i-cecile
Copy link
Member

I'm happy that this exists! Unsure if this is the right crate for it: I really wish the rendering crate structure was better split up so users could more easily pull in only the things they need for their games.

@alice-i-cecile
Copy link
Member

Currently this is not used in any examples. Should it be? Also, what should we do about the existing skybox example?

This should be used to replace the existing skybox example please.

@superdump
Copy link
Contributor

Part of the value of the current skybox example is to show use of cubemaps with some different compressed texture types. So, I would say, update the current skybox example to use this plugin.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

This isn't a full review but there are some significant things to address.

crates/bevy_core_pipeline/src/skybox/skybox.wgsl Outdated Show resolved Hide resolved
crates/bevy_core_pipeline/src/skybox/skybox.wgsl Outdated Show resolved Hide resolved
crates/bevy_core_pipeline/src/skybox/mod.rs Outdated Show resolved Hide resolved
crates/bevy_core_pipeline/src/skybox/node.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@JonahPlusPlus JonahPlusPlus 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 liked how you used the vertex shader to put the vertices on the far plane.

I was going to ask whether it handles split screen, but I see it adds the mesh during the pipeline, so that works. You could probably remove that match statement for drawing indexed/unindexed since you know the mesh before hand.

Other than that, there was some talk about using a fullscreen triangle? Did that not work out, or is it planned for a future PR? Either way, this works fine and probably makes the math for sampling easier.

@JMS55
Copy link
Contributor Author

JMS55 commented Mar 31, 2023

A fullscreen triangle is probably possible, but I couldn't figure out the math myself. This seems simpler, and works. Someone else could try and get the triangle working if they want.

@JMS55 JMS55 requested a review from superdump March 31, 2023 22:54
//
// The top-left has UV 0,0, the bottom-left has 0,2, and the top-right has 2,0.
// This means that the UV gets interpolated to 1,1 at the bottom-right corner
// of the clip-space rectangle that is at 1,-1 in clip space.
Copy link
Contributor

Choose a reason for hiding this comment

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

For other reviewers, I added this documentation as part of figuring out how to do the fullscreen triangle approach.

crates/bevy_core_pipeline/src/skybox/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@robtfm robtfm 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.

it's not this pr's fault i think, but it doesn't line up with environment mapping.

on the example i added a box mesh and an environment map with the skybox cubemap texture to check it looked right. it looks correct (i.e. reflects the skybox) from above and below, and looking at +/- x, but when i look at +z or -z the reflection is the same as the skybox behind so i suspect we need the same handedness switch for environment maps.

Comment on lines +29 to +34
let clip_position = vec4(
f32(vertex_index & 1u),
f32((vertex_index >> 1u) & 1u),
0.25,
0.5
) * 4.0 - vec4(1.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let clip_position = vec4(
f32(vertex_index & 1u),
f32((vertex_index >> 1u) & 1u),
0.25,
0.5
) * 4.0 - vec4(1.0);
let clip_xy = vec2<f32>(f32(vertex_index & 1u), f32(vertex_index >> 1u)) * 4.0 - 1.0;
let clip_position = vec4<f32>(clip_xy, 0.0, 1.0);

just to keep the logic closer to fullscreen.wgsl and reduce cognitive load a bit.

Copy link
Contributor

@superdump superdump Apr 2, 2023

Choose a reason for hiding this comment

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

I would agree but I remember it being argued by industry peeps that my version is faster. I don’t see how though.

Co-authored-by: robtfm <50659922+robtfm@users.noreply.github.com>
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 2, 2023
@JMS55
Copy link
Contributor Author

JMS55 commented Apr 2, 2023

@robtfm is the environment map sampling now correct?

@superdump
Copy link
Contributor

@robtfm is the environment map sampling now correct?

I checked, it is now correct! :)

@superdump superdump enabled auto-merge April 2, 2023 10:47
@superdump superdump added this pull request to the merge queue Apr 2, 2023
Merged via the queue into bevyengine:main with commit f0f5d79 Apr 2, 2023
@Selene-Amanita Selene-Amanita added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 2023
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First-party skybox support
7 participants