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

C++ API #7

Merged
merged 22 commits into from
Jun 2, 2024
Merged

C++ API #7

merged 22 commits into from
Jun 2, 2024

Conversation

ipadjen
Copy link
Collaborator

@ipadjen ipadjen commented Apr 2, 2024

Working pull request

todo:

  • Use footprint containing vertex elevation for building base
  • Expose (some) reconstruction parameters (through a configuration struct probably?)
  • Error handling? eg if reconstruction is not possible due to insufficient points
  • Docs
  • Tests
  • Make it header only?

@balazsdukai
Copy link
Member

@Ylannl Ylannl mentioned this pull request Apr 3, 2024
37 tasks
@Ylannl
Copy link
Member

Ylannl commented Apr 9, 2024

Some further remarks:

  1. We discussed to make different reconstruction functions for different LoD's. In retrospect I think this should not be done, let's just keep the reconstruction API as simple as possible and provide one function (possibly with overloads, like you have now) that can do everything. Each LoD could still be reconstructed by using the right parameters. This means that the reconstruct app won't use the API, but this is ok.
  2. Regarding lifting the floor of the building mesh. A good option here could also be to triangulate the input footprint (with vertex elevations), and then for each point in the mesh floor: locate it in the triangulation, do (linear) interpolation (I have some code snippets for this) and update the z of the floor vertex in the mesh accordingly. This would be quite robust I think. Especially in handling cases like when a polygon hole is created in the floor of the building during reconstruction (ie a hole that is not present in input footprint).

@Ylannl
Copy link
Member

Ylannl commented Apr 9, 2024

I added the most basic parameters.

There are a lot more parameters that could be added, but those are tricky to tune without looking at output of intermediate steps in reconstruction pipeline. So I started with the most basic ones here that are easy to understand and not dependent on the quality of the point cloud. These should be sufficient for most use cases.

Ideally we can in the future have a method to automatically judge the point cloud quality (eg based on pointdensity, and planarity) to set the quality dependent parameters automatically to sensible values.

@ipadjen

This comment was marked as resolved.

@ipadjen ipadjen assigned ipadjen and unassigned ipadjen Apr 11, 2024
@ipadjen

This comment was marked as resolved.

@Ylannl

This comment was marked as resolved.

@ipadjen

This comment was marked as resolved.

@Ylannl Ylannl added this to the API complete milestone Apr 24, 2024
@Ylannl Ylannl changed the title API C++ API Apr 24, 2024
Copy link
Member

@Ylannl Ylannl left a comment

Choose a reason for hiding this comment

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

For later reference, some things to look at/consider.

Copy link
Member

Choose a reason for hiding this comment

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

we could also do this using templates/concepts


//todo temp for testing
void write_cdt_to_obj(const DT& cdt, const std::string& filename);
Copy link
Member

Choose a reason for hiding this comment

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

remove?

*
* //todo doc
*/
template <typename Footprint>
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to define a concept for Footprint

elevation_provider = roofer::reconstruction::createElevationProvider(cfg.floor_elevation);
}

#ifdef ROOFER_VERBOSE
Copy link
Member

Choose a reason for hiding this comment

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

use the roofer::logger

const float floor_elevation,
ArrangementExtruderConfig cfg
Arrangement_2& arr,
const ElevationProvider& elevation_provider,
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned before, consider use a concept for ElevationProvider (?)

@@ -15,6 +15,14 @@
// along with this program. If not, see <http://www.gnu.org/licenses/>.
#include <roofer/reconstruction/cdt_util.hpp>

#include <CGAL/Polygon_2.h>
#include <CGAL/Barycentric_coordinates_2/triangle_coordinates_2.h>
//todo temp for testing
Copy link
Member

Choose a reason for hiding this comment

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

remove?

@Ylannl Ylannl merged commit 0d52eac into develop Jun 2, 2024
7 of 8 checks passed
@Ylannl
Copy link
Member

Ylannl commented Jun 2, 2024

FYI I merged this now, to keep the code in sync with other developments. I suggest we open a new PR for the API for the remaining work.

@Ylannl Ylannl deleted the develop-api branch June 2, 2024 14:41
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.

3 participants