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

Support for Curve Geometry #8

Open
zhangjinzhou opened this issue Jun 3, 2020 · 7 comments
Open

Support for Curve Geometry #8

zhangjinzhou opened this issue Jun 3, 2020 · 7 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@zhangjinzhou
Copy link

zhangjinzhou commented Jun 3, 2020

According to this RFC, gdal is able to handle curve geometry since 2.0. How does the nodejs interface handle this? Currently, the lib returns not supported geometry error.

@yocontra
Copy link
Owner

yocontra commented Jun 5, 2020

@zhangjinzhou Can you post your example code that returns the error? Our interface to GDAL may need some updating

@zhangjinzhou
Copy link
Author

@contra The code is straightforward.

const dataset = gdal.open("path to fgdb including curve geometry")
const layer = dataset.layers.get("layer including curve geometry")
layer.features.forEach((feature) => {
    const geom = feature.getGeometry();
});

@yocontra
Copy link
Owner

yocontra commented Jun 5, 2020

@zhangjinzhou Can you include the file then? I need a way to reproduce your error.

@zhangjinzhou
Copy link
Author

@contra I generated a fgdb with one layer and one geometry. Hope it is enough to explain the issue.
CurveGeom.gdb.zip

@yocontra yocontra added the bug Something isn't working label Jun 5, 2020
@mmomtchev
Copy link
Contributor

In fact I would qualify this as a missing feature. Curved geometries are indeed currently not supported by node-gdal-next even if the underlying support in GDAL exists. There is a substantial amount of code that is missing - mostly the underlying C++ classes for the JS objects representing the curved geometries. @contra, I think you can add it to the roadmap, but once again, it is a substantial amount of work and not a simple bugfix.
I have lots of free time at the moment, so maybe I will take a look.

@yocontra yocontra added enhancement New feature or request help wanted Extra attention is needed and removed bug Something isn't working labels Nov 10, 2020
@mmomtchev
Copy link
Contributor

I just went through the underlying C++ classes for the various geometry types and I found a huge amount of copying and pasting.
There are currently 7 geometry classes, with no common base, each of about 120 to 150 lines, with most of them being 100% identical.

Someone should completely rewrite that part, with one base class and some templating/inheritance, I think that the amount of code can be reduced with something like 80% to 90%, all while producing the missing curve geometries in the process.
If there is someone with good understanding of C++ and GDAL geometries, I am willing to provide Node/V8 guidance.

Look in src/gdal_[multi]{line|point|polygon}string.cpp

@mmomtchev
Copy link
Contributor

The geometries are a major mess. On the JS side, it is a hierarchy of classes that inherit from a base class, on the GDAL C++ side it is a hierarchy too and in node-gdal there are separate independent classes in which major care have been taken to always declare all the shared variables in the exact same order so that pointer casting works. It is total fubar and needs fixing otherwise I won't be able to async it.

mmomtchev added a commit to mmomtchev/node-gdal-async that referenced this issue May 5, 2021
oscarnc pushed a commit to AmristarSolutions/node-gdal-next that referenced this issue Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants