Vect_cidx_find_all should not allow mixing GV_AREA with other geometries

[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.

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.

[mmetz]

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.

[neteler]

Ticket retargeted after milestone closed

[martinl]

Ticket retargeted after milestone closed

[martinl]

Still relevant?

[marisn]

Replying to martinl:

Still relevant?

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

[mmetz]

In 74446:

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

[mmetz]

In 74447:

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

[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).

[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)...

[mmetz]

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.