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

Fix bug #9360-revival: fix whole layer not rendered (when simplify geometry activated) #1087

Merged
merged 6 commits into from Jan 22, 2014

Conversation

ahuarte47
Copy link
Contributor

Ths patch fixes the bug #9360 when the code was translated from OGRGeometry* to OGRGeometryH classes in commit (3305a6c)

Changes:

  • Restore OGRGeometry* management to resize the simplified geometry with 'lineString->setPoints(...)'
  • Restore the shared point buffer to optimize the memory for simplification of large datasets

ahuarte47 referenced this pull request Jan 19, 2014
- stick to C API
- enable simplification only with GDAL >=1.9
@nirvn
Copy link
Contributor

nirvn commented Jan 19, 2014

@ahuarte47 , small clarification: I can reproduce the polygon layer not rendering regression (the second test project) using the qgis master build based on commit 174577b (debian-nightly ppa hosted on qgis.org). That build doesn't include the C++ -> C commit by @jef-n.

@ahuarte47
Copy link
Contributor Author

ok @nirvn, thanks, I will test it,
but would like to be corrected before the simplification in the master version, now the code is wrong

OGRGeometryH g = OGR_G_CreateGeometry( isaLinearRing ? wkbLinearRing : wkbLineString );

for (int i = 0; i < numPoints; i++, points++)
OGR_G_SetPoint( g, i, points->x, points->y, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jef-n, I released an experimental commit, to show the problem: this 'OGR_G_SetPoint' is a terrible bottleneck because of it does a realloc of one point each new point.

The only solution that I see is disable the simplification on ogr provider side when "__cplusplus" is not enabled, I can do it because of the provider capabalities indicates if it can simplify.

What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using GEOS.... there is other option:

GEOSCoordSequence* seq = GEOSCoordSeq_create(unsigned int size, unsigned int dims);
...

GEOSGeometry * g = GEOSGeom_createLinearRing(GEOSCoordSequence* s);
char * wkb = GEOSGeomToWKT(const GEOSGeometry* g);

Do you agree ?

NOTE:
OGR provider has not references to GEOS, it is the above option better ? (Disable ogr simplification ifndef "__cplusplus" )

@ahuarte47
Copy link
Contributor Author

Hi @nirvn, I see the layer disappear for scales ~1:499.973.983 I think that it occurs because of other error not present in my patch.

When a layer is rendered, the current map extent is transformed to layer coordinates (using the CRS of the layer), this transformation for so big extreme scales returns empty and any geometry is fetched to be drawed. 

The error occurs https://github.com/qgis/QGIS/blob/master/src/core/qgsmaprenderer.cpp#L414. I think this error is not caused by my patch, you may please disable simplification and verify the layer disappears?

If it is true, we can open one specific issue to fix it
Thank you very much!

@ahuarte47 , small clarification: I can reproduce the polygon layer not rendering regression (the second test project) using the qgis master build based on commit 174577b (debian-nightly ppa hosted on qgis.org). That build doesn't include the C++ -> C commit by @jef-n.

@nirvn
Copy link
Contributor

nirvn commented Jan 20, 2014

@ahuarte47 indeed, the point layer disappears in the extreme zoomed out scales even with simplification disabled. My friend, this is not your problem anymore ;)

Will file an issue with a test case project later this week. Keep up the excellent work your doing, all very appreciated.

@ahuarte47
Copy link
Contributor Author

Hi @nirvn, thank you very much for your support :-)

About this error, it is located in (https://github.com/qgis/QGIS/blob/master/src/core/qgsmaprenderer.cpp#L414): the code tries to transform a big extent from EPSG:4326 (map CRS) to the layer CRS and it returns an empty extent (then any geometry is fetched to be drawed).

When the new issue be added, I can try fix it.

@ahuarte47 indeed, the point layer disappears in the extreme zoomed out scales even with simplification disabled. My friend, this is not your problem anymore ;)
Will file an issue with a test case project later this week. Keep up the excellent work your doing, all very appreciated.

@ahuarte47
Copy link
Contributor Author

Ok, I disable ogr simplification ( snif! snif! ) while OGR_G_SetPoints does not exist, I add it to GDAL-OGR!
But as with OSREPSGTreatsAsNorthingEasting function I guess we can not use it for a long time.

Thanks @jef-n

@ahuarte47
Copy link
Contributor Author

Hi @jef-n, my pull request in GDAL has beed merged, I guess that it will included in 1.11
Best Regards

@ahuarte47
Copy link
Contributor Author

Hi @jef-n, Are you agree with the current state of this patch ?
The current simplification in master is broken.

Best Regards

jef-n added a commit that referenced this pull request Jan 22, 2014
Fix bug #9360-revival: fix whole layer not rendered (when simplify geometry activated; followup 3305a6c; fixes #9360)
@jef-n jef-n merged commit 12a463d into qgis:master Jan 22, 2014
@ahuarte47 ahuarte47 deleted the Issue_9360R branch January 22, 2014 11:44
@ahuarte47
Copy link
Contributor Author

Thanks @jef-n

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

3 participants