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

[Oracle] Fix MultiSurface with straight polygon and take into account orientation when writing polygon rings #33959

Merged
merged 3 commits into from
Jan 23, 2020

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Jan 21, 2020

When fixing warnings of #33930, I
looked at the code that transforms a QGIS geometry of type MultiSurface
to a Oracle geometry, and it appeared quite convoluted/risky (variables
being reaffected with values of other variables), and not
being able to deal with straight Polygon in MultiSurface.
The reverse situation (Oracle MultiSurface to QGIS MultiSurface) had the
same issue as well.

The MultiCurve/CompoundCurve code has been modified similarly. There was
no real bug. Just a sub-optimal behaviour on reading of MultiCurve
from Oracle, where all parts where promoted to CompoundCurve, even when
not strictly needed.

Also fixes polygon writing by reverse rings when needed to honour the winding order of Oracle polygons (#29085)

Note: If backporting is wished, #33930 will have to be backported first

… that we have automoc

Was causing duplicated symbols on my build environment
When fixing warnings of qgis#33930, I
looked at the code that transforms a QGIS geometry of type MultiSurface
to a Oracle geometry, and it appeared quite convoluted/risky (variables
being reaffected with values of other variables), and not
being able to deal with straight Polygon in MultiSurface.
The reverse situation (Oracle MultiSurface to QGIS MultiSurface) had the
same issue as well.

The MultiCurve/CompoundCurve code has been modified similarly. There was
no real bug. Just a sub-optimal behaviour on reading of MultiCurve
from Oracle, where all parts where promoted to CompoundCurve, even when
not strictly needed.
@rouault rouault added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Jan 21, 2020
@m-kuhn m-kuhn changed the title [BUGFIX] [Oracle] Fix MultiSurface with straight polygon [Oracle] Fix MultiSurface with straight polygon Jan 21, 2020
@rouault rouault changed the title [Oracle] Fix MultiSurface with straight polygon [Oracle] Fix MultiSurface with straight polygon and take into account orientation when writing polygon rings Jan 21, 2020
@nyalldawson
Copy link
Collaborator

@troopa81 can you review please?

Copy link
Contributor

@troopa81 troopa81 left a comment

Choose a reason for hiding this comment

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

great work! looks ok to me, I just have one question for rectangle 3D

@troopa81
Copy link
Contributor

It's all good for me, LGTM

@rouault
Copy link
Contributor Author

rouault commented Jan 22, 2020

@troopa81
OK, I see that in #33976 you propose to backport some functionality touching code related to the one of this PR. I'm not completely clear if all functionality that is in master related to Oracle geometry handling is backported per #33976 and thus if this PR be able to be backported ? (not critical to me for it to be backported. just assessing if it is doable)

@troopa81
Copy link
Contributor

@rouault You won't be able to backport everything indeed because Curve/MultiCurve/MultiSurface exists only in master. All the code to reverse rings to force them to be in the Oracle appropriate order could be backported but I'm not sure it should be.

It was a limitation and you remove it, it's kind of a feature to me.

@rouault
Copy link
Contributor Author

rouault commented Jan 23, 2020

It was a limitation and you remove it, it's kind of a feature to me.

Fair enough. Keeping this as master-only material.

@rouault rouault merged commit 76bb060 into qgis:master Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants