ronantakizawa/codereview-bench huggingface dataset:
Repository: bevyengine/bevy
File: benches/benches/bevy_ecs/events/send.rs (Rust)

Code before:
use bevy_ecs::prelude::*;

struct A(f32);

pub struct Benchmark(Events<A>, usize);

impl Benchmark {
    pub fn new(count: usize) -> Self {
        let mut events = Events::default();
        
        // Force the internal buffers to be allocated
        for _ in 0..2 {
            for _ in 0..count {
                events.send(A(0.0));
            }
            events.update();
        }

        Self(events, count)
    }

    pub fn run(&mut self) {
        for _ in 0..self.1 {
            self.0.send(A(0.0));
        }
        self.0.update();
    }
}


Review comment (suggestion):
```suggestion
            self.0.send(std::hint::black_box(A(0.0)));
```

This might prevent some over-optimizations.

Code after:
use bevy_ecs::prelude::*;

pub struct Benchmark<const SIZE: usize> {
    events: Events<[u8; SIZE]>,
    count: usize,
}

impl<const SIZE: usize> Benchmark<SIZE> {
    pub fn new(count: usize) -> Self {
        let mut events = Events::default();
        
        // Force both internal buffers to be allocated.
        for _ in 0..2 {
            for _ in 0..count {
                events.send([0u8; SIZE]);
            }
            events.update();
        }

        Self { events, count }
    }

    pub fn run(&mut self) {
        for _ in 0..self.count {
            self.events.send(std::hint::black_box([0u8; SIZE]));
        }
        self.events.update();
    }
}
Repository: bevyengine/bevy
File: benches/benches/bevy_ecs/events/send.rs (Rust)

Code before:
use bevy_ecs::prelude::*;

struct A(f32);

pub struct Benchmark(Events<A>, usize);

impl Benchmark {
    pub fn new(count: usize) -> Self {
        let mut events = Events::default();
        
        // Force the internal buffers to be allocated
        for _ in 0..2 {
            for _ in 0..count {
                events.send(A(0.0));
            }
            events.update();
        }

        Self(events, count)
    }

    pub fn run(&mut self) {
        for _ in 0..self.1 {
            self.0.send(A(0.0));
        }
        self.0.update();
    }
}


Review comment (suggestion):
```suggestion
        // Force both internal buffers to be allocated.
```

Code after:
use bevy_ecs::prelude::*;

pub struct Benchmark<const SIZE: usize> {
    events: Events<[u8; SIZE]>,
    count: usize,
}

impl<const SIZE: usize> Benchmark<SIZE> {
    pub fn new(count: usize) -> Self {
        let mut events = Events::default();
        
        // Force both internal buffers to be allocated.
        for _ in 0..2 {
            for _ in 0..count {
                events.send([0u8; SIZE]);
            }
            events.update();
        }

        Self { events, count }
    }

    pub fn run(&mut self) {
        for _ in 0..self.count {
            self.events.send(std::hint::black_box([0u8; SIZE]));
        }
        self.events.update();
    }
}
Repository: bevyengine/bevy
File: crates/bevy_ecs/src/schedule/set.rs (Rust)

Code before:

/// Types that can be converted into a [`SystemSet`].
pub trait IntoSystemSet<Marker>: Sized {
    /// The type of [`SystemSet`] this instance converts into.
    type Set: SystemSet;

    /// Converts this instance to its associated [`SystemSet`] type.
    fn into_system_set(self) -> Self::Set;
}

// systems sets
impl<S: SystemSet> IntoSystemSet<()> for S {
    type Set = Self;

    #[inline]
    fn into_system_set(self) -> Self::Set {
        self
    }
}

// systems
impl<Marker, F> IntoSystemSet<(IsFunctionSystem, Marker)> for F
where
    F: SystemParamFunction<Marker>,
{
    type Set = SystemTypeSet<Self>;

    #[inline]
    fn into_system_set(self) -> Self::Set {
        SystemTypeSet::new()
    }
}

// exclusive systems
impl<Marker, F> IntoSystemSet<(IsExclusiveFunctionSystem, Marker)> for F
where
    F: ExclusiveSystemParamFunction<Marker>,
{
    type Set = SystemTypeSet<Self>;

    #[inline]
    fn into_system_set(self) -> Self::Set {
        SystemTypeSet::new()
    }
}

#[cfg(test)]
mod tests {
    use crate::{
        schedule::{tests::ResMut, Schedule},
        system::Resource,

Review comment (suggestion):
This is _technically_ a breaking change, but `IntoSystem<Marker> for F` already requires `Marker: 'static` so I don't think any actual code would depend on a non-static marker.

Code after:
}

/// Types that can be converted into a [`SystemSet`].
pub trait IntoSystemSet<Marker>: Sized {
    /// The type of [`SystemSet`] this instance converts into.
    type Set: SystemSet;

    /// Converts this instance to its associated [`SystemSet`] type.
    fn into_system_set(self) -> Self::Set;
}

// systems sets
impl<S: SystemSet> IntoSystemSet<()> for S {
    type Set = Self;

    #[inline]
    fn into_system_set(self) -> Self::Set {
        self
    }
}

// systems
impl<Marker, F> IntoSystemSet<(IsFunctionSystem, Marker)> for F
where
    Marker: 'static,
    F: SystemParamFunction<Marker>,
{
    type Set = SystemTypeSet<FunctionSystem<Marker, F>>;

    #[inline]
    fn into_system_set(self) -> Self::Set {
        SystemTypeSet::<FunctionSystem<Marker, F>>::new()
    }
}

// exclusive systems
impl<Marker, F> IntoSystemSet<(IsExclusiveFunctionSystem, Marker)> for F
where
    Marker: 'static,
    F: ExclusiveSystemParamFunction<Marker>,
{
    type Set = SystemTypeSet<ExclusiveFunctionSystem<Marker, F>>;

    #[inline]
    fn into_system_set(self) -> Self::Set {
        SystemTypeSet::<ExclusiveFunctionSystem<Marker, F>>::new()
    }
}

#[cfg(test)]
mod tests {
Repository: bevyengine/bevy
File: crates/bevy_ui/src/widget/text.rs (Rust)

Code before:
///     },
///     TextColor(BLUE.into()),
/// ));
///
/// // With text justification.
/// world.spawn((
///     TextUi::new("hello world\nand bevy!"),
///     TextLayout::new_with_justify(JustifyText::Center)
/// ));
/// ```
#[derive(Component, Debug, Default, Clone, Deref, DerefMut, Reflect)]
#[reflect(Component, Default, Debug)]
#[require(
    TextLayout,
    TextFont,
    TextColor,
    TextNodeFlags,
    Node,
    Style, // TODO: Remove when Node uses required components.
    ContentSize, // TODO: Remove when Node uses required components.
    FocusPolicy, // TODO: Remove when Node uses required components.
    ZIndex, // TODO: Remove when Node uses required components.
    Visibility, // TODO: Remove when Node uses required components.
    Transform // TODO: Remove when Node uses required components.
)]
pub struct TextUi(pub String);

impl TextUi {
    /// Makes a new text component.
    pub fn new(text: impl Into<String>) -> Self {
        Self(text.into())
    }
}

impl TextRoot for TextUi {}

impl TextSpanAccess for TextUi {
    fn read_span(&self) -> &str {
        self.as_str()
    }
    fn write_span(&mut self) -> &mut String {
        &mut *self
    }
}

impl From<&str> for TextUi {
    fn from(value: &str) -> Self {
        Self(String::from(value))
    }
}


Review comment (suggestion):
It's good to have a consistant naming scheme, but I find this a bit ugly. I prefer just having `Text` refer to UI text, with the -`2d` suffix  on the text2d types.


Code after:
/// # let mut world = World::default();
/// #
/// // Basic usage.
/// world.spawn(Text::new("hello world!"));
///
/// // With non-default style.
/// world.spawn((
///     Text::new("hello world!"),
///     TextFont {
///         font: font_handle.clone().into(),
///         font_size: 60.0,
///         ..Default::default()
///     },
///     TextColor(BLUE.into()),
/// ));
///
/// // With text justification.
/// world.spawn((
///     Text::new("hello world\nand bevy!"),
///     TextLayout::new_with_justify(JustifyText::Center)
/// ));
/// ```
#[derive(Component, Debug, Default, Clone, Deref, DerefMut, Reflect)]
#[reflect(Component, Default, Debug)]
#[require(
    TextLayout,
    TextFont,
    TextColor,
    TextNodeFlags,
    Node,
    Style, // TODO: Remove when Node uses required components.
    ContentSize, // TODO: Remove when Node uses required components.
    FocusPolicy, // TODO: Remove when Node uses required components.
    ZIndex, // TODO: Remove when Node uses required components.
    Visibility, // TODO: Remove when Node uses required components.
    Transform // TODO: Remove when Node uses required components.
)]
pub struct Text(pub String);

impl Text {
    /// Makes a new text component.
    pub fn new(text: impl Into<String>) -> Self {
        Self(text.into())
    }
}

impl TextRoot for Text {}

impl TextSpanAccess for Text {
    fn read_span(&self) -> &str {
        self.as_str()
Repository: bevyengine/bevy
File: crates/bevy_audio/src/sinks.rs (Rust)

Code before:
    ///
    /// If the sink is not muted, this is `None`.
    ///
    /// If the sink is muted, this is `Some(volume)` where `volume` is the
    /// user's intended volume setting, even if the underlying sink's volume is
    /// 0.
    pub(crate) managed_volume: Option<f32>,
}

impl AudioSink {
    /// Create a new audio sink.
    pub fn new(sink: Sink) -> Self {
        Self {
            sink,
            managed_volume: None,
        }
    }
}

impl AudioSinkPlayback for AudioSink {
    fn volume(&self) -> f32 {
        if let Some(volume) = self.managed_volume {
            volume
        } else {
            self.sink.volume()
        }
    }

    fn set_volume(&mut self, volume: f32) {
        if self.is_muted() {
            self.managed_volume = Some(volume);
        } else {
            self.sink.set_volume(volume);
        }
    }

    fn speed(&self) -> f32 {
        self.sink.speed()
    }

    fn set_speed(&self, speed: f32) {
        self.sink.set_speed(speed);
    }

    fn play(&self) {
        self.sink.play();
    }

    fn pause(&self) {
        self.sink.pause();
    }

Review comment (suggestion):
```suggestion
        self.managed_volume.unwrap_or_else(|| self.sink.volume())
```

Code after:

    /// Managed volume allows the sink to be muted without losing the user's
    /// intended volume setting.
    ///
    /// This is used to restore the volume when [`unmute`](Self::unmute) is
    /// called.
    ///
    /// If the sink is not muted, this is `None`.
    ///
    /// If the sink is muted, this is `Some(volume)` where `volume` is the
    /// user's intended volume setting, even if the underlying sink's volume is
    /// 0.
    pub(crate) managed_volume: Option<f32>,
}

impl AudioSink {
    /// Create a new audio sink.
    pub fn new(sink: Sink) -> Self {
        Self {
            sink,
            managed_volume: None,
        }
    }
}

impl AudioSinkPlayback for AudioSink {
    fn volume(&self) -> f32 {
        self.managed_volume.unwrap_or_else(|| self.sink.volume())
    }

    fn set_volume(&mut self, volume: f32) {
        if self.is_muted() {
            self.managed_volume = Some(volume);
        } else {
            self.sink.set_volume(volume);
        }
    }

    fn speed(&self) -> f32 {
        self.sink.speed()
    }

    fn set_speed(&self, speed: f32) {
        self.sink.set_speed(speed);
    }

    fn play(&self) {
        self.sink.play();
    }

    fn pause(&self) {
Repository: bevyengine/bevy
File: crates/bevy_audio/src/sinks.rs (Rust)

Code before:
    fn is_paused(&self) -> bool {
        self.sink.is_paused()
    }

    fn stop(&self) {
        self.sink.stop();
    }

    fn empty(&self) -> bool {
        self.sink.empty()
    }

    fn is_muted(&self) -> bool {
        self.managed_volume.is_some()
    }

    fn mute(&mut self) {
        self.managed_volume = Some(self.volume());
        self.sink.set_volume(0.0);
    }

    fn unmute(&mut self) {
        if let Some(volume) = self.managed_volume {
            self.sink.set_volume(volume);
            self.managed_volume = None;
        }
    }
}

/// Used to control spatial audio during playback.
///
/// Bevy inserts this component onto your entities when it begins playing an audio source
/// that's configured to use spatial audio.
///
/// You can use this component to modify the playback settings while the audio is playing.
///
/// If this component is removed from an entity, and a [`AudioSource`][crate::AudioSource] is
/// attached to that entity, that [`AudioSource`][crate::AudioSource] will start playing. If
/// that source is unchanged, that translates to the audio restarting.
#[derive(Component)]
pub struct SpatialAudioSink {
    pub(crate) sink: SpatialSink,

    /// Managed volume allows the sink to be muted without losing the user's
    /// intended volume setting.
    ///
    /// This is used to restore the volume when [`unmute`](Self::unmute) is
    /// called.
    ///
    /// If the sink is not muted, this is `None`.
    ///

Review comment (suggestion):
```suggestion
        if let Some(volume) = self.managed_volume.take() {
            self.sink.set_volume(volume);
        }
```

Code after:
    }

    fn is_paused(&self) -> bool {
        self.sink.is_paused()
    }

    fn stop(&self) {
        self.sink.stop();
    }

    fn empty(&self) -> bool {
        self.sink.empty()
    }

    fn is_muted(&self) -> bool {
        self.managed_volume.is_some()
    }

    fn mute(&mut self) {
        self.managed_volume = Some(self.volume());
        self.sink.set_volume(0.0);
    }

    fn unmute(&mut self) {
        if let Some(volume) = self.managed_volume.take() {
            self.sink.set_volume(volume);
        }
    }
}

/// Used to control spatial audio during playback.
///
/// Bevy inserts this component onto your entities when it begins playing an audio source
/// that's configured to use spatial audio.
///
/// You can use this component to modify the playback settings while the audio is playing.
///
/// If this component is removed from an entity, and a [`AudioSource`][crate::AudioSource] is
/// attached to that entity, that [`AudioSource`][crate::AudioSource] will start playing. If
/// that source is unchanged, that translates to the audio restarting.
#[derive(Component)]
pub struct SpatialAudioSink {
    pub(crate) sink: SpatialSink,

    /// Managed volume allows the sink to be muted without losing the user's
    /// intended volume setting.
    ///
    /// This is used to restore the volume when [`unmute`](Self::unmute) is
    /// called.
    ///
    /// If the sink is not muted, this is `None`.
Repository: bevyengine/bevy
File: crates/bevy_audio/src/sinks.rs (Rust)

Code before:
    fn is_paused(&self) -> bool {
        self.sink.is_paused()
    }

    fn stop(&self) {
        self.sink.stop();
    }

    fn empty(&self) -> bool {
        self.sink.empty()
    }

    fn is_muted(&self) -> bool {
        self.managed_volume.is_some()
    }

    fn mute(&mut self) {
        self.managed_volume = Some(self.volume());
        self.sink.set_volume(0.0);
    }

    fn unmute(&mut self) {
        if let Some(volume) = self.managed_volume {
            self.sink.set_volume(volume);
            self.managed_volume = None;
        }
    }
}

impl SpatialAudioSink {
    /// Set the two ears position.
    pub fn set_ears_position(&self, left_position: Vec3, right_position: Vec3) {
        self.sink.set_left_ear_position(left_position.to_array());
        self.sink.set_right_ear_position(right_position.to_array());
    }

    /// Set the listener position, with an ear on each side separated by `gap`.
    pub fn set_listener_position(&self, position: Transform, gap: f32) {
        self.set_ears_position(
            position.translation + position.left() * gap / 2.0,
            position.translation + position.right() * gap / 2.0,
        );
    }

    /// Set the emitter position.
    pub fn set_emitter_position(&self, position: Vec3) {
        self.sink.set_emitter_position(position.to_array());
    }
}


Review comment (suggestion):
```suggestion
        if let Some(volume) = self.managed_volume.take() {
            self.sink.set_volume(volume);
        }
```

Code after:

    fn is_paused(&self) -> bool {
        self.sink.is_paused()
    }

    fn stop(&self) {
        self.sink.stop();
    }

    fn empty(&self) -> bool {
        self.sink.empty()
    }

    fn is_muted(&self) -> bool {
        self.managed_volume.is_some()
    }

    fn mute(&mut self) {
        self.managed_volume = Some(self.volume());
        self.sink.set_volume(0.0);
    }

    fn unmute(&mut self) {
        if let Some(volume) = self.managed_volume.take() {
            self.sink.set_volume(volume);
        }
    }
}

impl SpatialAudioSink {
    /// Set the two ears position.
    pub fn set_ears_position(&self, left_position: Vec3, right_position: Vec3) {
        self.sink.set_left_ear_position(left_position.to_array());
        self.sink.set_right_ear_position(right_position.to_array());
    }

    /// Set the listener position, with an ear on each side separated by `gap`.
    pub fn set_listener_position(&self, position: Transform, gap: f32) {
        self.set_ears_position(
            position.translation + position.left() * gap / 2.0,
            position.translation + position.right() * gap / 2.0,
        );
    }

    /// Set the emitter position.
    pub fn set_emitter_position(&self, position: Vec3) {
        self.sink.set_emitter_position(position.to_array());
    }
}

#[cfg(test)]
Repository: bevyengine/bevy
File: examples/2d/pixel_grid_snap.rs (Rust)

Code before:
        InGameCamera,
        PIXEL_PERFECT_LAYERS,
    ));

    // spawn the canvas
    commands.spawn((Sprite::from_image(image_handle), Canvas, HIGH_RES_LAYERS));

    // the "outer" camera renders whatever is on `HIGH_RES_LAYERS` to the screen.
    // here, the canvas and one of the sample sprites will be rendered by this camera
    commands.spawn((Camera2d, Msaa::Off, OuterCamera, HIGH_RES_LAYERS));
}

/// Rotates entities to demonstrate grid snapping.
fn rotate(time: Res<Time>, mut transforms: Query<&mut Transform, With<Rotate>>) {
    for mut transform in &mut transforms {
        let dt = time.delta_secs();
        transform.rotate_z(dt);
    }
}

/// Scales camera projection to fit the window (integer multiples only).
fn fit_canvas(
    mut resize_events: EventReader<WindowResized>,
    mut projection: Single<&mut OrthographicProjection, With<OuterCamera>>,
) {
    for event in resize_events.read() {
        let h_scale = event.width / RES_WIDTH as f32;
        let v_scale = event.height / RES_HEIGHT as f32;
        projection.scale = 1. / h_scale.min(v_scale).round();
    }
}


Review comment (suggestion):
This could be a lot nicer post @NthTensor's system Result work so we could just do `projection.orthographic()?`.

Code after:
        InGameCamera,
        PIXEL_PERFECT_LAYERS,
    ));

    // spawn the canvas
    commands.spawn((Sprite::from_image(image_handle), Canvas, HIGH_RES_LAYERS));

    // the "outer" camera renders whatever is on `HIGH_RES_LAYERS` to the screen.
    // here, the canvas and one of the sample sprites will be rendered by this camera
    commands.spawn((Camera2d, Msaa::Off, OuterCamera, HIGH_RES_LAYERS));
}

/// Rotates entities to demonstrate grid snapping.
fn rotate(time: Res<Time>, mut transforms: Query<&mut Transform, With<Rotate>>) {
    for mut transform in &mut transforms {
        let dt = time.delta_secs();
        transform.rotate_z(dt);
    }
}

/// Scales camera projection to fit the window (integer multiples only).
fn fit_canvas(
    mut resize_events: EventReader<WindowResized>,
    mut projection: Single<&mut Projection, With<OuterCamera>>,
) {
    let Projection::Orthographic(projection) = &mut **projection else {
        return;
    };
    for event in resize_events.read() {
        let h_scale = event.width / RES_WIDTH as f32;
        let v_scale = event.height / RES_HEIGHT as f32;
        projection.scale = 1. / h_scale.min(v_scale).round();
    }
}
Repository: bevyengine/bevy
File: crates/bevy_ui/src/widget/viewport.rs (Rust)

Code before:
    hover::HoverMap,
    pointer::{Location, PointerId, PointerInput},
    Pickable,
};
use bevy_platform_support::collections::HashSet;
use bevy_reflect::Reflect;
use bevy_render::{
    camera::{Camera, NormalizedRenderTarget},
    render_resource::Extent3d,
};
use bevy_transform::components::GlobalTransform;
use bevy_utils::default;
#[cfg(feature = "bevy_ui_picking_backend")]
use uuid::Uuid;

use crate::{ComputedNode, ImageNode, Node, PositionType, Val};

/// Component used to render a [`Camera::target`]  to a node.
///
/// # See Also
///
/// [`on_add_viewport`]
/// [`update_viewport_render_target_size`]
#[derive(Component, Debug, Clone, Copy, Reflect)]
#[reflect(Component, Debug)]
pub struct Viewport {
    /// The entity representing the [`Camera`] associated with this viewport.
    ///
    /// Note that removing the [`Viewport`] component will not despawn this entity.
    pub camera: Entity,
}

impl Viewport {
    /// Creates a new [`Viewport`] with a given `camera`.
    pub fn new(camera: Entity) -> Self {
        Self { camera }
    }
}

#[cfg(feature = "bevy_ui_picking_backend")]
/// Handles viewport picking logic.
///
/// Viewport entities that are being hovered or dragged will have all pointer inputs sent to them.
#[expect(
    clippy::too_many_arguments,
    reason = "System requires a lot of arguments"
)]
pub fn viewport_picking(
    mut commands: Commands,
    viewport_query: Query<(&Viewport, &PointerId, &Children)>,
    node_query: Query<(&ComputedNode, &ImageNode, &GlobalTransform)>,

Review comment (suggestion):
Name is confusing as there's already a `Viewport` type.
```suggestion
pub struct ViewportNode {
```


Code after:
use bevy_reflect::Reflect;
use bevy_render::{
    camera::{Camera, NormalizedRenderTarget},
    render_resource::Extent3d,
};
use bevy_transform::components::GlobalTransform;
use bevy_utils::default;
#[cfg(feature = "bevy_ui_picking_backend")]
use uuid::Uuid;

use crate::{ComputedNode, Node};

/// Component used to render a [`Camera::target`]  to a node.
///
/// # See Also
///
/// [`update_viewport_render_target_size`]
#[derive(Component, Debug, Clone, Copy, Reflect)]
#[reflect(Component, Debug)]
#[require(Node)]
#[cfg_attr(
    feature = "bevy_ui_picking_backend",
    require(PointerId::Custom(Uuid::new_v4()))
)]
pub struct ViewportNode {
    /// The entity representing the [`Camera`] associated with this viewport.
    ///
    /// Note that removing the [`ViewportNode`] component will not despawn this entity.
    pub camera: Entity,
}

impl ViewportNode {
    /// Creates a new [`ViewportNode`] with a given `camera`.
    pub fn new(camera: Entity) -> Self {
        Self { camera }
    }
}

#[cfg(feature = "bevy_ui_picking_backend")]
/// Handles viewport picking logic.
///
/// Viewport entities that are being hovered or dragged will have all pointer inputs sent to them.
pub fn viewport_picking(
    mut commands: Commands,
    mut viewport_query: Query<(
        Entity,
        &ViewportNode,
        &PointerId,
        &mut PointerLocation,
        &ComputedNode,
        &GlobalTransform,