Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#3235 closed defect (fixed)

Vect_cidx_find_all should not allow mixing GV_AREA with other geometries

Reported by: marisn Owned by: grass-dev@…
Priority: major Milestone: 7.6.2
Component: LibVector Version: svn-trunk
Keywords: Cc:
CPU: Unspecified Platform: Unspecified

Description

At the moment Vect_cidx_find_all accepts any type mask. This leads to unusable results in case if typemask allows both GV_LINES and GV_AREA in the output. Reason - Vect_cidx_find_next will return line ID for GV_LINES and area ID for GV_AREA. As both ID spaces are separate (thus might contain identical ID values), mixing them in a single output structure is a road to disaster.

The fix - Vect_cidx_find_all should fail to accept type mask matching GV_LINES and GV_AREA (other combinations?) simultaneously. Other option - change Vect_cidx_find_all to modify two structs - one with line IDs and other with area IDs.

Change History (13)

in reply to:  description comment:1 by mmetz, 6 years ago

Replying to marisn:

At the moment Vect_cidx_find_all accepts any type mask. This leads to unusable results in case if typemask allows both GV_LINES and GV_AREA in the output. Reason - Vect_cidx_find_next will return line ID for GV_LINES and area ID for GV_AREA. As both ID spaces are separate (thus might contain identical ID values), mixing them in a single output structure is a road to disaster.

That really is a disaster. The original implementation of the category index did not take into account that area categories are added to the category index. Probably because this is duplicate information, an area can only have category values if it has a centroid with category values.

The fix - Vect_cidx_find_all should fail to accept type mask matching GV_LINES and GV_AREA (other combinations?) simultaneously. Other option - change Vect_cidx_find_all to modify two structs - one with line IDs and other with area IDs.

The real fix would be to not add area category values to the category index at all and update modules accordingly. This would be a substantial change in the vector libraries, but it can be implemented without breaking compatibility with G7. That would be a bit of work but I think it is worth the effort because it avoids ID collision.

comment:2 by martinl, 6 years ago

Milestone: 7.2.17.2.2

comment:3 by neteler, 5 years ago

Milestone: 7.2.27.2.3

Ticket retargeted after milestone closed

comment:4 by martinl, 5 years ago

Milestone: 7.2.3

Ticket retargeted after milestone closed

comment:5 by martinl, 5 years ago

Milestone: 7.2.4

comment:6 by martinl, 4 years ago

Still relevant?

in reply to:  6 comment:7 by marisn, 4 years ago

Milestone: 7.2.48.0.0

Replying to martinl:

Still relevant?

Complex code doesn't fix itself by accident. Yes, still relevant.

comment:8 by mmetz, 4 years ago

Resolution: fixed
Status: newclosed

In 74446:

vectorlib: Vect_cidx_find_all() should not allow mixing GV_AREA with other geometries, fixes #3235

comment:9 by mmetz, 4 years ago

In 74447:

vectorlib: Vect_cidx_find_all() should not allow mixing GV_AREA with other geometries, fixes #3235 (backport trunk r74446)

comment:10 by neteler, 4 years ago

Milestone: 8.0.07.6.2

comment:11 by mmetz, 4 years ago

Milestone: 7.6.28.0.0

Modules using Vect_cidx_find_all() are v.profile, v.out.ogr, v.db.select, v.net, v.edit, and none of them mixes GV_AREA with other geometries. A fatal error will occur if a new module does mix GV_AREA with other geometries when calling Vect_cidx_find_all() (programmer's error).

in reply to:  11 ; comment:12 by neteler, 4 years ago

Replying to mmetz:

Modules using Vect_cidx_find_all() are v.profile, v.out.ogr, v.db.select, v.net, v.edit, and none of them mixes GV_AREA with other geometries. A fatal error will occur if a new module does mix GV_AREA with other geometries when calling Vect_cidx_find_all() (programmer's error).

Then the bug report needs to be reopened again since we didn't reach 8.0.0 yet. Say, closing it with changes applied to 7.6.2 while the milestone is 8 is confusing (for me)...

in reply to:  12 comment:13 by mmetz, 4 years ago

Milestone: 8.0.07.6.2

Replying to neteler:

Replying to mmetz:

Modules using Vect_cidx_find_all() are v.profile, v.out.ogr, v.db.select, v.net, v.edit, and none of them mixes GV_AREA with other geometries. A fatal error will occur if a new module does mix GV_AREA with other geometries when calling Vect_cidx_find_all() (programmer's error).

Then the bug report needs to be reopened again since we didn't reach 8.0.0 yet. Say, closing it with changes applied to 7.6.2 while the milestone is 8 is confusing (for me)...

Sorry, that was trac confusion because we were editing the same time.

Note: See TracTickets for help on using tickets.