8000 [Idea] Add span support by pr0g · Pull Request #50 · jhasse/poly2tri · GitHub
[go: up one dir, main page]
Skip to content

[Idea] Add span support #50

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
Draft
pr0g wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

[Idea] Add span support #50

pr0g wants to merge 3 commits into from

Conversation

@pr0g
Copy link
@pr0g pr0g commented Apr 23, 2023

This is a speculative/demonstrative PR to show how the poly2tri API could be potentially improved upon by accepting a span of p2t::Point instead of a std::vector<p2t::Point*>.

In this case, the span implementation is taken from the Guidelines Support Library (GSL) from Microsoft, but it could also use std::span if poly2tri was updated to use C++ 20.

In my particular use case, I needed to create a vector of p2t::Point types, and an additional 'view' vector of p2t::Point*. By using a span, the semantics of using a vector of pointers remains the same, but you don't need to allocate a new container of pointers, you can just use the original one. You also can avoid having to new each point (as is done in the testbed example and unittest project).

I realize this is a breaking API change and it's possible there are some downsides I'm not considering, but for my particular use case, it made things a lot cleaner and simpler (and more efficient).

I'd definitely be interested to hear what people think.

Thanks!

@pr0g pr0g marked this pull request as dra 9132 ft April 23, 2023 19:18
@pr0g
Copy link
Author
pr0g commented May 14, 2023

I believe the reason the build failed for this before was the minimum required version of CMake (3.12) did not support FetchContent. I've bumped this to 3.15 and I think it should now work correctly (but again merging this as-is likely isn't a good idea, I was just interested if this alternative API might be useful in certain situations). Thanks!

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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@pr0g
0