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

Finish SimpleSets, add chorematic map, and refactoring #48

Open
wants to merge 107 commits into
base: master
Choose a base branch
from

Conversation

Yvee1
Copy link
Contributor

@Yvee1 Yvee1 commented Jan 24, 2025

This PR concerns:

  • remaining SimpleSets pieces:
    • smoothing
    • frontend
    • documentation
  • chorematic_map: a new module that summarizes (two-)classed region maps with a single disk.

Why both in one PR? Two reasons 1. The SimpleSets pieces were not quite finished when I started on the chorematic map module but I wanted to use some helper functions introduced in those commits; 2. during development of the chorematic map I moved SimpleSets helper functions to core.

The PR changes files in modules other than simplesets and chorematic_map:

  • I added settings to GeometryRenderer: line join, line cap, text alignment, clipping; with corresponding implementations in IpeRenderer, GeometryWidget, SvgRenderer, and PaintingRenderer.
  • I added a Halfplane class, with corresponding rendering functions. (I did not use it in the end, but I think we should just keep it in).
  • To drawText I added a parameter escape that specifies whether special characters should be escaped. When exporting to ipe you may want to use LaTeX command; when exporting to SVG you may want to use tags to make text bold, italic etc.
  • I added an option set the LaTeX preamble in the IpeRenderer.
  • In the IpeRenderer save function I check whether the extension is ".pdf", and if so set the file format to PDF. Somehow this doesn't quite work (on my pc at least). The pdf can be opened in ipe, but not directly in a pdf viewer. Opening the pdf in ipe and saving yields a working pdf.
  • I changed the drawing of PolygonWithHoles to an implementation in GeometryRenderer. PolygonSet is no longer drawn by drawing all its constituent PolygonWithHoles, but instead as a separate RenderPath. Now in e.g. ipe a PolygonSet is one object/path instead of several.
  • I fixed compiler warnings.
  • I moved/added all kinds of helper functions to core:
    • Rectangle helper functions from simplesets and isoline_simplification were moved to core.
    • Arrangement helper functions from simplesets
    • Circular-segment traits helper functions.
    • Vector helpers
    • pretendExact functions to go from Inexact to Exact representation.

Some things that require particular attention:

  • I merged in the simplification PR as I needed it at some point: Start simplification module with VW simplification #20. It's currently in a partially broken state. I made some changes to make the code compile on my side, and parameterized the simplification functions on face data such that RegionArrangement can be simplified while retaining their region information. I discovered a mistake in the use/documentation of a CGAL function (Arrangement merge_edge assigns incorrect direction CGAL/cgal#8659) that I created a workaround for that doesn't quite work. We need to decide what to do with this. Ideally we get it to a better state before merging, or somehow cut out all relevant commits from this PR.
  • For SimpleSets smoothing I wanted to offset polygons consisting of circular arcs and line segments (without approximating the circular arcs with line segments). CGAL does not offer such functionality but there is a C++ library that does, CavalierContours. I wrote wrappers around it and it seems to work quite well. Currently the library is copied inside the simplesets/helpers directory, but we need to think about how to handle this.
  • For chorematic_map I use external code as well, but it is just a single file (natural_breaks_external) for computing Fisher-Jenks natural breaks. It's under a GPL v3.0 license, which I reference in the header, so should be fine, but would be good to check.
  • The core got filled by quite some files with helper functions. I think it's good to put generally useful functions outside of modules implementing specific map types, just as was already done with e.g. centroid.cpp, but I'm not sure that core is the right place.
  • The chorematic_map module depends on GDAL for parsing GeoPackage files. GDAL support many formats; for the future, it could be nice to write a proper wrapper around GDAL so that RegionMaps cannot only be read from .ipe files but from any file format GDAL supports as well.
  • Related to that; the current chorematic_map demos read in such a GeoPackage file, but I did not add this to the cartocrow repository as they are quite large. This means you cannot run the demo without first downloading the appropriate GeoPackage file.
  • I added some functionality to the IpeReader so that shapes in an ipe file can be parsed to RenderPaths. As IpeReader was in core, that would mean core would depend on renderer (but renderer already depends on core). So I moved IpeReader to the renderer module. But that does not seem like the right place either. We may need to make a new module for parsing/reading data.
  • In the documentation main page I made categories "Spatial set visualization" and "Chorematic maps".

Meulemans and others added 30 commits October 25, 2022 11:12
Adapt regionMapToArrangement() to the new, templated version of
RegionArrangement. Unfortunately this means that this function too has
to become a function template, and hence it had to be pulled to the
header file.
* created a demo
* renamed the generic case to vertex removal
* generalized traits to also abstract from the vertex data type
* fixed bugs
- fixed bugs
- added historic arrangement, such that simplify() calls can also go back up in complexity
- updated demo
- pulled the history out of the simplification class, s.t. you can keep the history without the simplification object
- made the simplification class not require a historic arrangement, but can use it, if provided
- added an oblivious arrangement that does not keep a history
- added concepts for the templates
- Code cleanup
- Simplifying to threshold
- Providing some first, basic comments in all files
…ed long template implementations into separate files
For some reason, the order in which libraries are linked matters.
Interestingly enough, linking with mold works regardless of the order.
- batched operations in historic arrangement
- allowing shifting of vertices (in prep of other algorithms)
- util functions changed to modify not just one object, but everything around it
  * this is to keep a consistent geometry; the util functions are now one-invoke to achieve what is necessary, rather than puzzling out which edges, vertices, etc, had to move in conjunction
…ompilable again.

Culled isoline_simplification from cmake files, since it didn't compile
@Willem3141
Copy link
Member

Thank you! This is going to take quite some time to study 😅

After a first glance: as you already wrote, we have to figure out what to do with the CavalierContours dependency. In any case it cannot stay into the repo just like this. Instead of the current setup with a CMake include, I would prefer a setup where the user simply installs the library independently (with instructions in the README) with our CMake config picking up the existing installation.

Additionally I'm a bit concerned because according to the CavalierContours repo, the project is moving to Rust and the C++ version is already unmaintained.

The other external dependency, being only one single file, and given that it is not coming from a Git repo, I think is acceptable to have here.

Let's discuss soon how to approach this.

@Willem3141
Copy link
Member

Things to resolve before merging:

  • Simplification PR: is way too entangled to separate out again; will probably just have to be merged like this; discuss with Wouter.
  • CavalierContours: just go with the C++ version for now. Remove from this PR, add instructions to README on how to install (on all platforms). If it can't be made to work (easily) on for example MSVC, provide a CMake flag to turn off SimpleSets compilation and document it.
  • GDAL: make sure it's documented.
  • Chorematic maps demo: simplest solution is to change the error to say where the file can be downloaded. Alternative: make it use a smaller file.
  • IpeReader: make a new import module.
  • Circle segment support code: move from core to a new module. (Other helpers in core can stay there for now, will be cleaned up later.)

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.

2 participants