Part 1 of 2
TODO: UV Unwrap geometry node
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
I've voiced my skepticism about requiring BMesh for UV unwrapping already in chat. I'm still unhappy with that, but if that's required for your changes, it is what it is.
The description is quite lacking in the reasoning for the change and the context. I'd expect to at least have some sort of design task specifying the goals of your refactor and the connection to this particular change.
| source/blender/nodes/geometry/nodes/node_geo_uv_pack_islands.cc | ||
|---|---|---|
| 8 | Geometry nodes aren't meant to depend on any code from the editors modules. I'm actually surprised this works. | |
| 80 | for (const int i : polys.index_range()) { | |
| 93 | struct is unnecessary in C++ | |
| 93–102 | Designated initializers don't compile on windows | |
Comment Actions
I have the same questions as @Hans Goudey (HooglyBoogly). This bmesh implemention is slower and has more code, so without more context it's unclear why it's better.
Comment Actions
From reading #geometry-nodes chat my understanding is that the move to bmesh is to support edges with > 2 faces.
But you can't correctly UV unwrap those without overlapping faces, unless you add a seam. So those edges can be split when going into UV parametrizer data structures, always considering these as having a seam.
Comment Actions
Heyas! A couple quick clarifications.
The current code is about UV Packing, not unwrapping. If some parts of the UV's are overlapping or invalid, or non-manifold, we still need to pack the rest of them as best we can anyway.
The original packing code was duplicated (forked) in https://developer.blender.org/rB9296ba867462f7ff3c55bc0c9129af4121243bed / T82637 . The motivation for the *fork* was to support edges with > 2 faces.
Those two packing engines have diverged over time. The motivation for the current change is to deprecate one of the packing engines so we can remove it and reduce all the duplicate code, moving forward with one packing engine instead of two.
TL;DR: The move to BMesh for UV Packing already happened two years ago. This is about unifying the API to improve maintainability moving forward.
Comment Actions
Ok, I personally would not have re-implemented packing in bmesh to fix the > 2 edges case, but rather changed the existing implementation to support it. But this is really the domain of the modeling and geometry nodes owners, so will besides giving my opinion will leave the opinion to them.
Eventually I think the UV packing should be improved to take into account island shape and pack much more tightly, which would already make it much slower and bmesh overhead may not be that significant.
Comment Actions
Putting this back in "Requested changes" state for mostly the same reasons as I mentioned in my first comment.