-
Notifications
You must be signed in to change notification settings - Fork 153
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
Standard Iterator Categories for RangeSetBase #76
Comments
At the high level I think this sounds great, and would be very happy to see changes like this! I'm not too knowledgable about the different tags and categories, but I think forward and bidirectional should both be easy (whereas random access is not easy). I have a vague memory that when I initially set up the iterator classes, there was a blocker that made it difficult to satisfy the stl iterator requirements. Perhaps that the stl iterators expect a For const-correctness, yes please! The more const the better! Const containers eluded me initially as mentioned in #66, but I'm sure it's doable. And more generally, there are likely other places a const can be dropped in. Even if it requires some redesign, those redesigns are usually good improvements. A related thought: I'm always deeply curious about inlining/optimization, and when we pay a price for using |
Hey Nicholas, |
Design comments are very welcome! As with all projects the library is far from perfect, and these conversations can always influence future changes :) Actually, thinking about this some more, what we implement is very similar to the common paradigm you describe. Geometry-central does already define set types and iterator logic for each kind of traversal. The classes are declared in halfedge_element_types.h, with the logic in halfedge_element_types.ipp. However, just looking at those it's not clear that they fulfill the iterator paradigm, because they are actually just template substitutions for the Soooooo.... perhaps I need to think a bit more about what the actual blocker is to using these as STL iterators. There's even a comment about STL iterators from past-me at the top of |
Yeah. In years past I used to rely pretty heavily on boost for random stuff until it just became way too heavy to be of use. Having said that, are you familar with boost's iterator_facade template? It might help guide how to write a STL friendly iterator in terms of the necessary type traits, etc... Honestly, I think all the type trait decorations are primarily what's missing, but I could be wrong. I do now see your comment in element_iterators.h, but I'm not sure I'm following what you were getting when you said that an iterator expects a container to have one set of data over which to iterate? I don't think the iterator concepts are quite that restrictive. I don't even think you need a container to have an iterator. As long as |
Yeah, let's assume that that comment in the source code is a stale comment and not true :) Just adding type traits sounds reasonable to me! And as long as it works, I don't really think there's really any downside either. |
Hi Folks,
Would it make sense to add stl iterator_categories (and all the other sundry stl iterator typedefs) for the RangeSetBase class? There are times when I would like to use stl algorithms on some entitie of the mesh, like:
std::transform(begin(mesh.faces()), end(mesh.faces()),...)
But, it seems we need category support for that. My guess is that
std::forward_iterator_tag
would be the most likely candidate, but maybestd::bidirectional_iterator_tag
would work too?Ultimately, I would like to be able to use the parallel version of the stl algorithms, which I currently do in some code. But, I end up first copying the entity handles to a
std::vector
with a range based for loop, and then use that sequence as my iteration range for the algorithm. From a performance standpoint, the penalty of copying is probably in the noise for cases where parallelizing things is a benefit, but it still seems wasteful. I only bring that up because I don't quite know all the details for how the parallel versions of the std algorithms partition things, etc... Therefore, I don't know how smart/safe/etc... a given iterator/container pair need to actually be to work correctly in the parallel versions of the algorithms. A vector of handles seems to work fine, but obviously that is the least restrictive container/iterator type.As a last thought, this may also play into enhancing general const-correctness for the overall api as well. However, I seem to gather from #66 it is not as trival as sprinkling
const
everywhere.The text was updated successfully, but these errors were encountered: