Preserve the order of window child nodes between updates

[ickshonpe]

Objective

set_window_children doesn't preserve the order of nodes between updates because the order of the queried entities can change.

Example

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, add_another_component)
        .run();
}

fn setup(mut commands: Commands) {
    let style = Style {
        width: Val::Percent(33.33),
        ..Default::default()
    };

    commands.spawn(Camera2dBundle::default());
    commands.spawn(NodeBundle {
        style: style.clone(),
        background_color: Color::RED.into(),
        ..Default::default()
    });

    commands.spawn(NodeBundle {
        style: style.clone(),
        background_color: Color::GREEN.into(),
        ..Default::default()
    });

    commands.spawn(NodeBundle {
        style: style.clone(),
        background_color: Color::BLUE.into(),
        ..Default::default()
    });
}

#[derive(Component)]
struct AnotherComponent;

fn add_another_component(
    mut delay: Local<f32>,
    mut commands: Commands,
    time: Res<Time>,
    query: Query<Entity, With<Style>>,
) {
    *delay += time.delta_seconds();
    if 2. < *delay {
        *delay = f32::NEG_INFINITY;
        let item = query.iter().next().unwrap();
        commands.entity(item).insert(AnotherComponent);
    }
}

The example displays red, green and blue UI nodes:

After two seconds a system adds AotherComponent to the first entity, and the order of the UI nodes changes (ymmv):

Solution

Concatenate the list of current window child nodes to the end of the previous list and then remove duplicates and nodes not present in the current list.

[rparrett]

Just to clarify -- it seems like this is worth fixing either way -- there is no way to guarantee that root nodes spawn/query in a particular order, right?

e.g. (without the add_another_component system)

commands.spawn((
    NodeBundle {
        style: style.clone(),
        background_color: Color::RED.into(),
        ..Default::default()
    },
    AnotherComponent,
));

commands.spawn(NodeBundle {
    style: style.clone(),
    background_color: Color::GREEN.into(),
    ..Default::default()
});

commands.spawn((
    NodeBundle {
        style: style.clone(),
        background_color: Color::BLUE.into(),
        ..Default::default()
    },
    AnotherComponent,
));

Shows up as R-B-G.

[ickshonpe]

Just to clarify -- it seems like this is worth fixing either way -- there is no way to guarantee that root nodes spawn/query in a particular order, right?

No at the moment, as far I know.

There is a need for some sort of redesign but I think we should fix the existing problems first. Same thing with the ContentSize PR I posted yesterday,

[ickshonpe]

It feels like there should be a better way to construct the set_window_children function, using two hashsets seems like too much. It only needs to be done once per layout update though, so it's not a huge deal. Correctness is more important here.

[rparrett]

I just rediscovered #8574, although I guess this would still be necessary even with that.

[rparrett]

I was hoping to find some set of variable names that makes it a bit clearer at a glance what's going on here but I find that I can't improve on what you've done.

I do think that a comment or two would be super useful.

[ickshonpe]

I was hoping to find some set of variable names that makes it a bit clearer at a glance what's going on here but I find that I can't improve on what you've done.

I do think that a comment or two would be super useful.

Yeah, I guess just something to explain the intent. It was an awkward bit of a code to write.

[ickshonpe]

I just rediscovered #8574, although I guess this would still be necessary even with that.

Yep the window children can still get shuffled about when the archetypes change or whatever.

I thought #8574 was okay, but no one seemed that interested. I guess it's something that might be better implemented either in Taffy or as part of the ecs.

[rparrett]

Does this branch ever execute in practice? It seems like taffy.children unconditionally returns Ok.

[nicoburns]

It seems like taffy.children unconditionally returns Ok

It currently does but arguably shouldn't, as it will panic if the node doesn't exist (note that if the node exists in Taffy, then the children definitely will as children is default initialized to an empty Vec). Likely Taffy's API will split/duplicated into panicking and result-returning variants at some point. See DioxusLabs/taffy#519

[rparrett]

This could be simplified slightly by bringing in an ordered set. indexmap is all over in our deps so it seems like it shouldn't be an issue to add it?

let parent_node = *self.window_nodes.get(&parent_window).unwrap();
let existing = self.taffy.children(parent_node).unwrap();

let children = existing
    .iter()
    .copied()
    .chain(children.map(|e| *self.entity_to_taffy.get(&e).unwrap()))
    .collect::<IndexSet<_>>()
    .iter()
    .copied()
    .collect::<Vec<_>>();

self.taffy.set_children(parent_node, &children).unwrap();

[ickshonpe]

Closing this as the design has changed and it's no longer a problem.