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

Added polylabel article, updated README #76

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

Conversation

hollasch
Copy link
Contributor

@hollasch hollasch commented Dec 7, 2020

Article copied from the original post on the Mapbox blog:
https://blog.mapbox.com/a-new-algorithm-for-finding-a-visual-center-of-a-polygon-7c77e6492fbc
This includes a new directory, images/, to hold the article figures.
Some very light editing has also been done.

Also updated the project README to reference the article. Cleaned up the
formatting a bit to be useable in plaintext form.

Article copied from the original post on the Mapbox blog:
https://blog.mapbox.com/a-new-algorithm-for-finding-a-visual-center-of-a-polygon-7c77e6492fbc
This includes a new directory, images/, to hold the article figures.
Some very light editing has also been done.

Also updated the project README to reference the article. Cleaned up the
formatting a bit to be useable in plaintext form.
@hollasch
Copy link
Contributor Author

hollasch commented Dec 7, 2020

Forgot to mention that the article was converted to Markdeep, which is a Markdown-like super-variant that loads and browses just like a regular HTML file, but supports a lot of extra features. This includes the ability to freely style the article with standard CSS — I've just used the default styling for this submission.

@mourner
Copy link
Member

mourner commented Dec 7, 2020

Thanks for the PR! However you haven't indicated in the description why the article needs to be copied to the repo. Anything in particular that prompted this?

@hollasch
Copy link
Contributor Author

hollasch commented Dec 7, 2020

Yeah, I should really have created an issue first to get your feedback, but oh well. Don't feel like you need to take this.

My thinking is that the article is nowhere mentioned in the project or its README, and the project would greatly benefit from the additional explanation. I also decided to include the article to make it easy for users to reference it directly out of their clones as a local resource.

Finally, in going through it this weekend, I feel that there are pieces that could use more explanation or documentation. For example, I reverse-engineered the point-segment distance function to find it's just doing a projection of the point onto the line containing the segment to get a ray t parameter — that could use more explanation, but it's not huge.

More mysterious is the inside flag toggling equation, and what that's actually doing. Nowhere in the code is the input polygon explained, but it looks like a polygon is a set of rings, in arbitrary order, with arbitrary vertex winding order, and where the interior test is purely driven by edge-crossing count. I'm still puzzling out the inside/outside flag tracking.

All this to say that as people who use the library feed back aspects that could use more explanation, you could add these comments to a wiki, or the README, or code comments, or some combination, but the blog post that really provides the overview is read-only and static. Moving it into the project allows the approach to refine or evolve, and hosts a more "living document" approach than a now four-year-old article.

In the end, I just figured I'd sketch out a PR as much as a question as a something you might accept. I maintain open source myself, so you won't hurt my feelings at all if you decide to table this for now.

@hollasch
Copy link
Contributor Author

hollasch commented Dec 7, 2020

Oh — I should also mention that I'm playing with a spheroidal approach, using either cubic or icosohedral subdivision of the sphere. If it's fruitful, it would be good to augment this project.

And another final thought is that the documentation in the project needn't be a verbatim copy of the blog article, though it would look very much like it.

@TWiStErRob
Copy link

Have you considered a /docs folder or gh-pages branch? There's already a lot of files in the root directory.

@hollasch
Copy link
Contributor Author

hollasch commented Dec 8, 2020

Absolutely. You could certainly consider this an increment to a web-hosted project page. Trying small bites. :)

@hollasch
Copy link
Contributor Author

hollasch commented Dec 8, 2020

BTW, I can do the set up for site hosting if that's interesting to you. I'm one of the admins on the Ray Tracing in One Weekend site, so am very familiar with the process.

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