-
Notifications
You must be signed in to change notification settings - Fork 214
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
Defined a geometry for polyhedron #789
base: develop
Are you sure you want to change the base?
Defined a geometry for polyhedron #789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start!
But I can't see how it works yet. It looks quite unfinished.
Can you add
- a unit test, specifically for the polyhedron itself
- a unit test, for the wkt support (it seems as started too) of the polyhedron, to read it and to write it?
As far as I can judge the code, it won't work yet, so you will probably need to modify parts of the source code too.
I checked the code, after using it in one of the examples and tried to make sure that it doesn't throw any error, but I think there is a need to implement wkt support for a polyhedron, to see its working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Siddharth-coder13 thanks for the efforts!
From the design of the code I understand that the concept of a polyhedron is not very clear to you. I recommend to read the OGC standard e.g. https://www.ogc.org/standards/sfa and construct some unit tests (as @barendgehrels suggested). This will give you more intuition.
I also recommend to change the name from Polyhedron
to PolyhedralSurface
as in the standard.
|
||
namespace model | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add doxygen comments here to describe the parameters of the geometry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be simpler, clearer and more concrete.
After spending a week on the codebase, it seems to me that wkt input and output depends on multiple header files, including dimensions check and concept check. To read inputs, I added the following codes:-
I don't know why I am getting this error every time I build
I am also getting more error like the above, I need some help in understanding why this error turns up every time. |
The message indicates that the code expects a point, or an instance implementing the Point Concept, but it is not a point. Glancing through your code, the problem is probably here.
and it wants to append a point to a ring - but you get a ring, because you've a collection of rings here. Is that possible? It's just a guess, I didn't compile your code. |
Hey, @barendgehrels will you please review this PR, as I have implemented wkt read feature for polyhedral surfaces and I will also implement wkt write feature in a new PR, once this one gets merged. |
@Siddharth-coder13 Somehow this PR #789 and PR #782 have become a mess and they both contain the same commits (e.g. |
Actually, this PR is the most current version of my local repo, it contains all the changes I have done so far, since my last PR doesn't gets merged, so I have to make a new branch in order to contribute.
|
It's unfortunate. The 1 needs to be in the other separate PR and 2+3 as a separate feature in this PR. I understand this may make your Git operations less convenient but these two are distinct features, they should be treated independently. |
Yeah, I know both the things are different, using git sometimes is quite complex. I guess the only thing I can do now is to revert 1, and then this PR will become a separate PR for polyhedral surfaces. You may review 2 and 3 if you have any changes to suggest, as I want this PR to get merged, for 1 I will submit another PR after discussion if Z is to add or not. |
I'd strongly suggest to not to close any existing PRs and to not to open any new PRs.
This should be in the PR #782
These two should be in this PR #789 Since there should be no dependency between the two PRs, that is, the changes in #782 are not required by the #789, then I'd simply suggest to:
By the way, this situation should also give you why it is a bad idea to develop any contributions in your fork's |
@Siddharth-coder13 as an addition to the excellent comments from @mloskot you could also have a look at https://github.com/boostorg/geometry/wiki/Contribution-Tutorial |
80442cb
to
b551f6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nit-picks
include/boost/geometry/geometries/concepts/PolyhedralSurface_concept.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/geometries/concepts/PolyhedralSurface_concept.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/geometries/concepts/PolyhedralSurface_concept.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/geometries/concepts/PolyhedralSurface_concept.hpp
Outdated
Show resolved
Hide resolved
include/boost/geometry/geometries/concepts/PolyhedralSurface_concept.hpp
Outdated
Show resolved
Hide resolved
Hey @mloskot, I think this PR is able to merge now. |
or suggest changes if any. |
It’s big, important and needs to be reviewed and approved properly, by multiple people. |
@Siddharth-coder13 Please, hit the "Resolve conversation" for the items that you have addressed. This way, we can see what's been done and what's not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
It's much better, but not ready yet. See my comments.
As asked earlier:
Can you add
-
a unit test, specifically for the polyhedron itself (without wkt)
a: create it from scratch
b: modify it
c: iterate through it
d: clear it
e: check the concepts
such that we can really see how it works internally? -
a unit test, for the wkt support of the polyhedral surface, to read it and to write it?
-
documentation including an example for the user (
doc/src/examples/geometries/polyhedral_surface.cpp
)
And please check yourself for consistencies in capitilization of filenames and struct/class names, for the style (I know in the current code we've many places where C++11
is not yet applied - but now we can)
Hey, @barendgehrels Thanks for your reviews, but I think this point needs a separate PR, because a single PR can not define everything about polyhedral surfaces.
I will do the rest changes asap. |
Thank you! Success! |
Hello @mloskot @barendgehrels @vissarion @awulkiew, as I have extended this PR as a project in GSOC'21 including other features of polyhedral surfaces. I want to get this PR merged before the GSOC period starts. It would be of great help if you leave your comment on this. |
Hi @Siddharth-coder13 , sorry for the late reply. I think it looks good now! There is no unit test yet, did you plan to do that in this PR or in a next one (along with the documentation)? Documentation we can do later but a unit test is normally included in a PR (especially for new stuff). |
Hi @barendgehrels , thanks for the review. Actually, I added the example file of polyhedral surfaces, if unit test is needed in this PR, then I will add it asap. |
As suggested by @barendgehrels and @vissarion in comments #683, I defined geometry for polyhedral surfaces.