Rectangle::getUnion fails property tests

Here are some properties that getUnion could sensibly be expected to hold (I’m using the <> operator for getUnion and Haskell-like syntax for brevity):

x <> y == y <> x
reduced c x <> reduced c y == reduced c (x <> y)
expanded c (reduced c x <> reduced c y) == x <> y

Sensible programmers would expect these properties to hold, as Jules did when he implemented juce::Grid, resulting in a bug.

I argue that this behaviour Rectangle::getUnion is thus bug-prone and that it should hold these sensible properties just like Range::getUnionWith does.

Cheers, Yair

Those are sensible properties, and getUnion holds all of them except in the case where both rectangles have zero area.

The difficult thing here is that the docs say “If either this or the other rectangle are empty, they will not be counted as part of the resulting region.” which makes returning {} the only sensible commutation-preserving choice for the double empty situation. However, making this change will almost certainly introduce some subtle bugs into people’s software.

The other option is changing the docs and behaviour of that function to make getUnion take empty rects into account (like Range does), but this would be even more disruptive.

Ultimately I agree that commutativity and consistency with Range are both desirable, but I don’t think it justifies breaking people’s projects.

I’m making an educated guess that most use case will not be affected, but in almost all instances where behavior will change as a result of this, just like in the juce::Grid instance - it will result with a bug being fixed, whether the authors were aware of the bug or not.

My feeling is that a lot of drawing code could be silently changed. Operations like taking the union of two rects marked as dirty.

1 Like

Our first approach for juce::Grid was to fix rectangle.

Another ‘nice to have’ (I guess it should be discussed by the JUCE team) is to have
Rectangle::getUnionWith this keep both behaviors and has same name as the one in Range

Also, if eventually it seems that getUnion is redundant with it, it’ll be possible to deprecate/easily find its usage since there is getUnionWith.

There were already changes in JUCE that made old code behave differently in the past.
juce::String(floatValue) changes

1 Like

Not just when they have zero areas. For example take the following property i mentioned before:

reduced c x <> reduced c y == reduced c (x <> y)

x and y themselves may be non-empty yet this property would fail.

Ah, I interpreted that shrink as a scale (meaning that you couldn’t shrink down to zero).

1 Like

You’re right, the method name in juce is reduced and I didn’t remember correctly (fixed my code above now)