Simplify sort/max_by calls (#17048)

# Objective

Some sort calls and `Ord` impls are unnecessarily complex.

## Solution

Rewrite the "match on cmp, if equal do another cmp" as either a
comparison on tuples, or `Ordering::then_with`, depending on whether the
compare keys need construction.

`sort_by` -> `sort_by_key` when symmetrical. Do the same for
`min_by`/`max_by`.

Note that `total_cmp` can only work with `sort_by`, and not on tuples.

When sorting collected query results that contain
`Entity`/`MainEntity`/`RenderEntity` in their `QueryData`, with that
`Entity` in the sort key:
stable -> unstable sort (all queried entities are unique)

If key construction is not simple, switch to `sort_by_cached_key` when
possible.

Sorts that are only performed to discover the maximal element are
replaced by `max_by_key`.

Dedicated comparison functions and structs are removed where simple.

Derive `PartialOrd`/`Ord` when useful.

Misc. closure style inconsistencies.

## Testing
- Existing tests.

crates/bevy_animation/src/animatable.rs

@@ -125,7 +125,7 @@ impl Animatable for bool {
     #[inline]
     fn blend(inputs: impl Iterator<Item = BlendInput<Self>>) -> Self {
         inputs
-            .max_by(|a, b| FloatOrd(a.weight).cmp(&FloatOrd(b.weight)))
+            .max_by_key(|x| FloatOrd(x.weight))
             .map(|input| input.value)
             .unwrap_or(false)
     }

crates/bevy_ecs/src/schedule/graph/node.rs

@@ -3,7 +3,7 @@ use core::fmt::Debug;
 /// Unique identifier for a system or system set stored in a [`ScheduleGraph`].
 ///
 /// [`ScheduleGraph`]: crate::schedule::ScheduleGraph
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
 pub enum NodeId {
     /// Identifier for a system.
     System(usize),
@@ -11,18 +11,6 @@ pub enum NodeId {
     Set(usize),
 }
 
-impl PartialOrd for NodeId {
-    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
-        Some(Ord::cmp(self, other))
-    }
-}
-
-impl Ord for NodeId {
-    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
-        self.cmp(other)
-    }
-}
-
 impl NodeId {
     /// Returns the internal integer value.
     pub(crate) const fn index(&self) -> usize {

crates/bevy_input_focus/src/tab_navigation.rs

@@ -45,7 +45,7 @@ use crate::{FocusedInput, InputFocus, InputFocusVisible};
 ///
 /// Note that you must also add the [`TabGroup`] component to the entity's ancestor in order
 /// for this component to have any effect.
-#[derive(Debug, Default, Component, Copy, Clone)]
+#[derive(Debug, Default, Component, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub struct TabIndex(pub i32);
 
 /// A component used to mark a tree of entities as containing tabbable elements.
@@ -179,7 +179,7 @@ impl TabNavigation<'_, '_> {
                     .map(|(e, tg, _)| (e, *tg))
                     .collect();
                 // Stable sort by group order
-                tab_groups.sort_by(compare_tab_groups);
+                tab_groups.sort_by_key(|(_, tg)| tg.order);
 
                 // Search group descendants
                 tab_groups.iter().for_each(|(tg_entity, _)| {
@@ -194,7 +194,7 @@ impl TabNavigation<'_, '_> {
         }
 
         // Stable sort by tabindex
-        focusable.sort_by(compare_tab_indices);
+        focusable.sort_by_key(|(_, idx)| *idx);
 
         let index = focusable.iter().position(|e| Some(e.0) == focus.0);
         let count = focusable.len();
@@ -233,15 +233,6 @@ impl TabNavigation<'_, '_> {
     }
 }
 
-fn compare_tab_groups(a: &(Entity, TabGroup), b: &(Entity, TabGroup)) -> core::cmp::Ordering {
-    a.1.order.cmp(&b.1.order)
-}
-
-// Stable sort which compares by tab index
-fn compare_tab_indices(a: &(Entity, TabIndex), b: &(Entity, TabIndex)) -> core::cmp::Ordering {
-    a.1 .0.cmp(&b.1 .0)
-}
-
 /// Plugin for navigating between focusable entities using keyboard input.
 pub struct TabNavigationPlugin;
 

crates/bevy_math/src/primitives/polygon.rs

@@ -52,10 +52,7 @@ impl Ord for SweepLineEvent {
 
 /// Orders 2D points according to the order expected by the sweep line and event queue from -X to +X and then -Y to Y.
 fn xy_order(a: Vec2, b: Vec2) -> Ordering {
-    match a.x.total_cmp(&b.x) {
+    a.x.total_cmp(&b.x).then_with(|| a.y.total_cmp(&b.y))
-        Ordering::Equal => a.y.total_cmp(&b.y),
-        ord => ord,
-    }
 }
 
 /// The event queue holds an ordered list of all events the [`SweepLine`] will encounter when checking the current polygon.
@@ -121,6 +118,7 @@ impl PartialEq for Segment {
     }
 }
 impl Eq for Segment {}
+
 impl PartialOrd for Segment {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         Some(self.cmp(other))
@@ -128,10 +126,10 @@ impl PartialOrd for Segment {
 }
 impl Ord for Segment {
     fn cmp(&self, other: &Self) -> Ordering {
-        match self.left.y.total_cmp(&other.left.y) {
+        self.left
-            Ordering::Equal => self.right.y.total_cmp(&other.right.y),
+            .y
-            ord => ord,
+            .total_cmp(&other.left.y)
-        }
+            .then_with(|| self.right.y.total_cmp(&other.right.y))
     }
 }
 

crates/bevy_pbr/src/cluster/assign.rs

@@ -26,8 +26,6 @@ use crate::{
     MAX_UNIFORM_BUFFER_CLUSTERABLE_OBJECTS,
 };
 
-use super::ClusterableObjectOrderData;
-
 const NDC_MIN: Vec2 = Vec2::NEG_ONE;
 const NDC_MAX: Vec2 = Vec2::ONE;
 
@@ -254,16 +252,10 @@ pub(crate) fn assign_objects_to_clusters(
     if clusterable_objects.len() > MAX_UNIFORM_BUFFER_CLUSTERABLE_OBJECTS
         && !supports_storage_buffers
     {
-        clusterable_objects.sort_by(|clusterable_object_1, clusterable_object_2| {
+        clusterable_objects.sort_by_cached_key(|clusterable_object| {
-            crate::clusterable_object_order(
+            (
-                ClusterableObjectOrderData {
+                clusterable_object.object_type.ordering(),
-                    entity: &clusterable_object_1.entity,
+                clusterable_object.entity,
-                    object_type: &clusterable_object_1.object_type,
-                },
-                ClusterableObjectOrderData {
-                    entity: &clusterable_object_2.entity,
-                    object_type: &clusterable_object_2.object_type,
-                },
             )
         });
 

crates/bevy_pbr/src/cluster/mod.rs

@@ -2,7 +2,6 @@
 
 use core::num::NonZero;
 
-use self::assign::ClusterableObjectType;
 use bevy_core_pipeline::core_3d::Camera3d;
 use bevy_ecs::{
     component::Component,
@@ -516,34 +515,6 @@ impl Default for GpuClusterableObjectsUniform {
     }
 }
 
-pub(crate) struct ClusterableObjectOrderData<'a> {
-    pub(crate) entity: &'a Entity,
-    pub(crate) object_type: &'a ClusterableObjectType,
-}
-
-#[allow(clippy::too_many_arguments)]
-// Sort clusterable objects by:
-//
-// * object type, so that we can iterate point lights, spot lights, etc. in
-//   contiguous blocks in the fragment shader,
-//
-// * then those with shadows enabled first, so that the index can be used to
-//   render at most `point_light_shadow_maps_count` point light shadows and
-//   `spot_light_shadow_maps_count` spot light shadow maps,
-//
-// * then by entity as a stable key to ensure that a consistent set of
-//   clusterable objects are chosen if the clusterable object count limit is
-//   exceeded.
-pub(crate) fn clusterable_object_order(
-    a: ClusterableObjectOrderData,
-    b: ClusterableObjectOrderData,
-) -> core::cmp::Ordering {
-    a.object_type
-        .ordering()
-        .cmp(&b.object_type.ordering())
-        .then_with(|| a.entity.cmp(b.entity)) // stable
-}
-
 /// Extracts clusters from the main world from the render world.
 pub fn extract_clusters(
     mut commands: Commands,

crates/bevy_pbr/src/light/mod.rs

@@ -522,24 +522,6 @@ pub enum SimulationLightSystems {
     CheckLightVisibility,
 }
 
-// Sort lights by
-// - those with volumetric (and shadows) enabled first, so that the volumetric
-//   lighting pass can quickly find the volumetric lights;
-// - then those with shadows enabled second, so that the index can be used to
-//   render at most `directional_light_shadow_maps_count` directional light
-//   shadows;
-// - then by entity as a stable key to ensure that a consistent set of lights
-//   are chosen if the light count limit is exceeded.
-pub(crate) fn directional_light_order(
-    (entity_1, volumetric_1, shadows_enabled_1): (&Entity, &bool, &bool),
-    (entity_2, volumetric_2, shadows_enabled_2): (&Entity, &bool, &bool),
-) -> core::cmp::Ordering {
-    volumetric_2
-        .cmp(volumetric_1) // volumetric before shadows
-        .then_with(|| shadows_enabled_2.cmp(shadows_enabled_1)) // shadow casters before non-casters
-        .then_with(|| entity_1.cmp(entity_2)) // stable
-}
-
 pub fn update_directional_light_frusta(
     mut views: Query<
         (

crates/bevy_pbr/src/render/light.rs

@@ -837,16 +837,10 @@ pub fn prepare_lights(
     // - then those with shadows enabled first, so that the index can be used to render at most `point_light_shadow_maps_count`
     //   point light shadows and `spot_light_shadow_maps_count` spot light shadow maps,
     // - then by entity as a stable key to ensure that a consistent set of lights are chosen if the light count limit is exceeded.
-    point_lights.sort_by(|(entity_1, light_1, _), (entity_2, light_2, _)| {
+    point_lights.sort_by_cached_key(|(entity, light, _)| {
-        clusterable_object_order(
+        (
-            ClusterableObjectOrderData {
+            ClusterableObjectType::from_point_or_spot_light(light).ordering(),
-                entity: entity_1,
+            *entity,
-                object_type: &ClusterableObjectType::from_point_or_spot_light(light_1),
-            },
-            ClusterableObjectOrderData {
-                entity: entity_2,
-                object_type: &ClusterableObjectType::from_point_or_spot_light(light_2),
-            },
         )
     });
 
@@ -858,12 +852,10 @@ pub fn prepare_lights(
     //   shadows
     // - then by entity as a stable key to ensure that a consistent set of
     //   lights are chosen if the light count limit is exceeded.
-    directional_lights.sort_by(|(entity_1, light_1), (entity_2, light_2)| {
+    // - because entities are unique, we can use `sort_unstable_by_key`
-        directional_light_order(
+    //   and still end up with a stable order.
-            (entity_1, &light_1.volumetric, &light_1.shadows_enabled),
+    directional_lights
-            (entity_2, &light_2.volumetric, &light_2.shadows_enabled),
+        .sort_unstable_by_key(|(entity, light)| (light.volumetric, light.shadows_enabled, *entity));
-        )
-    });
 
     if global_light_meta.entity_to_index.capacity() < point_lights.len() {
         global_light_meta

crates/bevy_picking/src/hover.rs

@@ -231,7 +231,7 @@ pub fn update_interactions(
         if let Some(pointers_hovered_entities) = hover_map.get(pointer) {
             // Insert a sorted list of hit entities into the pointer's interaction component.
             let mut sorted_entities: Vec<_> = pointers_hovered_entities.clone().drain().collect();
-            sorted_entities.sort_by_key(|(_entity, hit)| FloatOrd(hit.depth));
+            sorted_entities.sort_by_key(|(_, hit)| FloatOrd(hit.depth));
             pointer_interaction.sorted_entities = sorted_entities;
 
             for hovered_entity in pointers_hovered_entities.iter().map(|(entity, _)| entity) {

crates/bevy_render/src/camera/camera.rs

@@ -1220,10 +1220,7 @@ pub fn sort_cameras(
     // sort by order and ensure within an order, RenderTargets of the same type are packed together
     sorted_cameras
         .0
-        .sort_by(|c1, c2| match c1.order.cmp(&c2.order) {
+        .sort_by(|c1, c2| (c1.order, &c1.target).cmp(&(c2.order, &c2.target)));
-            core::cmp::Ordering::Equal => c1.target.cmp(&c2.target),
-            ord => ord,
-        });
     let mut previous_order_target = None;
     let mut ambiguities = <HashSet<_>>::default();
     let mut target_counts = <HashMap<_, _>>::default();

crates/bevy_scene/src/serde.rs

@@ -180,7 +180,7 @@ impl<'a> Serialize for SceneMapSerializer<'a> {
                     )
                 })
                 .collect::<Vec<_>>();
-            entries.sort_by_key(|(type_path, _partial_reflect)| *type_path);
+            entries.sort_by_key(|(type_path, _)| *type_path);
             entries
         };
 

crates/bevy_winit/src/winit_windows.rs

@@ -333,52 +333,26 @@ impl WinitWindows {
 ///
 /// The heuristic for "best" prioritizes width, height, and refresh rate in that order.
 pub fn get_fitting_videomode(monitor: &MonitorHandle, width: u32, height: u32) -> VideoModeHandle {
-    let mut modes = monitor.video_modes().collect::<Vec<_>>();
+    monitor
-
+        .video_modes()
-    fn abs_diff(a: u32, b: u32) -> u32 {
+        .max_by_key(|x| {
-        if a > b {
+            (
-            return a - b;
+                x.size().width.abs_diff(width),
-        }
+                x.size().height.abs_diff(height),
-        b - a
+                x.refresh_rate_millihertz(),
-    }
+            )
-
+        })
-    modes.sort_by(|a, b| {
+        .unwrap()
-        use core::cmp::Ordering::*;
-        match abs_diff(a.size().width, width).cmp(&abs_diff(b.size().width, width)) {
-            Equal => {
-                match abs_diff(a.size().height, height).cmp(&abs_diff(b.size().height, height)) {
-                    Equal => b
-                        .refresh_rate_millihertz()
-                        .cmp(&a.refresh_rate_millihertz()),
-                    default => default,
-                }
-            }
-            default => default,
-        }
-    });
-
-    modes.first().unwrap().clone()
 }
 
 /// Gets the "best" video-mode handle from a monitor.
 ///
 /// The heuristic for "best" prioritizes width, height, and refresh rate in that order.
 pub fn get_best_videomode(monitor: &MonitorHandle) -> VideoModeHandle {
-    let mut modes = monitor.video_modes().collect::<Vec<_>>();
+    monitor
-    modes.sort_by(|a, b| {
+        .video_modes()
-        use core::cmp::Ordering::*;
+        .max_by_key(|x| (x.size(), x.refresh_rate_millihertz()))
-        match b.size().width.cmp(&a.size().width) {
+        .unwrap()
-            Equal => match b.size().height.cmp(&a.size().height) {
-                Equal => b
-                    .refresh_rate_millihertz()
-                    .cmp(&a.refresh_rate_millihertz()),
-                default => default,
-            },
-            default => default,
-        }
-    });
-
-    modes.first().unwrap().clone()
 }
 
 pub(crate) fn attempt_grab(

tools/build-templated-pages/src/examples.rs

@@ -26,10 +26,7 @@ struct Example {
 
 impl Ord for Example {
     fn cmp(&self, other: &Self) -> Ordering {
-        match self.category.cmp(&other.category) {
+        (&self.category, &self.name).cmp(&(&other.category, &other.name))
-            Ordering::Equal => self.name.cmp(&other.name),
-            ordering => ordering,
-        }
     }
 }