Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve split with lines #3539

Closed
wants to merge 1,005 commits into from
Closed

Conversation

bstroebl
Copy link

No description provided.

spatialIndex = vector.spatialindex(splitLayer)
splitGeoms = {}

for aSplitFeature in vector.features(splitLayer):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could speed this up a bit by passing a feature request to vector.features which uses setSubsetOfAttributes([]) to avoid fetching the unused attributes.


outFeat = QgsFeature()
features = vector.features(layerA)
total = 100.0 / float(len(features))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need to handle the case that len(features)==0

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, although this would only happen with an empty layer. This was basically a copy-paste operation from some other algo, so I expect to see this line in others, too. But will fix

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's how I know to look out for it... I hit it in some other algorithm with the same block

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-)

outFeat = QgsFeature()
features = vector.features(layerA)
total = 100.0 / float(len(features))
allowedWkbTypes = [2, #WkbLineString
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fragile, and doesn't handle Z/M geometries - I'd suggest instead checking the QgsGeometry.type() for QgsWkbTypes.LineGeometry and PolygonGeometry instead, and then using QgsGeometry.isMultipart() to avoid the multipart types.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be either Line (type = 1) or Polygon (type = 2) anyways because INPUT_A is set to be either TYPE_VECTOR_POLYGON or TYPE_VECTOR_LINE?
I could skip that altogether and when handling the geometry itself check if it is multipart.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically I would prefer to disable multi-layers to be used as input in the first place, but there is no method to do so AFAIK.

@bstroebl
Copy link
Author

bstroebl commented Oct 5, 2016

tests have been removed to have this PR pass the checks. I will add them again in a later PR

@Gustry
Copy link
Contributor

Gustry commented Oct 14, 2016

Is this PR ready to be reviewed ?

@bstroebl
Copy link
Author

Is this PR ready to be reviewed ?

Yes, from my point of view it is. Is there anything I have to do to have it reviewed? Nyall already did some reviewing (see discussion) on which I already reacted.

rldhont and others added 24 commits November 8, 2016 20:00
* Use QgsStringMap

* Better description

* Use new translated metadata methods
- strpos() now relies on a simple string within a string search
- regexp_match() now returns pos of a matching regular expression
[expression] strpos() and regexp_match() improvements
…vement

[processing] use algorithm description in modeler dependencies dialog
…faults

[processing] fix Ogr2OgrToPostGisList algorithm
WMS server: add user setting if custom datasources are allowed in wms…
…arts

[FIXBUG] Fix iteration of features in SinglePartsToMultiparts algorithm
This commit restores some pre 3.0 processing behaviour for number inputs.
Now, if a number input is required outside of modeller than a spin box
will be shown instead of a free text input.

Clicking the expression builder button results in an expression
which is evaluated immediately to avoid users expecting that
the expression will be evaluated per feature.
Export parametric SVG, will fallback symbols for the systems that cannot understand them
accept numeric, file and table field inputs in modeler

This allows a non-string parameter to be reused as a string
parameter in contexts where it makes sense.
This adds a new input type for expression inputs. Expression
inputs can be linked to a parent layer so that the builder
shows the correct fields and layer variables.

It's designed for two use cases:

1. to be used when an algorithm specifically requires an expression,
eg Select by Expression and Extract by Expression.

2. to be potentially used as a replacement input instead of string
or number literals in algorithms. Eg - if the simplify algorithm
tolerance parameter was replaced with an expression paremeter, then
this expression would be evaluated for every feature before
simplifying that feature. It would allow parameters to be calculated
per feature, as opposed to the current approach of calculating
a parameter once before running the algorithm. It would also
mean algorithms like "variable distance buffer" would no longer
be needed, as a single "buffer" algorithm could then be used
for either a fixed distance, field based, or expression based
distance.
alexbruy and others added 23 commits November 22, 2016 19:57
Clean up code, allow setting z/m columns
- use a proper toolbar to match other parts of QGIS
- show keyboard shortcuts in toolbar action tooltips
- create vector icons to replace PNG ones
[processing] improve the modeler dialog UI and icons
The -DCMAKE_POSITION_INDEPENDENT_CODE=ON resolves to -fPIE instead
of -fPIC (on some platforms?) rendering cmake broken.
Rename algorithm SplitLinesWithLines to SplitWithLines
Accept polygon as input, too
Use only selected lines to split with (if processing is set to use selection only)
Issue log message if trying to split multi geometries
Update help
@bstroebl
Copy link
Author

Now this got out of control because I tried git rebase master
I will create a new pull request with all changes that were included in this one but leave this open for reference for the time being
Sorry for any inconveniences

@bstroebl bstroebl deleted the improveSplitWithLines branch November 24, 2016 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet