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.

Sign up for GitHub

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

Jump to bottom

Two new star shapes #1325

Merged
merged 10 commits into from
Aug 23, 2018
Merged

Two new star shapes #1325

merged 10 commits into from
Aug 23, 2018

Conversation

akrah
Copy link
Contributor

@akrah akrah commented Jun 13, 2018 •
edited
Loading

PR Description

We propose two new star shapes: Astroid and Lemniscate.

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

@dcoeurjo
Copy link
Member

Thanks @akrah
could you please update the doc and the changelog before I review this PR?

@akrah
Copy link
Contributor Author

akrah commented Jun 20, 2018

Two questions:
- Should I merge the master branch into this branch or I let you deal with that?
- Could you detail which file is concerned by the doc? Doxygen comments are still in the .h and .ih files, I don't find informations about how to update the doc. Have you a README or tutorial about the documentation update?

@dcoeurjo
Copy link
Member

Please merge the master to this branch yourself (and fix the possible conflicts;)).
For the doc, I think you should have a look to moduleShape.dox

@dcoeurjo
Copy link
Member

thx for the PR BTW

@akrah
Copy link
Contributor Author

akrah commented Aug 6, 2018

Merge conflicts done.
Concerning moduleShape.dox, I just added my name, there is no real documentation about the precise content of the module to complete.

@dcoeurjo
Copy link
Member

It looks like you’ve only added your name to the package, the module is missing.
Btw, you could add snapshots in the module page. The shape names are not really explicit and a visualization would help.

Thx

@akrah
- Add Astroid and Lemniscate to the Shape Factory
2771c33 
- Bug fix in Lemniscate and Astroid about the declaration of Point2D
- Add figure of Lemniscate and Astroid in moduleShape.dox
- Delete double entries in moduleShape.dox
- Add new example in doc: exampleParametricShapes.cpp
@akrah
Merge branch 'master' of https://github.com/DGtal-team/DGtal into New…
1358697 
…StarShapes
@akrah
Copy link
Contributor Author

akrah commented Aug 14, 2018

Right, sorry, I didn't see the file moduleShape.dox.
So, I updated this file, corrected a wrong declaration and added an example file.
Do you think all is ok?

@dcoeurjo
Copy link
Member

Thanks. It looks like there is an issue while generating the doc. Could you please have a look ?

@akrah
Copy link
Contributor Author

akrah commented Aug 23, 2018

@dcoeurjo All seems ok ,it was a just a typo in a path.

@dcoeurjo
Copy link
Member

perfect.thx

@dcoeurjo
Copy link
Member

Merging

@dcoeurjo dcoeurjo merged commit 033fee2 into DGtal-team:master Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Shapes waiting author feedbacks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akrah @dcoeurjo