Stop considering Rectangle(0,0,0,0) null - while Rectangle(1,1,1,1) is not null #54646

strk · 2023-09-18T10:33:22Z

Aims at closing GH-45563

src/core/geometry/qgsrectangle.cpp

src/core/geometry/qgsrectangle.h

strk · 2023-10-04T08:16:31Z

Next failure is TestQgsLabelingEngine::testRegisterFeatureUnprojectible in tests/src/core/testqgslabelingengine.cpp which is reported as being a test for bug #23431

It looks like that test was written by myself in 4ed096b so I can't blame anyone else for missing comments (oops :)

strk · 2023-10-04T12:35:32Z

Difference spotted is again an Empty vs. Null difference, in

tests/src/core/testqgslabelingengine.cpp:947 : (testRegisterFeatureUnprojectible) [1ms] vl2->extent(): Null

vl2->extent() prints as Empty in master branch, as Null in the branch associated with this PR

Honestly I don't know what the correct answer should be, as vl2 is a QgsVectorLayer started empty. An EMPTY Rectangle is defined as being a rectangle with no area (a point, at this moment, which I'd also extend to a line). But an empty layer cannot have a point information in it. I start wondering if we really need the distinction between EMPTY and NULL, for a rectangle.

strk · 2023-10-04T13:04:59Z

Ok the Null vs. Empty is a non-issue as it's just an improved distinction output in the branch associated with this PR. Master would never print Null, always Empty, even for Null rectangles.

tests/src/python/providertestbase.py

strk · 2023-10-05T14:58:36Z

Next issue is with a WMS test of QGIS server that expects a GetFeatureInfoResponse having this bounding box:

 <BoundingBox maxy="0" maxx="0" miny="0" CRS="EPSG:3857" minx="0"/>

We're now outputting the -inf,+inf bounding box instead.

The question is: what is expected as a BoundingBox for a response with an empty bounding box ?

The file in question is https://github.com/qgis/QGIS/blob/master/tests/testdata/qgis_server/get_postgres_types_json_list.txt

@elpaso should we just omit the BoundingBox tag when the box is null ? What is the reference spec ?

strk · 2023-10-05T15:10:26Z

I've filed PR #54858 to address the null BoundingBox in WMS output

src/core/geometry/qgsrectangle.cpp

src/core/geometry/qgsrectangle.h

nyalldawson · 2023-10-06T22:32:30Z

@strk

Can you open PRS for 0dc22c1,
d5d9e5c,
6372bae
And
c74837a

?

Those can all be merged now

strk · 2023-10-07T23:23:57Z

@strk

Can you open PRS for 0dc22c1, d5d9e5c, 6372bae And c74837a

?

Those can all be merged now

Done in GH-54872 - I will rebase this one once that one is merged

strk · 2023-10-12T07:59:37Z

I don't understand the "testExtent" test from tests/src/python/providertestbase.py:

    def testExtent(self):
        reference = QgsGeometry.fromRect(
            QgsRectangle(-71.123, 66.33, -65.32, 78.3))
        provider_extent = self.source.extent()
        self.assertAlmostEqual(provider_extent.xMinimum(), -71.123, 3)

How is assigning the "reference" local variable expected to affects self.source.extent() ?

This was suggested by Benoit in qgis#54646 (comment) It may make it easier to spot missing isNull() checks by callers.

Expose these methods: - referenceExtent() - referenceSubsetString3Extent()

Construct a proper null rectangle by default. Make sure a Null rectangle is always also considered Empty. Print Null rectangle as Null, still print details of Empty rectangles. Update expected QgsRectangle::toString output on Null rectangle Includes unit tests Closes qgisGH-45563

strk · 2023-10-22T19:28:13Z

Nyall since this is now working, and has the default QgsRectangle constructor construct a null rectangle, should I re-move the ::createNull() in this PR ? I'd lean toward yes :)

nyalldawson · 2023-10-22T22:48:22Z

Woohoo! Nice work @strk!

This was suggested by Benoit in qgis#54646 (comment)

agiudiceandrea · 2023-11-07T18:43:48Z

Hi @strk, it seems to me this PR may have generated an issue in the "TIN interpolation" and "IDW interpolation" QGIS processing algorithms. Please see #55138.

According to the WFS 1.1.0 specs "The <WGS84BoundingBox> element is used to indicate the edges of an enclosing rectangle in decimal degrees of latitude and longitude in WGS84" but the expected behavior is not specified when the data are empty. Up to QGIS 3.32, QGIS server returned: ``` <ows:WGS84BoundingBox dimensions="2"> <ows:LowerCorner>0 0</ows:LowerCorner> <ows:UpperCorner>0 0</ows:UpperCorner> ``` However, as a side effect of a change in `QGSRectangle` (see: qgis#54646), QGIS server now returns since the 3.34 version: ``` <ows:WGS84BoundingBox dimensions="2"> <ows:LowerCorner>inf inf</ows:LowerCorner> <ows:UpperCorner>-inf -inf</ows:UpperCorner> ``` This changes restores the QGIS 3.32 behavior.

According to the WFS 1.1.0 specs "The <WGS84BoundingBox> element is used to indicate the edges of an enclosing rectangle in decimal degrees of latitude and longitude in WGS84" but the expected behavior is not specified when the data are empty. Up to QGIS 3.32, QGIS server returned: ``` <ows:WGS84BoundingBox dimensions="2"> <ows:LowerCorner>0 0</ows:LowerCorner> <ows:UpperCorner>0 0</ows:UpperCorner> ``` However, as a side effect of a change in `QGSRectangle` (see: #54646), QGIS server now returns since the 3.34 version: ``` <ows:WGS84BoundingBox dimensions="2"> <ows:LowerCorner>inf inf</ows:LowerCorner> <ows:UpperCorner>-inf -inf</ows:UpperCorner> ``` This changes restores the QGIS 3.32 behavior.

This was suggested by Benoit in qgis#54646 (comment)

github-actions bot added this to the 3.34.0 milestone Sep 18, 2023

benoitdm-oslandia assigned strk Sep 19, 2023

benoitdm-oslandia reviewed Sep 19, 2023

View reviewed changes

src/core/geometry/qgsrectangle.cpp Outdated Show resolved Hide resolved

benoitdm-oslandia reviewed Sep 19, 2023

View reviewed changes

src/core/geometry/qgsrectangle.h Show resolved Hide resolved

strk force-pushed the rectangle-null branch 2 times, most recently from 8b19439 to a60739f Compare October 2, 2023 10:03

kannes mentioned this pull request Oct 3, 2023

Improve layout clipping test #54820

Merged

strk mentioned this pull request Oct 4, 2023

Fix aspect ratio computation in QgsLayoutItemMap::zoomToExtent #54827

Merged

strk force-pushed the rectangle-null branch from dbdb762 to 9b320e3 Compare October 4, 2023 14:38

strk commented Oct 4, 2023

View reviewed changes

tests/src/python/providertestbase.py Outdated Show resolved Hide resolved

strk force-pushed the rectangle-null branch from 9b320e3 to da8ed22 Compare October 5, 2023 07:10

strk force-pushed the rectangle-null branch from da8ed22 to ac6f543 Compare October 5, 2023 15:00

strk mentioned this pull request Oct 5, 2023

Do not include a BoundingBox tag in GetFeatureInfo responses, when it is null #54858

Merged

strk force-pushed the rectangle-null branch from ac6f543 to f93e1a9 Compare October 6, 2023 08:54

lnicola reviewed Oct 6, 2023

View reviewed changes

src/core/geometry/qgsrectangle.cpp Show resolved Hide resolved

lnicola reviewed Oct 6, 2023

View reviewed changes

src/core/geometry/qgsrectangle.h Outdated Show resolved Hide resolved

strk force-pushed the rectangle-null branch from bcf3722 to c74837a Compare October 6, 2023 15:17

strk mentioned this pull request Oct 7, 2023

Tweak Null rectangle handling #54872

Merged

strk force-pushed the rectangle-null branch from c74837a to 0c80066 Compare October 9, 2023 14:58

strk mentioned this pull request Oct 11, 2023

Fix extent computation in QgsSpatiaLiteProvider #54917

Merged

strk force-pushed the rectangle-null branch 2 times, most recently from 76a8ee9 to 0ff14a1 Compare October 11, 2023 23:23

strk force-pushed the rectangle-null branch 2 times, most recently from baf8156 to 87b7dad Compare October 18, 2023 13:13

strk mentioned this pull request Oct 18, 2023

Fix extent computation in QgsVirtualLayerProvider #54976

Merged

strk force-pushed the rectangle-null branch from 87b7dad to b62cfc7 Compare October 19, 2023 10:29

strk added a commit to strk/QGIS that referenced this pull request Oct 19, 2023

Use all nan values for default-constructed QgsRectangle

393fbcf

This was suggested by Benoit in qgis#54646 (comment) It may make it easier to spot missing isNull() checks by callers.

strk mentioned this pull request Oct 19, 2023

Use all nan values to represent a NULL QgsRectangle #54989

Closed

strk added the API Break! Breaks stable API. Proceed with extreme caution!! label Oct 19, 2023

strk force-pushed the rectangle-null branch from b62cfc7 to 2802966 Compare October 20, 2023 07:21

strk added 3 commits October 22, 2023 15:44

Show more detail on testExtentSubsetString null extent test failure

822b99e

Reduce use of magic numbers in provider tests

fec18c6

Expose these methods: - referenceExtent() - referenceSubsetString3Extent()

strk force-pushed the rectangle-null branch from 2802966 to 7ec8fa2 Compare October 22, 2023 13:46

Drop the temporary QgsRectangle::createNull static, not more needed

ef5692d

nyalldawson approved these changes Oct 22, 2023

View reviewed changes

nyalldawson merged commit f426999 into qgis:master Oct 23, 2023

strk deleted the rectangle-null branch October 23, 2023 07:42

strk added a commit to strk/QGIS that referenced this pull request Oct 23, 2023

Use all nan values to represent a null QgsRectangle

52765a3

This was suggested by Benoit in qgis#54646 (comment)

strk added a commit to strk/QGIS that referenced this pull request Oct 31, 2023

Use all nan values to represent a null QgsRectangle

27558c7

This was suggested by Benoit in qgis#54646 (comment)

ptitjano mentioned this pull request Jan 22, 2024

qgswfsgetcapabilities: Restore previous empty bounding box behavior #55927

Merged

strk added a commit to strk/QGIS that referenced this pull request Jun 25, 2024

Use all nan values to represent a null QgsRectangle

a3e7fa9

This was suggested by Benoit in qgis#54646 (comment)

nyalldawson mentioned this pull request Feb 19, 2025

[mssql] Fix empty extent handling #60671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop considering Rectangle(0,0,0,0) null - while Rectangle(1,1,1,1) is not null #54646

Stop considering Rectangle(0,0,0,0) null - while Rectangle(1,1,1,1) is not null #54646

strk commented Sep 18, 2023

strk commented Oct 4, 2023

strk commented Oct 4, 2023

strk commented Oct 4, 2023

strk commented Oct 5, 2023

strk commented Oct 5, 2023

nyalldawson commented Oct 6, 2023

strk commented Oct 7, 2023

strk commented Oct 12, 2023

strk commented Oct 22, 2023

nyalldawson commented Oct 22, 2023

agiudiceandrea commented Nov 7, 2023

Stop considering Rectangle(0,0,0,0) null - while Rectangle(1,1,1,1) is not null #54646

Stop considering Rectangle(0,0,0,0) null - while Rectangle(1,1,1,1) is not null #54646

Conversation

strk commented Sep 18, 2023

strk commented Oct 4, 2023

strk commented Oct 4, 2023

strk commented Oct 4, 2023

strk commented Oct 5, 2023

strk commented Oct 5, 2023

nyalldawson commented Oct 6, 2023

strk commented Oct 7, 2023

strk commented Oct 12, 2023

strk commented Oct 22, 2023

nyalldawson commented Oct 22, 2023

agiudiceandrea commented Nov 7, 2023