[Qgis-developer] Potentially serious performance regression in new geometry - should 2.10 be delayed?

[Marco Hugentobler]

Hi Nyall

>- For methods which don't have a geos implementation (eg,
>QgsGeometry::smooth ), should they just be implemented in the
>QgsGeometryEngine base class?

They can stay in QgsGeometry as they don't require conversion to a 
different format and they would not benefit from prepare().

>It means that they'd need to be aware that QGIS uses geos
>for these geometry operations and use that QgsGeos is the class
>required for geometry relation/modification operations


Ideally, they work just with QgsGeometryEngine and don't realize which 
library is behind it.

>is there a good reason we couldn't instead
>move these geos implementations to QgsGeometryEngine itself and do
>away with QgsGeos?


See above. QgsGeos is a subclass of QgsGeometryEngine. The only thing to 
be done is probably to move the method 
QgsGeometryEditUtils::geometryEngine() to a better place (e.g. 
QgsGeometry) and add python bindings for it. That way, plugin 
programmers just get the recommended subclass (obviously QgsGeos) and 
work with the base class interface.

>and do
>away with QgsGeos?


Do you mean remove the python bindings for QgsGeos? Agreed, it will be 
good to discourage the direct use of the subclass.

Regards,
Marco


On 24.06.2015 01:13, Nyall Dawson wrote:
> On 24 June 2015 at 00:44, Marco Hugentobler
> <marco.hugentobler at sourcepole.ch> wrote:
>> Hi Nyall
>>
>>> I'm still a bit unclear as to where
>>> you are taking these classes though. Is the intention that eventually
>>> ALL operations will be done using QgsGeos?
>>
>> In the end, yes, everything should go through QgsGeometryEngine interface.
>> There are two possibilities:
>>
>> 1. client code creates QgsGeos, calls prepare() on it and makes repeaded
>> intersection(), etc. directly on QgsGeometryEngine. This can be done with
>> master branch as is.
> This sounds like the right approach to me, but I have a couple of
> further questions:
> - For methods which don't have a geos implementation (eg,
> QgsGeometry::smooth ), should they just be implemented in the
> QgsGeometryEngine base class?
> - I'm a bit concerned about the usability of this approach for plugin
> developers. It means that they'd need to be aware that QGIS uses geos
> for these geometry operations and use that QgsGeos is the class
> required for geometry relation/modification operations. That's not
> obvious and could be a stumbling block for new PyQGIS developers. It
> also locks these plugins then into using the geos engine, and if we
> ever decided to switch to a different engine then the plugins would
> also need to be updated.
> Just thinking aloud here... is there a good reason we couldn't instead
> move these geos implementations to QgsGeometryEngine itself and do
> away with QgsGeos? Then, in the (unlikely?) situation that a geos
> implementation is inferior to a native QGIS method we can
> mix-and-match between geos and alternative implementations as
> required.
>
>> 2. client code uses QgsGeometry and calls intersection() on it. To benefit
>> from prepared geometries, we would need to enhance QgsGeometry to cache
>> QgsGeos and call prepare() the first time it is used.
>>
>> Thinking about it, version 1 might be better. We cannot know if the cached
>> QgsGeos is really needed a second time and it might use more memory if not.
>> In the first case, the client code knows if it is intended to do repeaded
>> operations on the same geometry.
>>
>> What do you think?
>>
>>> Could we put a giant "NOT STABLE API AND MAY CHANGE" for
>>> all these classes, so that we can address any unforeseen issues in
>>> 2.12?
>>
>> That would be ok.
> Great! That's my biggest reservation - if we've got the opportunity to
> tweak the API for 2.12 then my fears have been unfounded :)
>
> Thanks!
> Nyall


-- 
Dr. Marco Hugentobler
Sourcepole -  Linux & Open Source Solutions
Weberstrasse 5, CH-8004 Zürich, Switzerland
marco.hugentobler at sourcepole.ch http://www.sourcepole.ch
Technical Advisor QGIS Project Steering Committee