In the gui it's still capped at 64 characters (at least it *should* be if I didn't make any mistakes since...) . Though I must admit I don't have a clear mental model of all the ways people could set these names.

I'll have a better look at that. We do need to have this constant available in the source though, so enforcing the max length of 64 should not be based on this constant.

It's an ugly situation. And by far my least favourite part of the patch. But all other solutions of linking attributes I came up with were a lot more complicated.

I agree some documentation would be nice. However.. I though a doc-string was python documentation? This function is not callable from python. I think a comment would suffice?

Will do.

This is indeed a murky area of the python api, with the python objects often having raw pointers pointing inside the customdata layers. The original ptr->data can easily be invalidated as well if the uv layer is reallocated in some way between getting the BPY_MLoopUV object and using it. :-(

Hans is not gonna like it ;-) , but indeed better to be consistent.

I added back in the report.
btw, wouldn't it be better to change it to

"UV map '%s" not found"

as Texture Layer isn't really the correct term anymore I think?

Will change.

Some (lots of, actually) files were moved to C++ *after* I started the patch. I suspect this is one of them.

You are completely right, I somehow had confused it with it as Bpy_MLoopUV.

I must admit I didn't understand much of the python API stuff when creating this. And it's still not 100% clear to me how everything fits together. So this code is the result of progressive search&replace from the original code to keep the same outcome for each change I made.

It could undoubtedly be more efficient.

Will try to find out how to do that...

Oops, probably written at night when I couldn't decide between <= -5 or < -4

I don't really agree, but will change.

Oh. hm. it's C, so no pass by const reference.

Still... isn't this something better left to the compiler these days?

Oh, hm. I now see your other comment that it's apparently not (anymore?) capped to 64 in the gui.
I'll need to look into that.

(urg, this should go *above the 'NOTE:' comment as it's a replay to the one above that. But as I wrote it later phab decides to put it below everything)

They should never be NULL when accessed/needed but created on-demand eventually. So probably I should add more asserts/ensure() calls.

My first plan was to have the lazy-allocation of the bool layers also work in BMesh (like it works in Mesh). But since allocating CustomData layers on demand in BMesh is
a) expensive
b) re-allocates layers/ rebuilds blocks, thereby invalidating existing python objects.
I decided against that for this first version.
So currently when converting a Mesh to a BMesh the bool layers are always created. (and removed if 'all false' when converting back to Mesh). So at the moment they can never be NULL.

So strictly speaking all the asserts and ensure() calls are currently not needed. However, it would be very nice if we could extend the lazy allocation to BMesh, since for a lot of BMesh uses the bool layers aren't needed, therefor I decided to keep the assert/ ensure.

For what it's worth, I agree that the abbreviations are unnecessary and make the code harder to read.
In this case it makes it seem like the function does something totally different too-- selecting something instead of just creating the selection attribute.

This struct is very small, it's just four integers-- the same size as Span and other structs we typically pass by value. It makes the code much simpler to pass by value, especially C code without the concept of references. So I'm in favor of passing by value here.

I'm pretty sure it's an edge selection stored on corners because you can select split edges in the UV editor even if they actually come from the same edge on the mesh.

Is it the intention to deprecate BMLoopUV use? (in favor of accessing primitive data).

We didn't talk about that yet. I think it would be good to deprecate BMLoopUV in 4.0 so we can simplify these things in the future.
Eventually we should also change the API so that scripts can add one of the boolean attributes without invalidating all current Python objects referencing BMesh attribute values.
I'm not sure how that would be done yet, but it must be possible.
The last thing we'd need is a more intuitive way to find the name of the boolean sub-attributes. Some Mesh/BMesh helper function would be enough for that IMO.

If UV's should not be accessed via BPy_BMLoopUV, shouldn't Python templates/example code be updated? (or noted as a TODO).

Yes, that might be nice, I think it's fine for that to be a separate patch though.

Agreed that it's worth keeping the asserts and "ensure" calls in BMesh code to ease the transition to lazily creating the boolean layers in the future.

If there is no vertex selection layer, the element is just not selected, it's not an error.
Though this won't be reachable currently because the boolean attributes are always created, it's planned in a next step to create the attributes on-demand.

This is fine right now, since the boolean attributes are always created for BMesh. It will have to change in the future when they're created on-demand though.

Actually I can't, since these functions are used in mesh_data.cc so they should be public.

True! These are 'face edges' not 'mesh edges'.

Still wouldn't hurt to document it maybe. I'll have a go a adding a comment.

Currently anything creating new CustomData layers in a BMesh causes all python objects having pointers into that same CustomData set (domain) to have invalid pointers which can possibly crash blender.

That is something that would need to be changed before we could do lazy allocation.

Apparently I had tested it the wrong way. The layername sizes should now be capped at 64 from a user point of view (including when blender appends a sequence numer to make the names unique).

Not the most elegant solution but I couldn't think of a better one.

maybe MAX_CUSTOMDATA_LAYER_NAME_GUI should be MAX_CUSTOMDATA_LAYERNAME_EXTERNAL ?

The new names are confusing but I don't think this relates especially to select/selection terms.
The uv_index is also ambiguous as mentioned in the last review.

Suggest:

• ED_mesh_uv_map_vert_select_layer_ensure(mesh, uv_layer_index)
• ED_mesh_uv_map_vert_select_layer_get(mesh, uv_layer_index)
• ED_mesh_uv_map_vert_pin_layer_ensure(mesh, uv_layer_index)
• ED_mesh_uv_map_vert_pin_layer_get(mesh, uv_layer_index)

... etc.

I was/am concerned with this adding overhead to draw code. for e.g. this passing 16 byte argument to drawing functions that run on every face & loop (updating edit-mesh draw data), even when the mesh doesn't have UV's (just to check the value is -1 and returning in some cases).

However I couldn't measure a performance difference so I suppose it's OK, it would be good to have a rule of thumb for whats reasonable to pass by value (noticed larger structs being passed by value creeping into our code recently), although some seem like mistakes too.

In this case they should be moved to ED_mesh API (source/blender/editors/mesh/mesh_data.cc seems appropriate).

This is fine but it's information that should be in code-comments, just a brief note on the current state and how it may change in future. Otherwise the reference to legacy #MLoopUV type reads like an outdated comment that should be removed as MLoopUV isn't referenced at all.

+1, in that case a comment noting these may be lazily allocated in the future would be good to include.

That's true for reading the value, for assigning, enabling the selection will be ignored and should raise an exception.

But they are in source/blender/editors/mesh/mesh_data.cc ??

Ah! Was jumping between this an rna_mesh.c & got mixed up, sorry for noise.

To me (maybe because english is not my native language) select still looks like a verb in those names and I don't really understand why it would be bad to add the 'ion' to make it absolutely clear. The word order doesn't really help to make the distinction either , as some parts of blender use english grammar order and other parts use 'yoda' order so...

I also have my doubts about uv_layer_index. In the CustomData code the layer_index signifies the index in *all* the layers (of all types) within the domain, while this is not really the layer_index, but the uv_map index (i.e. the n'th uv map). I do agree uv_index is confusing. I propose uv_map_index .

But it's not important enough to me to keep discussing it. At least not within the scope of this patch. That discussion would be better done on the style guide task maybe. And also it's probably more important to be consistent with the surrounding code.

I'll rename them to these new names. using uv_map_index instead of uv_layer_index , let me know if you disagree.

As an aside, some studying of generated code in godbolt made me think passing by reference is not a certified win either.

For small(ish) functions gcc -O3 was inlined when passed by value (struct with 4 integers), but not when passed by reference. Also when passing by reference extra instructions were generated to move the referenced value into registers to work on them, while in the pass-by-value they already were in registers.

I guess it depends on register-pressure which is faster, but imo without profiling it's not really possible to tell which is actually better. So to me passing this by reference/pointer feels like 'premature optimisation' .

Though I must admit I need to suppress an urge to do it anyway, since for me having learned C in the early 90s passing 128 bytes *on the stack* feels like heresy :-D

Even when having a BMLoop I still need the offsets...

I added a clarifying comment. Also explaining the plan is to add direct access to the bool layers.
I prefer not to make that part of this patch though. I propose to work on that as a separate patch.

I added a python exception, but left the assert in for debugmode builds.

Right, not an issue with this patch but wouldn't hurt to change it here.

It would be good to have a doc-strings here describing constraints on these names, for e.g. is it essential these never exceed 2 bytes? (MAX_CUSTOMDATA_LAYER_NAME could be referenced too).

STRPREFIX macro could be used instead of STREQLEN, avoids assumptions about length of defines.

Correction, as these are prefixes not extensions it could be MAX_CUSTOMDATA_LAYER_NAME_NO_PREFIX.

Maybe like this?

As this is being done elsewhere it would be better to make into a shared function (my patch added bke_id_attribute_name_max_length_calc), so that could be made public and reused.

I added this to CustomData, since the limitation really comes from there, and I thought it was important to maintain the fact that attributes are implemented with CustomData and not the other way around.