ronantakizawa/codereview-bench huggingface dataset:
Repository: bevyengine/bevy
File: examples/stress_tests/many_text2d.rs (Rust)

Code before:
    }

    if args.no_frustum_culling {
        let bundles = text2ds.into_iter().map(|bundle| (bundle, NoFrustumCulling));
        commands.spawn_batch(bundles);
    } else {
        commands.spawn_batch(text2ds);
    }
}

// System for rotating and translating the camera
fn move_camera(time: Res<Time>, mut camera_query: Query<&mut Transform, With<Camera>>) {
    let Ok(mut camera_transform) = camera_query.single_mut() else {
        return;
    };
    camera_transform.rotate_z(time.delta_secs() * 0.5);
    *camera_transform =
        *camera_transform * Transform::from_translation(Vec3::X * CAMERA_SPEED * time.delta_secs());
}

// System for printing the number of texts on every tick of the timer
fn print_counts(
    time: Res<Time>,
    mut timer: Local<PrintingTimer>,
    texts: Query<&ViewVisibility, With<Text2d>>,
    atlases: Res<FontAtlasSet>,
    font: Res<FontHandle>,
) {
    timer.tick(time.delta());
    if !timer.just_finished() {
        return;
    }

    let font_id = font.0.id();
    let num_atlases = atlases
        .iter()
        .filter(|(key, _)| key.0 == font_id)
        .map(|(_, atlases)| atlases.len())
        .sum::<usize>();

    let visible_texts = texts.iter().filter(|visibility| visibility.get()).count();

    info!(
        "Texts: {} Visible: {} Atlases: {}",
        texts.iter().count(),
        visible_texts,
        num_atlases
    );
}

fn random_text_font(rng: &mut ChaCha8Rng, args: &Args, font: Handle<Font>) -> TextFont {

Review comment (suggestion):
```suggestion
    font_atlas_set: Res<FontAtlasSet>,
```

Code after:
    }

    if args.no_frustum_culling {
        let bundles = text2ds.into_iter().map(|bundle| (bundle, NoFrustumCulling));
        commands.spawn_batch(bundles);
    } else {
        commands.spawn_batch(text2ds);
    }
}

// System for rotating and translating the camera
fn move_camera(time: Res<Time>, mut camera_query: Query<&mut Transform, With<Camera>>) {
    let Ok(mut camera_transform) = camera_query.single_mut() else {
        return;
    };
    camera_transform.rotate_z(time.delta_secs() * 0.5);
    *camera_transform =
        *camera_transform * Transform::from_translation(Vec3::X * CAMERA_SPEED * time.delta_secs());
}

// System for printing the number of texts on every tick of the timer
fn print_counts(
    time: Res<Time>,
    mut timer: Local<PrintingTimer>,
    texts: Query<&ViewVisibility, With<Text2d>>,
    font_atlas_set: Res<FontAtlasSet>,
    font: Res<FontHandle>,
) {
    timer.tick(time.delta());
    if !timer.just_finished() {
        return;
    }

    let font_id = font.0.id();
    let num_atlases = font_atlas_set
        .iter()
        .filter(|(key, _)| key.0 == font_id)
        .map(|(_, atlases)| atlases.len())
        .sum::<usize>();

    let visible_texts = texts.iter().filter(|visibility| visibility.get()).count();

    info!(
        "Texts: {} Visible: {} Atlases: {}",
        texts.iter().count(),
        visible_texts,
        num_atlases
    );
}

fn random_text_font(rng: &mut ChaCha8Rng, args: &Args, font: Handle<Font>) -> TextFont {
Repository: bevyengine/bevy
File: examples/ui/font_atlas_debug.rs (Rust)

Code before:
}

#[derive(Resource)]
struct State {
    atlas_count: u32,
    handle: Handle<Font>,
    timer: Timer,
}

impl Default for State {
    fn default() -> Self {
        Self {
            atlas_count: 0,
            handle: Handle::default(),
            timer: Timer::from_seconds(0.05, TimerMode::Repeating),
        }
    }
}

#[derive(Resource, Deref, DerefMut)]
struct SeededRng(ChaCha8Rng);

fn atlas_render_system(
    mut commands: Commands,
    mut state: ResMut<State>,
    font_atlas_sets: Res<FontAtlasSet>,
    images: Res<Assets<Image>>,
) {
    if let Some((_, font_atlases)) = font_atlas_sets.iter().next() {
        let x_offset = state.atlas_count as f32;
        if state.atlas_count == font_atlases.len() as u32 {
            return;
        }
        let font_atlas = &font_atlases[state.atlas_count as usize];
        let image = images.get(&font_atlas.texture).unwrap();
        state.atlas_count += 1;
        commands.spawn((
            ImageNode::new(font_atlas.texture.clone()),
            Node {
                position_type: PositionType::Absolute,
                top: Val::ZERO,
                left: px(image.width() as f32 * x_offset),
                ..default()
            },
        ));
    }
}

fn text_update_system(
    mut state: ResMut<State>,
    time: Res<Time>,

Review comment (suggestion):
```suggestion
    font_atlas_set: Res<FontAtlasSet>,
```

Code after:
}

#[derive(Resource)]
struct State {
    atlas_count: u32,
    handle: Handle<Font>,
    timer: Timer,
}

impl Default for State {
    fn default() -> Self {
        Self {
            atlas_count: 0,
            handle: Handle::default(),
            timer: Timer::from_seconds(0.05, TimerMode::Repeating),
        }
    }
}

#[derive(Resource, Deref, DerefMut)]
struct SeededRng(ChaCha8Rng);

fn atlas_render_system(
    mut commands: Commands,
    mut state: ResMut<State>,
    font_atlas_set: Res<FontAtlasSet>,
    images: Res<Assets<Image>>,
) {
    if let Some(font_atlases) = font_atlas_set.values().next() {
        let x_offset = state.atlas_count as f32;
        if state.atlas_count == font_atlases.len() as u32 {
            return;
        }
        let font_atlas = &font_atlases[state.atlas_count as usize];
        let image = images.get(&font_atlas.texture).unwrap();
        state.atlas_count += 1;
        commands.spawn((
            ImageNode::new(font_atlas.texture.clone()),
            Node {
                position_type: PositionType::Absolute,
                top: Val::ZERO,
                left: px(image.width() as f32 * x_offset),
                ..default()
            },
        ));
    }
}

fn text_update_system(
    mut state: ResMut<State>,
    time: Res<Time>,
Repository: bevyengine/bevy
File: crates/bevy_text/src/font_atlas_set.rs (Rust)

Code before:
#[derive(Debug, Hash, PartialEq, Eq)]
pub struct FontAtlasKey(pub AssetId<Font>, pub u32, pub FontSmoothing);

impl From<&TextFont> for FontAtlasKey {
    fn from(font: &TextFont) -> Self {
        FontAtlasKey(
            font.font.id(),
            font.font_size.to_bits(),
            font.font_smoothing,
        )
    }
}

/// Set of rasterized fonts stored in [`FontAtlas`]es.
#[derive(Debug, Default, Resource, Deref, DerefMut)]
pub struct FontAtlasSet(HashMap<FontAtlasKey, Vec<FontAtlas>>);

impl FontAtlasSet {
    /// Checks if the given subpixel-offset glyph is contained in any of the [`FontAtlas`]es for the font identified by the given [`FontAtlasKey`].
    pub fn has_glyph(&self, cache_key: cosmic_text::CacheKey, font_key: &FontAtlasKey) -> bool {
        self.get(font_key)
            .is_some_and(|font_atlas| font_atlas.iter().any(|atlas| atlas.has_glyph(cache_key)))
    }
}

/// System that automatically frees the unused texture atlases when a font asset is removed.
pub fn free_unused_font_atlases_system(
    mut font_atlas_sets: ResMut<FontAtlasSet>,
    mut font_events: MessageReader<AssetEvent<Font>>,
) {
    for event in font_events.read() {
        if let AssetEvent::Removed { id } = event {
            font_atlas_sets.retain(|key, _| key.0 != *id);
        }
    }
}


Review comment (suggestion):
```suggestion
/// A system that automatically frees unused texture atlases when a font asset is removed.
```

Code after:
#[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
pub struct FontAtlasKey(pub AssetId<Font>, pub u32, pub FontSmoothing);

impl From<&TextFont> for FontAtlasKey {
    fn from(font: &TextFont) -> Self {
        FontAtlasKey(
            font.font.id(),
            font.font_size.to_bits(),
            font.font_smoothing,
        )
    }
}

/// Set of rasterized fonts stored in [`FontAtlas`]es.
#[derive(Debug, Default, Resource, Deref, DerefMut)]
pub struct FontAtlasSet(HashMap<FontAtlasKey, Vec<FontAtlas>>);

impl FontAtlasSet {
    /// Checks whether the given subpixel-offset glyph is contained in any of the [`FontAtlas`]es for the font identified by the given [`FontAtlasKey`].
    pub fn has_glyph(&self, cache_key: cosmic_text::CacheKey, font_key: &FontAtlasKey) -> bool {
        self.get(font_key)
            .is_some_and(|font_atlas| font_atlas.iter().any(|atlas| atlas.has_glyph(cache_key)))
    }
}

/// A system that automatically frees unused texture atlases when a font asset is removed.
pub fn free_unused_font_atlases_system(
    mut font_atlas_sets: ResMut<FontAtlasSet>,
    mut font_events: MessageReader<AssetEvent<Font>>,
) {
    for event in font_events.read() {
        if let AssetEvent::Removed { id } = event {
            font_atlas_sets.retain(|key, _| key.0 != *id);
        }
    }
}
Repository: bevyengine/bevy
File: examples/picking/sprite_picking.rs (Rust)

Code before:
    commands
        .spawn((
            Sprite::from_atlas_image(
                texture_handle,
                TextureAtlas {
                    layout: texture_atlas_layout_handle,
                    index: animation_indices.first,
                },
            ),
            Transform::from_xyz(300.0, 0.0, 0.0).with_scale(Vec3::splat(6.0)),
            animation_indices,
            AnimationTimer(Timer::from_seconds(0.1, TimerMode::Repeating)),
            Pickable::default(),
        ))
        .observe(recolor_on::<Pointer<Over>>(Color::srgb(0.0, 1.0, 1.0)))
        .observe(recolor_on::<Pointer<Out>>(Color::srgb(1.0, 1.0, 1.0)))
        .observe(recolor_on::<Pointer<Press>>(Color::srgb(1.0, 1.0, 0.0)))
        .observe(recolor_on::<Pointer<Release>>(Color::srgb(0.0, 1.0, 1.0)));
}

// An observer that changes the target entity's color.
fn recolor_on<E: EntityEvent + Debug + Clone + Reflect>(
    color: Color,
) -> impl Fn(On<E>, Query<&mut Sprite>) {
    move |ev, mut sprites| {
        let Ok(mut sprite) = sprites.get_mut(ev.event_target().entity()) else {
            return;
        };
        sprite.color = color;
    }
}


Review comment (suggestion):
This is a pretty large tradeoff to make. This makes generic EntityEvent code much nastier to write. I'd prefer this logic to be internalized to the `EntityEvent` implementation. Ex:

```rust
impl EntityEvent for MyEvent {
  fn event_target(&self) -> Entity {
    self.instance.entity()
  }
}
```

I'd be open to us adding `ContainsEntity` support to the EntityEvent derive.

Code after:
    commands
        .spawn((
            Sprite::from_atlas_image(
                texture_handle,
                TextureAtlas {
                    layout: texture_atlas_layout_handle,
                    index: animation_indices.first,
                },
            ),
            Transform::from_xyz(300.0, 0.0, 0.0).with_scale(Vec3::splat(6.0)),
            animation_indices,
            AnimationTimer(Timer::from_seconds(0.1, TimerMode::Repeating)),
            Pickable::default(),
        ))
        .observe(recolor_on::<Pointer<Over>>(Color::srgb(0.0, 1.0, 1.0)))
        .observe(recolor_on::<Pointer<Out>>(Color::srgb(1.0, 1.0, 1.0)))
        .observe(recolor_on::<Pointer<Press>>(Color::srgb(1.0, 1.0, 0.0)))
        .observe(recolor_on::<Pointer<Release>>(Color::srgb(0.0, 1.0, 1.0)));
}

// An observer that changes the target entity's color.
fn recolor_on<E: EntityEvent + Debug + Clone + Reflect>(
    color: Color,
) -> impl Fn(On<E>, Query<&mut Sprite>) {
    move |ev, mut sprites| {
        let Ok(mut sprite) = sprites.get_mut(ev.event_target()) else {
            return;
        };
        sprite.color = color;
    }
}
Repository: bevyengine/bevy
File: examples/2d/rotation.rs (Rust)

Code before:
//! Demonstrates rotating entities in 2D using quaternions.

use bevy::{log::tracing_subscriber::field::MakeExt as _, math::ops, prelude::*};

const BOUNDS: Vec2 = Vec2::new(1200.0, 640.0);

fn main() {
    App::new()
        .add_plugins(DefaultPlugins.set(bevy::log::LogPlugin {
            fmt_layer: |_| {
                Some(Box::new(
                    bevy::log::tracing_subscriber::fmt::Layer::default()
                        .map_fmt_fields(|f| f.debug_alt())
                        .with_writer(std::io::stderr),
                ))
            },
            ..default()
        }))
        .insert_resource(Time::<Fixed>::from_hz(60.0))
        .add_systems(Startup, setup)
        .add_systems(
            FixedUpdate,
            (
                player_movement_system,
                snap_to_player_system,
                rotate_to_player_system,
            ),
        )
        .run();
}

/// Player component
#[derive(Component)]
struct Player {
    /// Linear speed in meters per second

Review comment (suggestion):
We should change the log_layers example instead if we're not turning this on by default: https://github.com/bevyengine/bevy/blob/c392d28d30f34b7d2b69b806136b9643379f9284/examples/app/log_layers.rs#L45-L49

Code after:
//! Demonstrates rotating entities in 2D using quaternions.

use bevy::{math::ops, prelude::*};

const BOUNDS: Vec2 = Vec2::new(1200.0, 640.0);

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .insert_resource(Time::<Fixed>::from_hz(60.0))
        .add_systems(Startup, setup)
        .add_systems(
            FixedUpdate,
            (
                player_movement_system,
                snap_to_player_system,
                rotate_to_player_system,
            ),
        )
        .run();
}

/// Player component
#[derive(Component)]
struct Player {
    /// Linear speed in meters per second
    movement_speed: f32,
    /// Rotation speed in radians per second
    rotation_speed: f32,
}

/// Snap to player ship behavior
#[derive(Component)]
struct SnapToPlayer;
Repository: bevyengine/bevy
File: crates/bevy_ecs/src/system/system_param.rs (Rust)

Code before:
impl<'s, 'a, T: FromWorld + Send + 'static> IntoIterator for &'a Local<'s, T>
where
    &'a T: IntoIterator,
{
    type Item = <&'a T as IntoIterator>::Item;
    type IntoIter = <&'a T as IntoIterator>::IntoIter;

    fn into_iter(self) -> Self::IntoIter {
        self.0.into_iter()
    }
}

impl<'s, 'a, T: FromWorld + Send + 'static> IntoIterator for &'a mut Local<'s, T>
where
    &'a mut T: IntoIterator,
{
    type Item = <&'a mut T as IntoIterator>::Item;
    type IntoIter = <&'a mut T as IntoIterator>::IntoIter;

    fn into_iter(self) -> Self::IntoIter {
        self.0.into_iter()
    }
}

// SAFETY: only local state is accessed
/// System private persistent value
unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> {
    type State = SyncCell<T>;
    type Item<'w, 's> = Local<'s, T>;

    fn init_state(world: &mut World) -> Self::State {
        SyncCell::new(T::from_world(world))
    }

    fn init_access(
        _state: &Self::State,
        _system_meta: &mut SystemMeta,
        _component_access_set: &mut FilteredAccessSet,
        _world: &mut World,
    ) {
    }

    #[inline]
    unsafe fn get_param<'w, 's>(
        state: &'s mut Self::State,
        _system_meta: &SystemMeta,
        _world: UnsafeWorldCell<'w>,
        _change_tick: Tick,
    ) -> Self::Item<'w, 's> {
        Local(state.get())
    }

Review comment (suggestion):
```suggestion
```
I'm not convinced that this docs note on the impl block is helpful.

Code after:
    }
}

impl<'s, T: FromWorld + Send + 'static> DerefMut for Local<'s, T> {
    #[inline]
    fn deref_mut(&mut self) -> &mut Self::Target {
        self.0
    }
}

impl<'s, 'a, T: FromWorld + Send + 'static> IntoIterator for &'a Local<'s, T>
where
    &'a T: IntoIterator,
{
    type Item = <&'a T as IntoIterator>::Item;
    type IntoIter = <&'a T as IntoIterator>::IntoIter;

    fn into_iter(self) -> Self::IntoIter {
        self.0.into_iter()
    }
}

impl<'s, 'a, T: FromWorld + Send + 'static> IntoIterator for &'a mut Local<'s, T>
where
    &'a mut T: IntoIterator,
{
    type Item = <&'a mut T as IntoIterator>::Item;
    type IntoIter = <&'a mut T as IntoIterator>::IntoIter;

    fn into_iter(self) -> Self::IntoIter {
        self.0.into_iter()
    }
}

// SAFETY: only local state is accessed
unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> {
    type State = SyncCell<T>;
    type Item<'w, 's> = Local<'s, T>;

    fn init_state(world: &mut World) -> Self::State {
        SyncCell::new(T::from_world(world))
    }

    fn init_access(
        _state: &Self::State,
        _system_meta: &mut SystemMeta,
        _component_access_set: &mut FilteredAccessSet,
        _world: &mut World,
    ) {
    }
Repository: bevyengine/bevy
File: crates/bevy_asset/src/io/web.rs (Rust)

Code before:
#[cfg(feature = "http")]
use crate::io::AssetSourceBuilder;
use crate::io::PathStream;
use crate::io::{AssetReader, AssetReaderError, Reader};
use crate::{AssetApp, AssetPlugin};
use alloc::boxed::Box;
use bevy_app::{App, Plugin};
use bevy_tasks::ConditionalSendFuture;
use std::path::{Path, PathBuf};
use tracing::warn;

/// Adds the `http` and `https` asset sources to the app.
///
/// NOTE: Make sure to add this plugin *before* `AssetPlugin` to properly register http asset sources.
///
/// WARNING: be careful about where your URLs are coming from! URLs can potentially be exploited by an
/// attacker to trigger vulnerabilities in our asset loaders, or DOS by downloading enormous files. We
/// are not aware of any such vulnerabilities at the moment, just be careful!
///
/// Any asset path that begins with `http` (when the `http` feature is enabled) or `https` (when the
/// `https` feature is enabled) will be loaded from the web via `fetch` (wasm) or `ureq` (native).
///
/// Example usage:
///
/// ```rust
/// # use bevy_app::{App, Startup};

Review comment (suggestion):
```suggestion
#[cfg(any(feature = "http", feature = "https"))]
```

Fixes compile error when only "https" is enabled.

Code after:
#[cfg(any(feature = "http", feature = "https"))]
use crate::io::AssetSourceBuilder;
use crate::io::PathStream;
use crate::io::{AssetReader, AssetReaderError, Reader};
use crate::{AssetApp, AssetPlugin};
use alloc::boxed::Box;
use bevy_app::{App, Plugin};
use bevy_tasks::ConditionalSendFuture;
use std::path::{Path, PathBuf};
use tracing::warn;

/// Adds the `http` and `https` asset sources to the app.
///
/// NOTE: Make sure to add this plugin *before* `AssetPlugin` to properly register http asset sources.
///
/// WARNING: be careful about where your URLs are coming from! URLs can potentially be exploited by an
/// attacker to trigger vulnerabilities in our asset loaders, or DOS by downloading enormous files. We
/// are not aware of any such vulnerabilities at the moment, just be careful!
///
/// Any asset path that begins with `http` (when the `http` feature is enabled) or `https` (when the
/// `https` feature is enabled) will be loaded from the web via `fetch` (wasm) or `ureq` (native).
///
/// Example usage:
///
/// ```rust
/// # use bevy_app::{App, Startup};
Repository: bevyengine/bevy
File: crates/bevy_ecs/src/system/system_param.rs (Rust)

Code before:
        _component_access_set: &mut FilteredAccessSet,
        _world: &mut World,
    ) {
        system_meta.set_has_deferred();
    }

    fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) {
        state.get().apply(system_meta, world);
    }

    fn queue(state: &mut Self::State, system_meta: &SystemMeta, world: DeferredWorld) {
        state.get().queue(system_meta, world);
    }

    #[inline]
    unsafe fn get_param<'w, 's>(
        state: &'s mut Self::State,
        _system_meta: &SystemMeta,
        _world: UnsafeWorldCell<'w>,
        _change_tick: Tick,
    ) -> Self::Item<'w, 's> {
        Deferred(state.get())
    }
}

/// A dummy type to tell the executor run the system exclusively.
pub struct ExclusiveMarker(PhantomData<()>);

// SAFETY: No world access.
unsafe impl SystemParam for ExclusiveMarker {
    type State = ();
    type Item<'w, 's> = Self;

    #[inline]
    fn init_state(_world: &mut World) -> Self::State {}

    fn init_access(
        _state: &Self::State,
        system_meta: &mut SystemMeta,
        _component_access_set: &mut FilteredAccessSet,
        _world: &mut World,
    ) {
        system_meta.set_exclusive();
    }

    #[inline]
    unsafe fn get_param<'world, 'state>(
        _state: &'state mut Self::State,
        _system_meta: &SystemMeta,
        _world: UnsafeWorldCell<'world>,
        _change_tick: Tick,

Review comment (suggestion):
```suggestion
/// A dummy type to tell the executor to run the system exclusively.
```
nit

Code after:
        _component_access_set: &mut FilteredAccessSet,
        _world: &mut World,
    ) {
        system_meta.set_has_deferred();
    }

    fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) {
        state.get().apply(system_meta, world);
    }

    fn queue(state: &mut Self::State, system_meta: &SystemMeta, world: DeferredWorld) {
        state.get().queue(system_meta, world);
    }

    #[inline]
    unsafe fn get_param<'w, 's>(
        state: &'s mut Self::State,
        _system_meta: &SystemMeta,
        _world: UnsafeWorldCell<'w>,
        _change_tick: Tick,
    ) -> Self::Item<'w, 's> {
        Deferred(state.get())
    }
}

/// A dummy type to tell the executor to run the system exclusively.
pub struct ExclusiveMarker(PhantomData<()>);

// SAFETY: No world access.
unsafe impl SystemParam for ExclusiveMarker {
    type State = ();
    type Item<'w, 's> = Self;

    #[inline]
    fn init_state(_world: &mut World) -> Self::State {}

    fn init_access(
        _state: &Self::State,
        system_meta: &mut SystemMeta,
        _component_access_set: &mut FilteredAccessSet,
        _world: &mut World,
    ) {
        system_meta.set_exclusive();
    }

    #[inline]
    unsafe fn get_param<'world, 'state>(
        _state: &'state mut Self::State,
        _system_meta: &SystemMeta,
        _world: UnsafeWorldCell<'world>,
        _change_tick: Tick,