Skip to content

Add geometry polyhedral surface #1402

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

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

vissarion
Copy link
Member

This PR is based on #789 and adds a 3D geometry type for polyhedral surfaces according to OGC standard.

The main class, concepts, i/o and basic traits are added.

Comparing to #789 it refines the code (since that was an old PR and the library evolved since then) and also defines the polyhedral surface as a set of polygons (instead of rings) as described in the standard.

Finally, there are docs and unit tests.

// Write polyhedral surface wkt
std::cout << bg::wkt(polyhedral0) << std::endl;
std::cout << bg::wkt(polyhedral1) << std::endl;
std::cout << bg::wkt(polyhedral2) << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great you picked it up again!

  • there should be credit to Sidharta in the copyright section
  • how sure are we that this all works and is according to the standards? Are there rules for ordering faces?
  • should we check at least one property (for example area or volume) and compare it with another library
  • or are there ways (other OS tools or so) to visualize it to see we didn't miss anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great you picked it up again!

* there should be credit to Sidharta in the copyright section

I have already added as a commit coauthor, he hasn't added copyright notes in his original PR, but I agree that should be mentioned in copyrights too, so I added. @Siddharth-coder13 please let me know if you agree with those additions.

* how sure are we that this all works and is according to the standards? Are there rules for ordering faces?

We are not sure at this point, that check (consistent orientation of faces) could be part of validity checks. Another check should be related to dimension (assert that is 3). Does it make sense to add this in is_valid too? AFAIU this is similar to the overlapping polygon check for multi_polyogons (that check also applies for polyhedral surfaces).

The standard also defines the following methods (that will be implemented in a subsequent PR):
--- NumPatches () : Integer ⎯ Returns the number of including polygons
--- PatchN (N: Integer): Polygon ⎯ Returns a polygon in this surface, the order is arbitrary.
--- BoundingPolygons (p: Polygon): MultiPolygon ⎯ Returns the collection of polygons in this surface that
bounds the given polygon “p” for any polygon “p” in the surface.
--- IsClosed (): Integer - Returns 1 (True) if the polygon closes on itself, and thus has no boundary and
encloses a solid

* should we check at least one property (for example area or volume) and compare it with another library

Sure, but I think that should be after implementing is_valid and the basic methods above.

* or are there ways (other OS tools or so) to visualize it to see we didn't miss anything?

there are a few visualization software (e.g. geomview) but I do not know of any that supports WKT for polyhedral surfaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

EDIT: NumPatches () and PatchN (N: Integer) are already implemented by calling size() and [].


/*!
\brief A Polyhedral Surface is a contiguous collection of polygons,
which share common boundary segments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't check it. Suggestion:
"The concepts and algorithms don't check if the boundary segments are common"

Copy link
Member Author

Choose a reason for hiding this comment

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

I also added that common boundary segments should be checked by is_valid()

@vissarion
Copy link
Member Author

Thanks @barendgehrels for the quick review! I've addressed all your comments and replied to questions.

The PolyhedralSurface Concept is defined as following:

* There must be a specialization of the metafunction `traits::tag`, defining `polyhedral_surface_tag` as type
* It must behave like a Boost.Range Random Access Range
Copy link
Collaborator

Choose a reason for hiding this comment

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

Both ranges and concepts have been standardized in C++20, which BG does not yet depend on, after the definition of the original BG concepts which have existing users. For new BG concepts like this one, which do not have legacy usage that they need to be compatible to, should we consider documenting the requirement as behaving like C++ standard ranges instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure... we cannot yet make it dependent on C++20 yet...
Also it would be weird to let some concepts depend on the std-ranges/concepts, and others not...

Copy link
Collaborator

@tinko92 tinko92 May 3, 2025

Choose a reason for hiding this comment

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

we cannot yet make it dependent on C++20 yet...

Yes. To be clear, my thought was to document it based on the STL concept, not actually bumping the version requirement of the library. The behaviour could be used without depending on the actual C++ version in which it became part of the STL. The rationale is avoiding a future breaking change in this BG concept, if BG moves to a newer standard eventually.

But I understand the point about consistency with existing BG concepts.


[heading Description]
[concept PolyhedralSurface..polyhedral surface]

Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be text and a reference to the corresponding OGC simple feature missing here.

@@ -72,6 +72,18 @@ struct polygon_clear
}
};

template <typename Polyhedral_surface>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shoulds this be PolyhedralSurface (like MultiPoint)?

@@ -396,6 +420,15 @@ struct wkt<Polygon, polygon_tag>
>
{};

template <typename Polyhedral_surface>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like previous comment, should this be PolyhedralSurface?

// Boost.Geometry
// QuickBook Example

// Copyright (c) 2025 Siddharth Kumar, Roorkee, India.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 thanks - and cool that you co-authored, hadn't spotted that indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @vissarion for mentioning me in the copyrights.

{{{0, 0, 0}, {1, 0, 0}, {1, 0, 1}, {0, 0, 1}, {0, 0, 0}}},
{{{1, 1, 1}, {1, 0, 1}, {0, 0, 1}, {0, 1, 1}, {1, 1, 1}}},
{{{1, 1, 1}, {1, 0, 1}, {1, 0, 0}, {1, 1, 0}, {1, 1, 1}}},
{{{1, 1, 1}, {1, 1, 0}, {0, 1, 0}, {0, 1, 1}, {1, 1, 1}}}}; /*< Creating a polyhedral surface (cube) using standard initialized list >*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not experienced in this subject. I had to parse it mentally and it is a cube. Then I noticed it is written there - can that comment go first?

Same below - it's nice to have a pyramid, which is kind of realistic. But I think the example is not correct. I would expect at least a point in the center (2.5, 2.5) which is not there. Or it is not a pyramid. Didn't parse the rest...

Maybe a (squarish) pyramid would be a more realistic example. It should have 5 faces, the floor face having 4 points, the other faces having 3 points, if I'm correct. Or the closure points should be added in all faces. The 4 other faces should have one common point in the center.

And then there should a picture of it (somehow, for example in geomview, constructed in another way then).

Writing this gives me also the questions:

  • should the surfaces be closed? Will it follow the polygon rules?
  • should they be clockwise? Will it follow the polygon rules?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants