ronantakizawa/codereview-bench huggingface dataset:
Repository: bevyengine/bevy
File: crates/bevy_pbr/src/decal/forward.rs (Rust)

Code before:
        }

        if let Some(label) = &mut descriptor.label {
            *label = format!("forward_decal_{label}").into();
        }

        Ok(())
    }
}

impl Default for ForwardDecalMaterialExt {
    fn default() -> Self {
        Self {
            depth_fade_factor: 8.0,
        }
    }
}

#[derive(Resource)]
struct ForwardDecalMesh(Handle<Mesh>);

fn forward_decal_set_mesh(mut world: DeferredWorld, HookContext { entity, .. }: HookContext) {
    let decal_mesh = world.resource::<ForwardDecalMesh>().0.clone();
    let mut entity = world.entity_mut(entity);
    let mut entity_mesh = entity.get_mut::<Mesh3d>().unwrap();
    // Only replace the mesh handle if the mesh handle is defaulted.
    if **entity_mesh == Handle::default() {
        entity_mesh.0 = decal_mesh;
    }
}


Review comment (suggestion):
This needs a comment about why we can't just use required components. I think I agree with your reasoning, but it needs to be recorded in the code base for future contributors to evaluate quickly if it's still valid.

Code after:
        }

        if let Some(label) = &mut descriptor.label {
            *label = format!("forward_decal_{label}").into();
        }

        Ok(())
    }
}

impl Default for ForwardDecalMaterialExt {
    fn default() -> Self {
        Self {
            depth_fade_factor: 8.0,
        }
    }
}

#[derive(Resource)]
struct ForwardDecalMesh(Handle<Mesh>);

// Note: We need to use a hook here instead of required components since we cannot access resources
// with required components, and we can't otherwise get a handle to the asset from a required
// component constructor, since the constructor must be a function pointer, and we intentionally do
// not want to use `uuid_handle!`.
fn forward_decal_set_mesh(mut world: DeferredWorld, HookContext { entity, .. }: HookContext) {
    let decal_mesh = world.resource::<ForwardDecalMesh>().0.clone();
    let mut entity = world.entity_mut(entity);
    let mut entity_mesh = entity.get_mut::<Mesh3d>().unwrap();
    // Only replace the mesh handle if the mesh handle is defaulted.
    if **entity_mesh == Handle::default() {
        entity_mesh.0 = decal_mesh;
    }
}
Repository: bevyengine/bevy
File: crates/bevy_ecs/src/relationship/mod.rs (Rust)

Code before:
        assert!(world.get_entity(parent).is_err());

        // Rel on Child
        // Despawn Child
        let mut commands = world.commands();
        let parent = commands.spawn_empty().id();
        let child = commands.spawn((ChildOf(parent), Rel(parent))).id();
        commands.entity(child).despawn();
        world.flush();

        assert!(world.get_entity(child).is_err());
        assert!(!world.entity(parent).contains::<RelTarget>());
    }

    // Spawn a batch of entities in relationship with a target entity
    #[test]
    fn spawn_batch_with_relationship() {
        use crate::relationship::{Relationship, RelationshipTarget};

        #[derive(Component)]
        #[relationship(relationship_target = RelTarget)]
        struct Rel(Entity);

        #[derive(Component)]
        #[relationship_target(relationship = Rel)]
        struct RelTarget(Vec<Entity>);

        let mut world = World::new();
        let target = world.spawn_empty().id();
        let rel_entities = world
            .spawn_batch((0..10).map(|_| Rel(target)))
            .collect::<Vec<_>>();

        for &entity in &rel_entities {
            assert!(world.get::<Rel>(entity).is_some_and(|r| r.get() == target));
        }
        assert!(world
            .get::<RelTarget>(target)
            .is_some_and(|rt| rt.len() == 10));
    }
}


Review comment (suggestion):
When these components basically are the same as the vanilla `ChildOf`/`Children`, I would use these instead.

Code after:
        // Despawn Child
        let mut commands = world.commands();
        let parent = commands.spawn_empty().id();
        let child = commands.spawn((ChildOf(parent), Rel(parent))).id();
        commands.entity(child).despawn();
        world.flush();

        assert!(world.get_entity(child).is_err());
        assert!(!world.entity(parent).contains::<RelTarget>());
    }

    #[test]
    fn spawn_batch_with_relationship() {
        let mut world = World::new();
        let parent = world.spawn_empty().id();
        let children = world
            .spawn_batch((0..10).map(|_| ChildOf(parent)))
            .collect::<Vec<_>>();

        for &child in &children {
            assert!(world
                .get::<ChildOf>(child)
                .is_some_and(|child_of| child_of.parent() == parent));
        }
        assert!(world
            .get::<Children>(parent)
            .is_some_and(|children| children.len() == 10));
    }

    #[test]
    fn insert_batch_with_relationship() {
        let mut world = World::new();
        let parent = world.spawn_empty().id();
        let child = world.spawn_empty().id();
        world.insert_batch([(child, ChildOf(parent))]);
        world.flush();

        assert!(world.get::<ChildOf>(child).is_some());
        assert!(world.get::<Children>(parent).is_some());
    }
}
Repository: bevyengine/bevy
File: crates/bevy_ecs/src/relationship/mod.rs (Rust)

Code before:
    // Spawn a batch of entities in relationship with a target entity
    #[test]
    fn spawn_batch_with_relationship() {
        use crate::relationship::{Relationship, RelationshipTarget};

        #[derive(Component)]
        #[relationship(relationship_target = RelTarget)]
        struct Rel(Entity);

        #[derive(Component)]
        #[relationship_target(relationship = Rel)]
        struct RelTarget(Vec<Entity>);

        let mut world = World::new();
        let target = world.spawn_empty().id();
        let rel_entities = world
            .spawn_batch((0..10).map(|_| Rel(target)))
            .collect::<Vec<_>>();

        for &entity in &rel_entities {
            assert!(world.get::<Rel>(entity).is_some_and(|r| r.get() == target));
        }
        assert!(world
            .get::<RelTarget>(target)
            .is_some_and(|rt| rt.len() == 10));
    }
}


Review comment (performance):
I am almost certain this also fixes the same issue with `insert_batch`:

```suggestion
    }

    #[test]
    fn insert_batch_with_relationship() {
        let mut world = World::new();
        let parent = world.spawn_empty().id();
        let child = world.spawn_empty().id();
        world.insert_batch([(child, ChildOf(parent))]);

        assert!(world.get::<ChildOf>(child).is_some());
        assert!(world.get::<Children>(parent).is_some());
    }
```

Code after:
        let parent = world.spawn_empty().id();
        let children = world
            .spawn_batch((0..10).map(|_| ChildOf(parent)))
            .collect::<Vec<_>>();

        for &child in &children {
            assert!(world
                .get::<ChildOf>(child)
                .is_some_and(|child_of| child_of.parent() == parent));
        }
        assert!(world
            .get::<Children>(parent)
            .is_some_and(|children| children.len() == 10));
    }

    #[test]
    fn insert_batch_with_relationship() {
        let mut world = World::new();
        let parent = world.spawn_empty().id();
        let child = world.spawn_empty().id();
        world.insert_batch([(child, ChildOf(parent))]);
        world.flush();

        assert!(world.get::<ChildOf>(child).is_some());
        assert!(world.get::<Children>(parent).is_some());
    }
}
Repository: bevyengine/bevy
File: crates/bevy_transform/src/commands.rs (Rust)

Code before:
            let parent_global = self.world_scope(|world| {
                world
                    .get_entity_mut(parent)
                    .ok()?
                    .add_child(child)
                    .get::<GlobalTransform>()
                    .copied()
            })?;
            let child_global = self.get::<GlobalTransform>()?;
            let new_child_local = child_global.reparented_to(&parent_global);
            // TODO: Can this just be `self.insert(new_child_local)`? (does it matter if self doesn't already have `Transform`?)
            let mut child_local = self.get_mut::<Transform>()?;
            *child_local = new_child_local;
            Some(())
        };
        update_transform();
        self
    }

    fn remove_parent_in_place(&mut self) -> &mut Self {
        self.remove::<ChildOf>();
        // FIXME: Replace this closure with a `try` block. See: https://github.com/rust-lang/rust/issues/31436.
        let mut update_transform = || {
            let global = self.get::<GlobalTransform>()?;
            let new_local = global.compute_transform();
            // TODO: Can this just be `self.insert(new_local)`? (does it matter if self doesn't already have `Transform`?)
            let mut local = self.get_mut::<Transform>()?;
            *local = new_local;
            Some(())
        };
        update_transform();
        self
    }
}


Review comment (suggestion):
```suggestion
```

The answer is no :) We should avoid adding `Transform` to children in surprising places.

Code after:
            let parent_global = self.world_scope(|world| {
                world
                    .get_entity_mut(parent)
                    .ok()?
                    .add_child(child)
                    .get::<GlobalTransform>()
                    .copied()
            })?;
            let child_global = self.get::<GlobalTransform>()?;
            let new_child_local = child_global.reparented_to(&parent_global);
            let mut child_local = self.get_mut::<Transform>()?;
            *child_local = new_child_local;
            Some(())
        };
        update_transform();
        self
    }

    fn remove_parent_in_place(&mut self) -> &mut Self {
        self.remove::<ChildOf>();
        // FIXME: Replace this closure with a `try` block. See: https://github.com/rust-lang/rust/issues/31436.
        let mut update_transform = || {
            let global = self.get::<GlobalTransform>()?;
            let new_local = global.compute_transform();
            let mut local = self.get_mut::<Transform>()?;
            *local = new_local;
            Some(())
        };
        update_transform();
        self
    }
}
Repository: bevyengine/bevy
File: crates/bevy_ecs/src/relationship/relationship_source_collection.rs (Rust)

Code before:
            None.into_iter()
        } else {
            Some(*self).into_iter()
        }
    }

    fn len(&self) -> usize {
        if *self == Entity::PLACEHOLDER {
            return 0;
        }
        1
    }

    fn clear(&mut self) {
        *self = Entity::PLACEHOLDER;
    }

    fn shrink_to_fit(&mut self) {}

    fn extend_from_iter(&mut self, entities: impl IntoIterator<Item = Entity>) {
        for entity in entities {
            *self = entity;
        }
    }
}

impl<const N: usize> OrderedRelationshipSourceCollection for SmallVec<[Entity; N]> {
    fn insert(&mut self, index: usize, entity: Entity) {
        self.push(entity);
        let len = self.len();
        if index < len {
            self.swap(index, len - 1);
        }
    }

    fn remove_at(&mut self, index: usize) -> Option<Entity> {
        (index < self.len()).then(|| self.swap_remove(index))
    }

    fn insert_stable(&mut self, index: usize, entity: Entity) {
        if index < self.len() {
            SmallVec::<[Entity; N]>::insert(self, index, entity);
        } else {
            self.push(entity);
        }
    }

    fn remove_at_stable(&mut self, index: usize) -> Option<Entity> {
        (index < self.len()).then(|| self.remove(index))
    }


Review comment (bug):
I think you can remove this method entirely, since the behavior is now the same as the default impl.  

... on the other hand, it looks like every other impl overrides this to do `.extend`.  The only exception is `BTreeSet`, and that seems like it might be a bug.  So maybe we should leave this here and do a follow-up to remove the default impl.

Code after:

        false
    }

    fn iter(&self) -> Self::SourceIter<'_> {
        if *self == Entity::PLACEHOLDER {
            None.into_iter()
        } else {
            Some(*self).into_iter()
        }
    }

    fn len(&self) -> usize {
        if *self == Entity::PLACEHOLDER {
            return 0;
        }
        1
    }

    fn clear(&mut self) {
        *self = Entity::PLACEHOLDER;
    }

    fn shrink_to_fit(&mut self) {}

    fn extend_from_iter(&mut self, entities: impl IntoIterator<Item = Entity>) {
        for entity in entities {
            *self = entity;
        }
    }

    fn source_to_remove_before_add(&self) -> Option<Entity> {
        if *self != Entity::PLACEHOLDER {
            Some(*self)
        } else {
            None
        }
    }
}

impl<const N: usize> OrderedRelationshipSourceCollection for SmallVec<[Entity; N]> {
    fn insert(&mut self, index: usize, entity: Entity) {
        self.push(entity);
        let len = self.len();
        if index < len {
            self.swap(index, len - 1);
        }
    }

    fn remove_at(&mut self, index: usize) -> Option<Entity> {
        (index < self.len()).then(|| self.swap_remove(index))
Repository: bevyengine/bevy
File: crates/bevy_feathers/src/alpha_pattern.rs (Rust)

Code before:
    fn fragment_shader() -> ShaderRef {
        "embedded://bevy_feathers/assets/shaders/alpha_pattern.wgsl".into()
    }
}

#[derive(Resource)]
pub(crate) struct AlphaPatternResource(pub(crate) Handle<AlphaPatternMaterial>);

impl FromWorld for AlphaPatternResource {
    fn from_world(world: &mut bevy_ecs::world::World) -> Self {
        let mut ui_materials = world
            .get_resource_mut::<Assets<AlphaPatternMaterial>>()
            .unwrap();
        Self(ui_materials.add(AlphaPatternMaterial::default()))
    }
}

/// Marker that tells us we want to fill in the [`MaterialNode`] with the alpha material.
#[derive(Component, Default, Clone)]
pub(crate) struct AlphaPattern;

/// Observer to fill in the material handle (since we don't have access to the materials asset
/// in the template)
fn on_add_alpha_pattern(
    ev: On<Add, AlphaPattern>,
    mut q_swatch: Query<&mut MaterialNode<AlphaPatternMaterial>>,
    r_material: Res<AlphaPatternResource>,
) {
    if let Ok(mut material) = q_swatch.get_mut(ev.target()) {
        material.0 = r_material.0.clone();
    }
}

/// Plugin which registers the systems for updating the button styles.
pub struct AlphaPatternPlugin;

impl Plugin for AlphaPatternPlugin {
    fn build(&self, app: &mut bevy_app::App) {
        app.add_observer(on_add_alpha_pattern);
    }
}


Review comment (suggestion):
@viridia sorry, not to be annoying, but is this intended? Or should it be just `q` or `q_material` or something?

 I am sorry, I am a non-native speaker, it just stands out. I may shut up at any point if you tell me to.

Super grateful for all your work related to UI... it's the sore spot in bevy and you do amazing stuff!

Code after:
    fn fragment_shader() -> ShaderRef {
        "embedded://bevy_feathers/assets/shaders/alpha_pattern.wgsl".into()
    }
}

#[derive(Resource)]
pub(crate) struct AlphaPatternResource(pub(crate) Handle<AlphaPatternMaterial>);

impl FromWorld for AlphaPatternResource {
    fn from_world(world: &mut bevy_ecs::world::World) -> Self {
        let mut ui_materials = world
            .get_resource_mut::<Assets<AlphaPatternMaterial>>()
            .unwrap();
        Self(ui_materials.add(AlphaPatternMaterial::default()))
    }
}

/// Marker that tells us we want to fill in the [`MaterialNode`] with the alpha material.
#[derive(Component, Default, Clone)]
pub(crate) struct AlphaPattern;

/// Observer to fill in the material handle (since we don't have access to the materials asset
/// in the template)
fn on_add_alpha_pattern(
    ev: On<Add, AlphaPattern>,
    mut q_material_node: Query<&mut MaterialNode<AlphaPatternMaterial>>,
    r_material: Res<AlphaPatternResource>,
) {
    if let Ok(mut material) = q_material_node.get_mut(ev.target()) {
        material.0 = r_material.0.clone();
    }
}

/// Plugin which registers the systems for updating the button styles.
pub struct AlphaPatternPlugin;

impl Plugin for AlphaPatternPlugin {
    fn build(&self, app: &mut bevy_app::App) {
        app.add_observer(on_add_alpha_pattern);
    }
}
Repository: bevyengine/bevy
File: crates/bevy_state/src/state/computed_states.rs (Rust)

Code before:

    #[derive(PartialEq, Eq, Hash, Debug, Clone)]
    struct TestComputedState;

    impl ComputedStates for TestComputedState {
        type SourceStates = TestState;

        fn compute(_: Self::SourceStates) -> Option<Self> {
            Some(TestComputedState)
        }
    }

    #[test]
    fn computed_states_are_state_scoped_by_default() {
        let mut app = App::new();
        app.add_plugins(StatesPlugin);
        app.insert_state(TestState);
        app.add_computed_state::<TestComputedState>();

        let world = app.world_mut();
        world.spawn((DespawnOnEnterState(TestComputedState), TestComponent));

        world.query::<&TestComponent>().single(world).unwrap();

        let world = app.world_mut();
        world.run_schedule(StateTransition);

        assert!(world.query::<&TestComponent>().single(world).is_err());
    }
}


Review comment (bug):
it's probably more robust to assert on `query.len()` since single could fail with too many too. Maybe also change the above single for consistency.

Code after:

    #[derive(PartialEq, Eq, Hash, Debug, Clone)]
    struct TestComputedState;

    impl ComputedStates for TestComputedState {
        type SourceStates = TestState;

        fn compute(_: Self::SourceStates) -> Option<Self> {
            Some(TestComputedState)
        }
    }

    #[test]
    fn computed_states_are_state_scoped_by_default() {
        let mut app = App::new();
        app.add_plugins(StatesPlugin);
        app.insert_state(TestState);
        app.add_computed_state::<TestComputedState>();

        let world = app.world_mut();

        world.spawn((DespawnOnEnterState(TestComputedState), TestComponent));

        assert!(world.query::<&TestComponent>().single(world).is_ok());
        world.run_schedule(StateTransition);
        assert_eq!(world.query::<&TestComponent>().iter(world).len(), 0);
    }
}
Repository: bevyengine/bevy
File: crates/bevy_render/src/lib.rs (Rust)

Code before:
//! # Useful Environment Variables
//!
//! `WGPU_DEBUG=1` enables debug labels in release builds\
//! `WGPU_VALIDATION=0` disables validation layers\
//! `WGPU_FORCE_FALLBACK_ADAPTER=1` attempts to force softare rendering\
//! `WGPU_ADAPTER_NAME`: select a specific adapter by name\
//! `WGPU_SETTINGS_PRIO=webgl2` uses webgl2 limits\
//! `WGPU_SETTINGS_PRIO=compatibility` uses webgpu limits\
//! `VERBOSE_SHADER_ERROR=1` prints shader defs and entrypoint shaders on wgsl compilation error

#![expect(missing_docs, reason = "Not all docs are written yet, see #3492.")]
#![expect(unsafe_code, reason = "Unsafe code is used to improve performance.")]
#![cfg_attr(
    any(docsrs, docsrs_dep),
    expect(
        internal_features,
        reason = "rustdoc_internals is needed for fake_variadic"
    )
)]
#![cfg_attr(any(docsrs, docsrs_dep), feature(doc_auto_cfg, rustdoc_internals))]
#![doc(
    html_logo_url = "https://bevy.org/assets/icon.png",
    html_favicon_url = "https://bevy.org/assets/icon.png"
)]

#[cfg(target_pointer_width = "16")]
compile_error!("bevy_render cannot compile for a 16-bit platform.");

extern crate alloc;
extern crate core;

Review comment (suggestion):
```suggestion
//! `WGPU_FORCE_FALLBACK_ADAPTER=1` attempts to force software rendering\
```

Code after:
//! # Useful Environment Variables
//!
//! Both `bevy_render` and `wgpu` have a number of environment variable options for changing the runtime behavior
//! of both crates. Many of these may be useful in development or release environments.
//!
//! - `WGPU_DEBUG=1` enables debug labels, which can be useful in release builds.
//! - `WGPU_VALIDATION=0` disables validation layers. This can help with particularly spammy errors.
//! - `WGPU_FORCE_FALLBACK_ADAPTER=1` attempts to force software rendering. This typically matches what is used in CI.
//! - `WGPU_ADAPTER_NAME` allows selecting a specific adapter by name.
//! - `WGPU_SETTINGS_PRIO=webgl2` uses webgl2 limits.
//! - `WGPU_SETTINGS_PRIO=compatibility` uses webgpu limits.
//! - `VERBOSE_SHADER_ERROR=1` prints more detailed information about WGSL compilation errors, such as shader defs and shader entrypoint.

#![expect(missing_docs, reason = "Not all docs are written yet, see #3492.")]
#![expect(unsafe_code, reason = "Unsafe code is used to improve performance.")]
#![cfg_attr(
    any(docsrs, docsrs_dep),
    expect(
        internal_features,
        reason = "rustdoc_internals is needed for fake_variadic"
    )
)]
#![cfg_attr(any(docsrs, docsrs_dep), feature(doc_auto_cfg, rustdoc_internals))]
#![doc(
    html_logo_url = "https://bevy.org/assets/icon.png",
    html_favicon_url = "https://bevy.org/assets/icon.png"
)]

#[cfg(target_pointer_width = "16")]
compile_error!("bevy_render cannot compile for a 16-bit platform.");
Repository: bevyengine/bevy
File: crates/bevy_render/src/lib.rs (Rust)

Code before:
//! # Useful Environment Variables
//! Both `bevy_render` and `wgpu` have a number of environment variable options for changing the runtime behavior
//! of both crates. Many of these may be useful in development or release environments. 
//!
//! -  `WGPU_DEBUG=1` enables debug labels in release builds
//! - `WGPU_VALIDATION=0` disables validation layers
//! - `WGPU_FORCE_FALLBACK_ADAPTER=1` attempts to force software rendering
//! - `WGPU_ADAPTER_NAME`: select a specific adapter by name
//! - `WGPU_SETTINGS_PRIO=webgl2` uses webgl2 limits
//! - `WGPU_SETTINGS_PRIO=compatibility` uses webgpu limits
//! - `VERBOSE_SHADER_ERROR=1` prints more detailed information about WGSL compilation errors

#![expect(missing_docs, reason = "Not all docs are written yet, see #3492.")]
#![expect(unsafe_code, reason = "Unsafe code is used to improve performance.")]
#![cfg_attr(
    any(docsrs, docsrs_dep),
    expect(
        internal_features,
        reason = "rustdoc_internals is needed for fake_variadic"
    )
)]
#![cfg_attr(any(docsrs, docsrs_dep), feature(doc_auto_cfg, rustdoc_internals))]
#![doc(
    html_logo_url = "https://bevy.org/assets/icon.png",
    html_favicon_url = "https://bevy.org/assets/icon.png"
)]

#[cfg(target_pointer_width = "16")]
compile_error!("bevy_render cannot compile for a 16-bit platform.");


Review comment (bug):
```suggestion
//! - `WGPU_DEBUG=1` enables debug labels in release builds
```

Code after:
//! # Useful Environment Variables
//!
//! Both `bevy_render` and `wgpu` have a number of environment variable options for changing the runtime behavior
//! of both crates. Many of these may be useful in development or release environments.
//!
//! - `WGPU_DEBUG=1` enables debug labels, which can be useful in release builds.
//! - `WGPU_VALIDATION=0` disables validation layers. This can help with particularly spammy errors.
//! - `WGPU_FORCE_FALLBACK_ADAPTER=1` attempts to force software rendering. This typically matches what is used in CI.
//! - `WGPU_ADAPTER_NAME` allows selecting a specific adapter by name.
//! - `WGPU_SETTINGS_PRIO=webgl2` uses webgl2 limits.
//! - `WGPU_SETTINGS_PRIO=compatibility` uses webgpu limits.
//! - `VERBOSE_SHADER_ERROR=1` prints more detailed information about WGSL compilation errors, such as shader defs and shader entrypoint.

#![expect(missing_docs, reason = "Not all docs are written yet, see #3492.")]
#![expect(unsafe_code, reason = "Unsafe code is used to improve performance.")]
#![cfg_attr(
    any(docsrs, docsrs_dep),
    expect(
        internal_features,
        reason = "rustdoc_internals is needed for fake_variadic"
    )
)]
#![cfg_attr(any(docsrs, docsrs_dep), feature(doc_auto_cfg, rustdoc_internals))]
#![doc(
    html_logo_url = "https://bevy.org/assets/icon.png",
    html_favicon_url = "https://bevy.org/assets/icon.png"
)]

#[cfg(target_pointer_width = "16")]
compile_error!("bevy_render cannot compile for a 16-bit platform.");