Hello,
I did this rewriting of get_uvs() function to correctly merge the uvs.
Previously would merge all uv points with exact same coordinates with no regard to actual topology, thus merging superposed uv islands.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
@Sybren A. Stüvel (sybren), could you look into this? I have neither the Blender sources nor my Alembic files on my computer at the moment. Overall, apart from some style issues it looks good to me.
(On a side note, some loops (here and elsewhere in the Alembic code) could be parallelized.)
Comment Actions
This patch appears to be against an old version of the code, please update it to latest master.
| source/blender/alembic/intern/abc_customdata.cc | ||
|---|---|---|
| 56 | I have a few questions: Why go from unordered_map to vector-of-vectors? When do you expect the limit of int to be hit here, and not be large enough to warrant a long? Adding a comment to explain what is contained in the type is a good idea. However, I don't think it explains it well -- which index into which vector is the vertex ID, and which covers the "more than one"? What is exactly in the uint64_t and what in the long? | |
| 114 | Instead of those two lines you can just do vertex_index_map idx_map(config.totvert); | |
| 117 | I'm all for more comments in Blender's source code, but they should add to what is already pretty clear in the code. Looping until num_poly already suggests that you're looping over all polygons. The same goes for current_poly = polygons[i]; the code is already clear that that is getting the current polygon. | |
| 121 | As an example of comments that I miss, here I would welcome a comment that explains why the polys are looped over in reverse order. Of course this wasn't in the original code either, so it's not something you have to add now, but if you know please add ;-) | |
| 128 | Thanks for renaming k to something way more expressive. | |
| 134 | Here you do a linear search through the std::vector. This is more efficiently done with a std::map or std::unordered_map. From what I can see the ordering of the vector isn't relevant, so I'd use an std::unordered_map here and reduce the lookup complexity from O(n) to O(1). However, if you've done some benchmarks of which approach is faster, and this one won, please let us know. | |
| source/blender/alembic/intern/abc_customdata.cc | ||
|---|---|---|
| 56 | I can't edit an inline comment -- in the 3rd sentence remove "not be large enough to" to make it understandable ;-) | |
| source/blender/alembic/intern/abc_customdata.cc | ||
|---|---|---|
| 56 | I agree this is not that much clear. The first vector represents the vertex ids. I used a vector instead of an unordered map because we don't need to search in it, vertex ids are the vector indices. Since the long is the index of the uv coordinates, with really big mesh it might one day be too short as an int. | |
| 114 | Yes I could. | |
| 117 | I agree they're not really useful^^ | |
| 134 | Because this vector represents the different uv position that a single vertex can have (because of seams), its size will unlikely be more than 5 or 6 elements, so I'm not sure if this is that much important. | |
| source/blender/alembic/intern/abc_customdata.cc | ||
|---|---|---|
| 134 | It's fine to use a linear lookup, for most meshes this will be faster. The main cause of slowness will be memory allocations, which is not great in either case but acceptable. | |
| source/blender/alembic/intern/abc_customdata.cc | ||
|---|---|---|
| 134 | As an anecdote about unordered_map vs. vector : I am currently working on a compiler for my own native programming langage, and when compiling functions I used to have an unordered_map for storing data about local variables (including function parameters). Then after some profiling I turned the unordered_map into a vector which ended up being faster. The reason being that most functions have few local variables meaning that looking up data in a small cache coherent vector was here much faster that hopping through the pointers indirection inherent to unordered_map. And here this is the same : most polygons will have 3 to 4 vertices, and most polygon edges will be part of one UV island only, so small vectors should not be a problem. So I would say that indeed the vector of vectors might be faster than a single unordered_map. Previously we had a single vector for all the UV data, so a map was bound to be faster than an algorithm that keeps iterating over a very large vector. Then, we are supposed to be computer scientist, so we should time and profile and not suppose! :) | |
| source/blender/alembic/intern/abc_customdata.cc | ||
|---|---|---|
| 121 | Cedric told me it was probably a copy-paste remnant. Reverse looping was used to get the correct winding order of the polygons, but it has probably no means here. | |
Comment Actions
This is more complicated than it needs to be, there is no need for UV hashing if we are going to compare them directly.
We also do not use long, because its size is platform dependent.