Prevent ImageBundles from causing constant layout recalculations

[Davier]

The complete UI layout is recalculated when a CalculatedSize is updated, and image_node_system updates them every frame for images. The fix checks if the size has actually changed.

[DJMcNab]

Seems sensible to me. I wonder if we could add something similar as a method to Mut, i.e.

impl<'a, T: PartialEq<T>> Mut<'a, T> {
    fn set_if_new(&mut self, value: T) -> Option<T>{
        if &*self == &value {
            *self = value;
            None
        } else {
            Some(value)
        }
    }
}

In theory, we could make a version which just returns T and returns the old value in the first branch, but I suspect that the return value of this function used often anyway.

Obviously to use it here we'd need to derive PartialEq for CalculatedSize, but that isn't the end of the world. I'm also not sure if just plain Eq would be more correct in this case

[alice-i-cecile]

Seems sensible to me. I wonder if we could add something similar as a method to Mut, i.e.

You definitely could, but I don't think it's worth expanding the API to save some pretty simple code?

I'm also not sure if just plain Eq would be more correct in this case

Eq is more correct here IMO: trying to figure out what the correct behavior is for when self == value but value != self or when we can't compare their equality seems unintuitive.

[rparrett]

Does this work? (Sorry, bit rude of me not to check myself.) Seems like you'd still be mut dereferencing CalculatedSize and triggering a Changed<CalculatedSize>.

This seems related to something that goes on with text, where we re-layout text on any Changed<Text> and thus modify CalculatedSize. So the act of mut dereferencing the text causes a re-layout even if the value ends up "the same."

At one point I wrote some code like this which seems to help but it's not exactly ergonomic.

fn test(mut queries: QuerySet<(Query<&Text>, Query<&mut Text>)>, state: Res<State>) {
    if let Some(text) = queries.q0().iter().next() {
        let score_text = format!("Score: {}", state.score);

        if (text.value != score_text) {
            if let Some(mut text) = queries.q1_mut().iter_mut().next() {
                text.value = score_text;
            }   
        }   
    }   
}

Something like that could perhaps go on in TextSystem, but there's already a QuerySet going on in there and it's a bit complicated. There's also the same concern around Changed<Style> I think.

[DJMcNab]

I can say with confidence that if the diagnosis of the problem is correct, this solution works.

The line calculated.size is only a Deref, not a DerefMut, which I'm confident of because the PartialEq impl which is being called takes Size by reference. The Deref impl of Mut<'a, T> does not change the mutated flag.

[rparrett]

Cool. I'm very happy that it's just my understanding of DerefMut that's flawed.

[cart]

Its probably a good idea to leave a comment here about why we're doing this check. People with less context might look at this and say "this check isn't needed".

[cart]

Nice catch!

set_if_new

I think the one benefit of adding a new method to Mut for this is that it self-documents intent. Otherwise i think we need comments everywhere to ensure people don't try to "remove an unnecessary branch"

[Davier]

Should I add set_if_new to this PR then? Or should that be a new PR?

[Davier]

Seems sensible to me. I wonder if we could add something similar as a method to Mut, i.e.

You definitely could, but I don't think it's worth expanding the API to save some pretty simple code?

As @rparrett pointed out, this issue is present everywhere. It's most notable in any UI component like Text, Style and Image since the layout calculations take so long in any non trivial app. I expect Mut::set_if_new to be useful in other parts of bevy too, since mutation detection is on by default (see #63).

Does this work?

Yes, I tested it.

[cart]

Hmm yeah lets do a separate pr to make it clearer / easier to point to in the changelog. We should also discuss the name as this will be used everywhere.

For now lets just add a comment above the if statement.