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

New MiraMonVector driver #9688

Merged
merged 8 commits into from
Apr 19, 2024
Merged

Conversation

AbelPau
Copy link
Contributor

@AbelPau AbelPau commented Apr 17, 2024

What does this PR do?

This driver allows to create and open MiraMon polygon, stringline and point vectors.

What are related issues/pull requests?

There are not related issues or pull requests.

Tasklist

  • Make sure code is correctly formatted (cf pre-commit configuration)
  • Add test case(s)
  • Add documentation
  • Updated Python API documentation (swig/include/python/docs/)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

Environment

Provide environment details, if relevant:

  • OS:
  • Compiler:

@rouault rouault force-pushed the miramon_clean_history branch from 21b7707 to 5443cfe Compare April 17, 2024 15:33
@rouault
Copy link
Member

rouault commented Apr 17, 2024

I've worked together with @AbelPau over the past weeks to put the driver into shape. You can find the details of the review in AbelPau#12 . The branch in its state of ~ one week ago was submitted to coverity-scan and issues were fixed .I've also run it through ossfuzz locally on my PC with the undefined and address santizers, on the new miramon dedicated fuzzer, and it now runs for a couple days without issues. So from my perspective, we are in a state acceptable for upstreaming.

@rouault rouault force-pushed the miramon_clean_history branch from 5443cfe to fad106a Compare April 17, 2024 15:53
doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved
doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved

In MiraMon the concepts of OGRMultiPoints and OGRMultiLineStrings are not supported,
but the driver translates a multipoint into N points and a multistring into N arcs.
So, when reading a MiraMon file of type *.pol*, the corresponding
Copy link
Collaborator

@nyalldawson nyalldawson Apr 17, 2024

Choose a reason for hiding this comment

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

This is a little confusing to me. How does the handling of multipolygons relate to the limitation on multipoint/linestrings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A multipolygon has a list of stringline indices (in the *.pol file) and when the vertices of a multipolygon (or a polygon) are dumped the coordinates are extracted from the corresponding *.arc file using that list of indices (and other details). You can consult how it's organized a multipolygon (or a polygon) more precisely here: https://www.miramon.cat/new_note/eng/notes/[MiraMon_structured_vectors_file_format.pdf](https://www.miramon.cat/new_note/eng/notes/MiraMon_structured_vectors_file_format.pdf)

But the concept of having a a group of connected lines it's not present in MiraMon, for the moment.

doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved
doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved
doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved
doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved
doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved
doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved
doc/source/drivers/vector/miramon.rst Outdated Show resolved Hide resolved
@rouault rouault force-pushed the miramon_clean_history branch from f30137f to 4b71cd2 Compare April 18, 2024 08:49
@coveralls
Copy link
Collaborator

coveralls commented Apr 18, 2024

Coverage Status

coverage: 69.004% (+0.007%) from 68.997%
when pulling 1430c03 on AbelPau:miramon_clean_history
into 88b0dd8 on OSGeo:master.

@rouault rouault merged commit 10fa4d1 into OSGeo:master Apr 19, 2024
32 checks passed
@rouault rouault added this to the 3.9.0 milestone Apr 19, 2024
@landryb
Copy link
Contributor

landryb commented Nov 16, 2024

was there a particular reason to disable that driver on little-endian ? this makes packaging on OpenBSD/powerpc64 fail because share/gdal/MM_m_idofic.csv isn't installed. I can work that around by disabling the driver but just wondering..

bob-beck pushed a commit to openbsd/ports that referenced this pull request Nov 16, 2024
…ndian

the driver was added in OSGeo/gdal#9688 but is
disabled on !BE, thus MM_m_idofic.csv isn't installed and packaging
fails on at least powerpc64
@rouault
Copy link
Member

rouault commented Nov 16, 2024

was there a particular reason to disable that driver on little-endian ?

You meant on big-endian. if I remember well, it would build fine, but it lacks appropriate byte-swapping, causing read errors on reading, and generating invalid files on writing on big-endian hosts, so it is a better service for potential users of those platform to just disable it.

@AbelPau
Copy link
Contributor Author

AbelPau commented Nov 17, 2024 via email

@rouault
Copy link
Member

rouault commented Nov 17, 2024

There is any I could do to improve the driver in this aspect?

GDAL has a number of CPL_LSBxxxx macros in cpl_port.h to do conditional byte-swapping . The issue is that Travis-CI is now defunct, so we no longer have any big-endian CI configuration to check our compatiblity with big-endian hosts. So to test, you'd likely have to use locally a Docker image of a Linux distribution (for example ubuntu, but also works with s390x/alpine:edge), with qemu-based emulation (which will be painfully slow, including compiling. Expect 10 times slower than bare-metal execution):

$ docker pull --platform linux/s390x s390x/ubuntu:24.04
$ docker run --name s390x_ubuntu -it --platform linux/s390x -v $HOME:$HOME s390x/ubuntu:24.04

But honestly, that would be a very bad use of your time. To the best of my knowledge, all big-endian hosts (powerpc or IBM s390x or some ARM chips) are now super legacy architectures that almost nobody actually use (or if they still use them, that should only be to run their legacy apps). I'm still wondering why some open source Unix distributions lose their time (and ours!) trying to still support them.

@landryb
Copy link
Contributor

landryb commented Nov 18, 2024

But honestly, that would be a very bad use of your time.

I have to agree with that statement, it was just out of curiosity. Since the shipped files list changes, it failed to package on powerpc64, so i tried to understand why.

To the best of my knowledge, all big-endian hosts (powerpc or IBM s390x or some ARM chips) are now super legacy architectures that almost nobody actually use (or if they still use them, that should only be to run their legacy apps).

powerpc64 which isn't that legacy at all.. but it falls in the 'exotic' category :)

@AbelPau
Copy link
Contributor Author

AbelPau commented Nov 18, 2024

OK, thanks for your explanations. So, if anybody really needs this feature we'll let it as it is.
Thanks!

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.

5 participants