CustomData layers are exposed to the python api with the help of the DEFINE_CUSTOMDATA_LAYER_COLLECTION macro in rna_mesh_utils.h. Which boils down to wrapping a pointer to the CustomDataLayer struct describing the layer. However, when inserting layers in CustomData the array of CustomDataLayer structs is always kept ordered by layer type, which means the CustomDataLayer struct’s address is changed. This means that any time you insert a CustomData layer all existing python references to CustomDataLayer structs (with a type enum higher than the one inserted) now point to the wrong layer, leading to erroneous results and potentially crashes.
This bug has always existed as far as I can see, but with the mesh SoA refactor it has become much more visible because there are now more types of layers and the type ordering has changed.
I don’t really see a way to fix this in a backwards compatible way, so for 3.6 I suppose we’d need to document this as a ‘known limitation’ , though that’s far from an ideal solution.
For 4.0 I guess it would be better to completely overhaul the CustomDataLayer access. Preferably rewrite it in the context of (name based) Attribute access.
I do think many of the custom data layer API points can be removed, since they’re now redundant with the attribute API. And in this situation, moving the face corner normals cache out of CustomData should do the trick to. But generally having such an easy way to create dangling pointers isn’t great.
I also fear the attribute API has the same problem, since it’s also just a wrapper around CustomDataLayer. I wonder if we should change the Python API for attirbutes a bit more aggressively, to more closely math AttributeReader and AttributeWriter from the C++ code. It would be nice to expose the const-correctness as well, since currently any access of attributes from Python has to make the layer mutable (often meaning a copy). Exposing implicit sharing would also make sense IMO.
Yes this is what I meant. Having something like an AttributeIdRef (which is name based I think? ) instead of a direct pointer to (some incarnation of) the data.
One problem is, as far as I can see, you need to pass stuff to python using PointerRNA, which has a data pointer which needs to point to something persistent on the C(++) side. You can’t really allocate metadata specifically for the PointerRNA, because it would to be deleted at some point as well.
I would think the issue is more than only ordering, but also reallocating the array itself which changes the memory address anyway? So you would need to make a it a list?
If we need a workaround for 3.6 then maybe we could somehow remove the ordering requirement, and make it so there is always space allocated for at least e.g. 8 layers so it’s unlikely to change. I’m not sure if that kind of hack is better than what you had in mind, I don’t have a clean solution that is forward compatible.
a lot of CustomData code assumes layers are ordered by type
Like you said reallocating is a problem as well.
I think just adding a fixed list could be a workaround for 3.6 though I think 8 layers is a bit too small. With UVs now taking up 4 data layers per UV layer, positions being a separate data layer etc etc. The number of attribute layers can grow quite quickly.
I’d think 32 (or even 64) would be a better ‘maximum’. That would add 64 pointers to each CustomData struct, and one integer to each CustomDataLayer struct. But it would not be a complete fix, as when you ever (even temporarily) have more than 64 CustomData layers you’d need to realloc the table and then all python CustomData references (so uv layers and attributes) would be invalidated. I don’t know if there’s some data structure in blender with an array interface which can grow without reallocing ?
Though I’m not completely convinced it is the way to go, as it is still a somewhat complicated hack for a temporary band-aid ‘solution’. Also I haven’t completely found out if I can for example override ‘as_pointer’ and how…