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

[Merged by Bors] - Rename Light => PointLight and remove unused properties #1778

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/bevy_pbr/src/entity.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{light::Light, material::StandardMaterial, render_graph::PBR_PIPELINE_HANDLE};
use crate::{light::PointLight, material::StandardMaterial, render_graph::PBR_PIPELINE_HANDLE};
use bevy_asset::Handle;
use bevy_ecs::bundle::Bundle;
use bevy_render::{
Expand Down Expand Up @@ -42,8 +42,8 @@ impl Default for PbrBundle {

/// A component bundle for "light" entities
#[derive(Debug, Bundle, Default)]
pub struct LightBundle {
pub light: Light,
pub struct PointLightBundle {
pub point_light: PointLight,
pub transform: Transform,
pub global_transform: GlobalTransform,
}
4 changes: 2 additions & 2 deletions crates/bevy_pbr/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub use light::*;
pub use material::*;

pub mod prelude {
pub use crate::{entity::*, light::Light, material::StandardMaterial};
pub use crate::{entity::*, light::PointLight, material::StandardMaterial};
}

use bevy_app::prelude::*;
Expand All @@ -26,7 +26,7 @@ pub struct PbrPlugin;
impl Plugin for PbrPlugin {
fn build(&self, app: &mut AppBuilder) {
app.add_asset::<StandardMaterial>()
.register_type::<Light>()
.register_type::<PointLight>()
.add_system_to_stage(
CoreStage::PostUpdate,
shader::asset_shader_defs_system::<StandardMaterial>.system(),
Expand Down
40 changes: 12 additions & 28 deletions crates/bevy_pbr/src/light.rs
Original file line number Diff line number Diff line change
@@ -1,30 +1,22 @@
use bevy_core::Byteable;
use bevy_ecs::reflect::ReflectComponent;
use bevy_reflect::Reflect;
use bevy_render::{
camera::{CameraProjection, PerspectiveProjection},
color::Color,
};
use bevy_render::color::Color;
use bevy_transform::components::GlobalTransform;
use std::ops::Range;

/// A point light
#[derive(Debug, Reflect)]
#[reflect(Component)]
pub struct Light {
pub struct PointLight {
pub color: Color,
pub fov: f32,
pub depth: Range<f32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume the original reasoning for why these extra fields were included, was to reuse the same Light type for both Point lights and (future) Spot lights?

I think I like this change. As a user, I think having multiple distinct clearly-named types for different kinds of lights is a good idea.

As for the implementation, I am not very familiar with how differently the different kinds of lights have to be handled for shading. I know Directional/Ambient are special, and those better be their own independent types anyway, but for Point/Spot I am not so sure.

If it's not going to complicate passing of light data and handling in the shader, then this change seems good, also from an implementation perspective. I wonder what the arguments for unifying them into a single type are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From experience I have a strong preference to not put in unused code like this 'because we'll use it soon'. It's simple enough to put it in the next PR and it's only confusing at this point.

For example, while working on shadowmaps, the above fields needed changing anyway...

Copy link
Member

Choose a reason for hiding this comment

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

I think separate types for each light type makes sense, even if point vs spot have some technical overlap. I expect most of the changes in this pr to hold up even when we add spotlights.

pub intensity: f32,
pub range: f32,
}

impl Default for Light {
impl Default for PointLight {
fn default() -> Self {
Light {
PointLight {
color: Color::rgb(1.0, 1.0, 1.0),
depth: 0.1..50.0,
fov: f32::to_radians(60.0),
intensity: 200.0,
range: 20.0,
}
Expand All @@ -33,33 +25,25 @@ impl Default for Light {

#[repr(C)]
#[derive(Debug, Clone, Copy)]
pub(crate) struct LightRaw {
pub proj: [[f32; 4]; 4],
pub(crate) struct PointLightUniform {
pub pos: [f32; 4],
pub color: [f32; 4],
pub inverse_range_squared: f32,
}

unsafe impl Byteable for LightRaw {}
unsafe impl Byteable for PointLightUniform {}

impl LightRaw {
pub fn from(light: &Light, global_transform: &GlobalTransform) -> LightRaw {
let perspective = PerspectiveProjection {
fov: light.fov,
aspect_ratio: 1.0,
near: light.depth.start,
far: light.depth.end,
};

let proj = perspective.get_projection_matrix() * global_transform.compute_matrix();
impl PointLightUniform {
pub fn from(light: &PointLight, global_transform: &GlobalTransform) -> PointLightUniform {
let (x, y, z) = global_transform.translation.into();

// premultiply color by intensity
// we don't use the alpha at all, so no reason to multiply only [0..3]
let color: [f32; 4] = (light.color * light.intensity).into();
LightRaw {
proj: proj.to_cols_array_2d(),
pos: [x, y, z, 1.0 / (light.range * light.range)], // pos.w is the attenuation.
PointLightUniform {
pos: [x, y, z, 1.0],
color,
inverse_range_squared: 1.0 / (light.range * light.range),
}
}
}
Expand Down
41 changes: 21 additions & 20 deletions crates/bevy_pbr/src/render_graph/lights_node.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{
light::{AmbientLight, Light, LightRaw},
light::{AmbientLight, PointLight, PointLightUniform},
render_graph::uniform,
};
use bevy_core::{AsBytes, Byteable};
Expand All @@ -20,13 +20,13 @@ use bevy_transform::prelude::*;
#[derive(Debug, Default)]
pub struct LightsNode {
command_queue: CommandQueue,
max_lights: usize,
max_point_lights: usize,
}

impl LightsNode {
pub fn new(max_lights: usize) -> Self {
LightsNode {
max_lights,
max_point_lights: max_lights,
command_queue: CommandQueue::default(),
}
}
Expand Down Expand Up @@ -57,7 +57,7 @@ impl SystemNode for LightsNode {
let system = lights_node_system.system().config(|config| {
config.0 = Some(LightsNodeSystemState {
command_queue: self.command_queue.clone(),
max_lights: self.max_lights,
max_point_lights: self.max_point_lights,
light_buffer: None,
staging_buffer: None,
})
Expand All @@ -72,7 +72,7 @@ pub struct LightsNodeSystemState {
light_buffer: Option<BufferId>,
staging_buffer: Option<BufferId>,
command_queue: CommandQueue,
max_lights: usize,
max_point_lights: usize,
}

pub fn lights_node_system(
Expand All @@ -82,7 +82,7 @@ pub fn lights_node_system(
// TODO: this write on RenderResourceBindings will prevent this system from running in parallel
// with other systems that do the same
mut render_resource_bindings: ResMut<RenderResourceBindings>,
query: Query<(&Light, &GlobalTransform)>,
query: Query<(&PointLight, &GlobalTransform)>,
) {
let state = &mut state;
let render_resource_context = &**render_resource_context;
Expand All @@ -91,16 +91,16 @@ pub fn lights_node_system(
let ambient_light: [f32; 4] =
(ambient_light_resource.color * ambient_light_resource.brightness).into();
let ambient_light_size = std::mem::size_of::<[f32; 4]>();
let light_count = query.iter().count();
let size = std::mem::size_of::<LightRaw>();
let point_light_count = query.iter().count();
let size = std::mem::size_of::<PointLightUniform>();
let light_count_size = ambient_light_size + std::mem::size_of::<LightCount>();
let light_array_size = size * light_count;
let light_array_max_size = size * state.max_lights;
let current_light_uniform_size = light_count_size + light_array_size;
let max_light_uniform_size = light_count_size + light_array_max_size;
let point_light_array_size = size * point_light_count;
let point_light_array_max_size = size * state.max_point_lights;
let current_point_light_uniform_size = light_count_size + point_light_array_size;
let max_light_uniform_size = light_count_size + point_light_array_max_size;

if let Some(staging_buffer) = state.staging_buffer {
if light_count == 0 {
if point_light_count == 0 {
return;
}

Expand Down Expand Up @@ -132,21 +132,22 @@ pub fn lights_node_system(
let staging_buffer = state.staging_buffer.unwrap();
render_resource_context.write_mapped_buffer(
staging_buffer,
0..current_light_uniform_size as u64,
0..current_point_light_uniform_size as u64,
&mut |data, _renderer| {
// ambient light
data[0..ambient_light_size].copy_from_slice(ambient_light.as_bytes());

// light count
data[ambient_light_size..light_count_size]
.copy_from_slice([light_count as u32, 0, 0, 0].as_bytes());
.copy_from_slice([point_light_count as u32, 0, 0, 0].as_bytes());

// light array
for ((light, global_transform), slot) in query
.iter()
.zip(data[light_count_size..current_light_uniform_size].chunks_exact_mut(size))
{
slot.copy_from_slice(LightRaw::from(&light, &global_transform).as_bytes());
for ((point_light, global_transform), slot) in query.iter().zip(
data[light_count_size..current_point_light_uniform_size].chunks_exact_mut(size),
) {
slot.copy_from_slice(
PointLightUniform::from(&point_light, &global_transform).as_bytes(),
);
}
},
);
Expand Down
4 changes: 3 additions & 1 deletion crates/bevy_pbr/src/render_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use bevy_render::{
};
use bevy_transform::prelude::GlobalTransform;

pub const MAX_POINT_LIGHTS: usize = 10;
pub(crate) fn add_pbr_graph(world: &mut World) {
{
let mut graph = world.get_resource_mut::<RenderGraph>().unwrap();
Expand All @@ -37,7 +38,8 @@ pub(crate) fn add_pbr_graph(world: &mut World) {
node::STANDARD_MATERIAL,
AssetRenderResourcesNode::<StandardMaterial>::new(true),
);
graph.add_system_node(node::LIGHTS, LightsNode::new(10));

graph.add_system_node(node::LIGHTS, LightsNode::new(MAX_POINT_LIGHTS));

// TODO: replace these with "autowire" groups
graph
Expand Down
20 changes: 10 additions & 10 deletions crates/bevy_pbr/src/render_graph/pbr_pipeline/pbr.frag
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@

const int MAX_LIGHTS = 10;

struct Light {
mat4 proj;
struct PointLight {
vec4 pos;
vec4 color;
float inverseRangeSquared;
};

layout(location = 0) in vec3 v_WorldPosition;
Expand All @@ -62,7 +62,7 @@ layout(std140, set = 0, binding = 1) uniform CameraPosition {
layout(std140, set = 1, binding = 0) uniform Lights {
vec4 AmbientColor;
uvec4 NumLights;
Light SceneLights[MAX_LIGHTS];
PointLight PointLights[MAX_LIGHTS];
};

layout(set = 3, binding = 0) uniform StandardMaterial_base_color {
Expand Down Expand Up @@ -130,9 +130,8 @@ float pow5(float x) {
//
// light radius is a non-physical construct for efficiency purposes,
// because otherwise every light affects every fragment in the scene
float getDistanceAttenuation(const vec3 posToLight, float inverseRadiusSquared) {
float distanceSquare = dot(posToLight, posToLight);
float factor = distanceSquare * inverseRadiusSquared;
float getDistanceAttenuation(float distanceSquare, float inverseRangeSquared) {
float factor = distanceSquare * inverseRangeSquared;
float smoothFactor = saturate(1.0 - factor * factor);
float attenuation = smoothFactor * smoothFactor;
return attenuation * 1.0 / max(distanceSquare, 1e-4);
Expand Down Expand Up @@ -343,13 +342,14 @@ void main() {
// accumulate color
vec3 light_accum = vec3(0.0);
for (int i = 0; i < int(NumLights.x) && i < MAX_LIGHTS; ++i) {
Light light = SceneLights[i];
PointLight light = PointLights[i];

vec3 lightDir = light.pos.xyz - v_WorldPosition.xyz;
vec3 L = normalize(lightDir);
vec3 light_to_frag = light.pos.xyz - v_WorldPosition.xyz;
vec3 L = normalize(light_to_frag);
float distance_square = dot(light_to_frag, light_to_frag);

float rangeAttenuation =
getDistanceAttenuation(lightDir, light.pos.w);
getDistanceAttenuation(distance_square, light.inverseRangeSquared);

vec3 H = normalize(L + V);
float NoL = saturate(dot(N, L));
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/3d_scene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ fn setup(
..Default::default()
});
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, 8.0, 4.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/load_gltf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
..Default::default()
});
commands
.spawn_bundle(LightBundle {
.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(3.0, 5.0, 3.0),
..Default::default()
})
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/msaa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn setup(
..Default::default()
});
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, 8.0, 4.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/orthographic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ fn setup(
..Default::default()
});
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(3.0, 8.0, 5.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/parenting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ fn setup(
});
});
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, 5.0, -4.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/pbr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ fn setup(
}
}
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_translation(Vec3::new(0.0, 5.0, 5.0)),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/spawner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ fn setup(
mut materials: ResMut<Assets<StandardMaterial>>,
) {
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, -4.0, 5.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/update_gltf_scene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ fn setup(
mut scene_spawner: ResMut<SceneSpawner>,
mut scene_instance: ResMut<SceneInstance>,
) {
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, 5.0, 4.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/wireframe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ fn setup(
// This enables wireframe drawing on this entity
.insert(Wireframe);
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, 8.0, 4.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/android/android.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ fn setup(
..Default::default()
});
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, 8.0, 4.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/asset/asset_loading.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ fn setup(
..Default::default()
});
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, 5.0, 4.0),
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion examples/asset/hot_asset_reloading.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
// mesh
commands.spawn_scene(scene_handle);
// light
commands.spawn_bundle(LightBundle {
commands.spawn_bundle(PointLightBundle {
transform: Transform::from_xyz(4.0, 5.0, 4.0),
..Default::default()
});
Expand Down
Loading