Mesh vertex internal naming (especially position data) #101688

New Issue

Hans Goudey · 2022-10-07T23:26:11+02:00

Hans Goudey commented

2022-10-07 23:26:11 +02:00

D15982 and related changes in #93602 have made it clear that the naming of mesh vertex elements has to be reconsidered.
After that patch lands, MVert won't be used at all. "Vertices" won't exist at all in the code.
Instead, we refer to various data stored on the point/vertex domain in separate arrays.
Each of these arrays needs to have a name, and sometimes single elements could have consistent naming guidelines as well.

Currently most mesh attribute arrays are named after the corresponding attribute.
For example, we use hide_vert for the ".hide_vert" attribute, select_vert for the ".select_vert" attribute, and so on.

Since the mesh position is called "position", I would propose referring to it with the positions variable name.
This means internal naming is consistent with the UI, which reduces overhead when talking about the code.
It also helps to build the abstraction of attributes, where we don't think of positions as vertices but just as an attribute.

Position data should usually be accessed with Mesh.positions() or Mesh.positions_for_write(), and passed around with spans.

In code we usually don't pass around one mesh position at a time. Instead, access usuallys look like this:

Span<float3> positions = ...
do_something(positions[vert_i]);

Functions currently using coords (as in "vert coords") should be change to positions instead.
Conceptually a vertex doesn't correspond to a single attribute on the vertex domain, so the same index variable (often vert_index or vert_i)
can be used to retrieve any attribute.

Abbreviations like co or p1, etc. can still be used, but for clarity (less state tracking, etc), we can favor the use of direct access with indices.

[D15982](https://archive.blender.org/developer/D15982) and related changes in #93602 have made it clear that the naming of mesh vertex elements has to be reconsidered. After that patch lands, `MVert` won't be used at all. "Vertices" won't exist at all in the code. Instead, we refer to various data stored on the point/vertex domain in separate arrays. Each of these arrays needs to have a name, and sometimes single elements could have consistent naming guidelines as well. Currently most mesh attribute arrays are named after the corresponding attribute. For example, we use `hide_vert` for the `".hide_vert"` attribute, `select_vert` for the `".select_vert"` attribute, and so on. --- Since the mesh position is called `"position"`, I would propose referring to it with the `positions` variable name. This means internal naming is consistent with the UI, which reduces overhead when talking about the code. It also helps to build the abstraction of attributes, where we don't think of positions as *vertices* but just as an attribute. Position data should usually be accessed with `Mesh.positions()` or `Mesh.positions_for_write()`, and passed around with spans. In code we usually don't pass around one mesh position at a time. Instead, access usuallys look like this: ``` Span<float3> positions = ... do_something(positions[vert_i]); ``` Functions currently using `coords` (as in "vert coords") should be change to `positions` instead. Conceptually a *vertex* doesn't correspond to a single attribute on the vertex domain, so the same index variable (often `vert_index` or `vert_i`) can be used to retrieve any attribute. Abbreviations like `co` or `p1`, etc. can still be used, but for clarity (less state tracking, etc), we can favor the use of direct access with indices.

Hans Goudey commented

2022-10-07 23:26:11 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Hans Goudey commented

2022-10-07 23:26:11 +02:00

Added subscribers: @HooglyBoogly, @ideasman42, @JacquesLucke

Campbell Barton commented

2022-10-11 05:04:49 +02:00

I'm not so keen on using term "positions" in isolation, mainly because I think it will lead to more ambiguous naming.

Firstly, I don't think it's necessary to follow naming from ID-attributes in general.
The link to vertices is important, even if the data represents positions, knowing the array is vertex aligned is important when used with other vertex data (typically normals).
"position" is a very generic term, similar to "vector", so if a function takes an argument "positions" it's more ambiguous than having vert_* prefix which is more obviously a vertex aligned data.
Naming choices tends to leak into other variable names in ways we can't easily control.

There are many places we refer to single indices, positions & normals. Currently references to this data lends it's self to abbreviations `v_index`, `v_co` & `v_no`. Even if we document preferred alternative abbreviations, I'm not convinced they would be followed *(we already have trouble following/enforcing existing naming conventions).*

Calling vertex "positions" lends it's self to `p_*` and `pos_*` prefixes. `p_` prefix can be confused with polygon access and look like they might be accidental if some code that handled positions ends up referencing vertex normals for e.g. Using a `pos_*` prefix is less ambiguous although it can be confused with the result of `GPU_vertformat_attr_add` which is almost always uses "pos" and used in `./source/blender/draw/intern/mesh_extractors/` which handles mesh data.

Less disruptive naming for D15982 could be to user the identifier "vert_coords", e.g: mesh.vert_coords() & BKE_mesh_vert_coords() this means we would mostly keep existing naming.

Existing abbreviations v_co, v_no & v_index are an obvious fit and we don't need to replace them with names that aren't are more likely to be confused with other variables.

An alternative could to call them "vert_positions" instead of just "positions", which at least fits with "vert_normals", developers are more likely to use v_ prefix related variables so it avoids most of the down-sides mentioned about using "positions" on it's own, although I'd prefer stick to the current, shorter naming.

I'm not so keen on using term "positions" in isolation, mainly because I think it will lead to more ambiguous naming. - Firstly, I don't think it's necessary to follow naming from ID-attributes in general. - The link to vertices is important, even if the data represents positions, knowing the array is vertex aligned is important when used with other vertex data (typically normals). - "position" is a very generic term, similar to "vector", so if a function takes an argument "positions" it's more ambiguous than having `vert_*` prefix which is more obviously a vertex aligned data. - Naming choices tends to leak into other variable names in ways we can't easily control. ``` There are many places we refer to single indices, positions & normals. Currently references to this data lends it's self to abbreviations `v_index`, `v_co` & `v_no`. Even if we document preferred alternative abbreviations, I'm not convinced they would be followed *(we already have trouble following/enforcing existing naming conventions).* ``` ``` Calling vertex "positions" lends it's self to `p_*` and `pos_*` prefixes. `p_` prefix can be confused with polygon access and look like they might be accidental if some code that handled positions ends up referencing vertex normals for e.g. Using a `pos_*` prefix is less ambiguous although it can be confused with the result of `GPU_vertformat_attr_add` which is almost always uses "pos" and used in `./source/blender/draw/intern/mesh_extractors/` which handles mesh data. ``` ---- Less disruptive naming for [D15982](https://archive.blender.org/developer/D15982) could be to user the identifier "vert_coords", e.g: `mesh.vert_coords()` & `BKE_mesh_vert_coords()` this means we would mostly keep existing naming. Existing abbreviations `v_co`, `v_no` & `v_index` are an obvious fit and we don't need to replace them with names that aren't are more likely to be confused with other variables. ---- An alternative could to call them "vert_positions" instead of just "positions", which at least fits with "vert_normals", developers are more likely to use `v_` prefix related variables so it avoids most of the down-sides mentioned about using "positions" on it's own, although I'd prefer stick to the current, shorter naming.

Hans Goudey commented

2022-10-14 23:37:20 +02:00

Firstly, I don't think it's necessary to follow naming from ID-attributes in general.

I've found this helpful in general when working on geometry nodes. It's a simple rule that's predictable for people, and consistency with the UI is good IMO.

The link to vertices is important, even if the data represents positions, knowing the array is vertex aligned is important when used with other vertex data (typically normals).

Honestly I don't agree about this. Positions aren't stored for any other domain in the vast majority of cases. I found it quite clear in most every place I used the name.
Wherever there was ambiguity, something like mesh_positions made it clear again. It depends on the situation though. I don't think some general rule for prefixes is necessary.
This positions naming has been used for quite a while in geometry nodes and curve code. I haven't noticed people having the confusions you're mentioning.
For examples, see positions in *.cc files, or CurvesGeometry::positions().

Calling vertex "positions" lends it's self to p_* and pos_* prefixes

I don't think it does frankly. At this point I've seen a fair number of developers interact with this naming, and I've never seen that. I've seen plenty of variants of v_idx, vert_index, i_vert, vert_i, all of which are clear, IMO.
But again, context depends a lot! In many places a simple i is fine.

Existing abbreviations v_co, v_no & v_index are an obvious fit

v_co and v_no aren't used much in the codebase. Generally I don't think their necessary at all, when compared to indexing arrays like positions[vert_i].
When a single Mesh position is passed to a separate function, I'd argue it's usually as a generic 3D vector rather than something specific to meshes.

Also, I think there's a real benefit to choosing simple single-word naming. "positions" is a full word, it's easy to read and type when compared to abbreviations or multi-word naming.

>Firstly, I don't think it's necessary to follow naming from ID-attributes in general. I've found this helpful in general when working on geometry nodes. It's a simple rule that's predictable for people, and consistency with the UI is good IMO. >The link to vertices is important, even if the data represents positions, knowing the array is vertex aligned is important when used with other vertex data (typically normals). Honestly I don't agree about this. Positions aren't stored for any other domain in the vast majority of cases. I found it quite clear in most every place I used the name. Wherever there was ambiguity, something like `mesh_positions` made it clear again. It depends on the situation though. I don't think some general rule for prefixes is necessary. This `positions` naming has been used for quite a while in geometry nodes and curve code. I haven't noticed people having the confusions you're mentioning. For examples, see `positions` in `*.cc` files, or `CurvesGeometry::positions()`. >Calling vertex "positions" lends it's self to p_* and pos_* prefixes I don't think it does frankly. At this point I've seen a fair number of developers interact with this naming, and I've never seen that. I've seen plenty of variants of `v_idx`, `vert_index`, `i_vert`, `vert_i`, all of which are clear, IMO. But again, context depends a lot! In many places a simple `i` is fine. >Existing abbreviations v_co, v_no & v_index are an obvious fit `v_co` and `v_no` aren't used much in the codebase. Generally I don't think their necessary at all, when compared to indexing arrays like `positions[vert_i]`. When a single `Mesh` position is passed to a separate function, I'd argue it's usually as a generic 3D vector rather than something specific to meshes. Also, I think there's a real benefit to choosing simple single-word naming. "positions" is a full word, it's easy to read and type when compared to abbreviations or multi-word naming.

Campbell Barton commented

2022-10-18 02:18:03 +02:00

Regarding consistency:

If this change is made, it leaves existing uses of vertices being referenced as coordinate abbreviated to co.
This proposal states that:

In code we usually don't pass around one mesh position at a time

Without mentioning how to handle the inconstancy created by having a different name for a single vertex position which is still done in enough places and not likely to be phased out entirely.

Examples:

BMVert.co (both C/C++ and Python API).
MeshVertex.co (Python API).
PBVHVertexIter.co C/C++, needed so sculpt-mode can generalize over BMesh and Mesh structures.
Iterators such as BKE_mesh_foreach_mapped_vert & BKE_mesh_foreach_mapped_edge pass single vertex coordinates as arguments.

More generally these conventions are used in code not directly related to mesh:

void plane_to_point_vector_v3_normalized(const float plane- [x], float r_plane_co- [x], float r_plane_no[3])
bool ED_view3d_clipping_test(const struct RegionView3D *rv3d, const float co- [x], bool is_local);

The terms co_ / _co as a prefix/suffix shows up in source/ over 4000 times (regex (\bco_|_co\b)), so we should be clear on this being an accepted inconsistency, or if this is a convention to move away from.

In #101688#1432636, @HooglyBoogly wrote:

Firstly, I don't think it's necessary to follow naming from ID-attributes in general.

I've found this helpful in general when working on geometry nodes. It's a simple rule that's predictable for people, and consistency with the UI is good IMO.

All things equal, +1 for consistency, in this case I think it's not such a straightforward case though.

The link to vertices is important, even if the data represents positions, knowing the array is vertex aligned is important when used with other vertex data (typically normals).

Honestly I don't agree about this. Positions aren't stored for any other domain in the vast majority of cases. I found it quite clear in most every place I used the name.
Wherever there was ambiguity, something like mesh_positions made it clear again. It depends on the situation though. I don't think some general rule for prefixes is necessary.
This positions naming has been used for quite a while in geometry nodes and curve code. I haven't noticed people having the confusions you're mentioning.
For examples, see positions in *.cc files, or CurvesGeometry::positions().

Checking naming for CurvesGeometry::positions, some abbreviations currently include:

&p1_cu = positions[segment_i];
const float3 &p2 = nurbs_positions[i + 1];
positions- [x] = math::interpolate(p1, p2, factor);

So I think it's reasonable to consider shortened names colliding with a common prefix for polygons (which isn't an issue for Curves).

Calling vertex "positions" lends it's self to p_* and pos_* prefixes

I don't think it does frankly. At this point I've seen a fair number of developers interact with this naming, and I've never seen that. I've seen plenty of variants of v_idx, vert_index, i_vert, vert_i, all of which are clear, IMO.
But again, context depends a lot! In many places a simple i is fine.

Existing code already uses p_ / p1,2,3 prefixes for positions - for curves, and I'm still not convinced it's practical to prevent it by poking developers not to use that abbreviation.

Assigning short variables to vertex names in modifiers, sculpt code etc... is still going to be done, so how developers are likely to name these shouldn't be overlooked.

Existing abbreviations v_co, v_no & v_index are an obvious fit

v_co and v_no aren't used much in the codebase. Generally I don't think their necessary at all, when compared to indexing arrays like positions[vert_i].
When a single Mesh position is passed to a separate function, I'd argue it's usually as a generic 3D vector rather than something specific to meshes.

As noted above, I think this is done enough that it shouldn't be ignored.

Also, I think there's a real benefit to choosing simple single-word naming. "positions" is a full word, it's easy to read and type when compared to abbreviations or multi-word naming.

I don't find using abbreviations to be an issue as long as they're consistent and unambiguous.

Regarding consistency: If this change is made, it leaves existing uses of vertices being referenced as coordinate abbreviated to `co`. This proposal states that: > In code we usually don't pass around one mesh position at a time Without mentioning how to handle the inconstancy created by having a different name for a single vertex position which is still done in enough places and not likely to be phased out entirely. Examples: - `BMVert.co` (both C/C++ and Python API). - `MeshVertex.co` (Python API). - `PBVHVertexIter.co` C/C++, needed so sculpt-mode can generalize over BMesh and Mesh structures. - Iterators such as `BKE_mesh_foreach_mapped_vert` & `BKE_mesh_foreach_mapped_edge` pass single vertex coordinates as arguments. More generally these conventions are used in code not directly related to mesh: `void plane_to_point_vector_v3_normalized(const float plane- [x], float r_plane_co- [x], float r_plane_no[3])` `bool ED_view3d_clipping_test(const struct RegionView3D *rv3d, const float co- [x], bool is_local);` The terms `co_` / `_co` as a prefix/suffix shows up in `source/` over 4000 times (regex `(\bco_|_co\b)`), so we should be clear on this being an accepted inconsistency, or if this is a convention to move away from. ----- > In #101688#1432636, @HooglyBoogly wrote: >>Firstly, I don't think it's necessary to follow naming from ID-attributes in general. > I've found this helpful in general when working on geometry nodes. It's a simple rule that's predictable for people, and consistency with the UI is good IMO. All things equal, +1 for consistency, in this case I think it's not such a straightforward case though. >>The link to vertices is important, even if the data represents positions, knowing the array is vertex aligned is important when used with other vertex data (typically normals). > Honestly I don't agree about this. Positions aren't stored for any other domain in the vast majority of cases. I found it quite clear in most every place I used the name. > Wherever there was ambiguity, something like `mesh_positions` made it clear again. It depends on the situation though. I don't think some general rule for prefixes is necessary. > This `positions` naming has been used for quite a while in geometry nodes and curve code. I haven't noticed people having the confusions you're mentioning. > For examples, see `positions` in `*.cc` files, or `CurvesGeometry::positions()`. Checking naming for `CurvesGeometry::positions`, some abbreviations currently include: - `&p1_cu = positions[segment_i];` - `const float3 &p2 = nurbs_positions[i + 1];` - `positions- [x] = math::interpolate(p1, p2, factor);` So I think it's reasonable to consider shortened names colliding with a common prefix for polygons (which isn't an issue for Curves). >>Calling vertex "positions" lends it's self to p_* and pos_* prefixes > I don't think it does frankly. At this point I've seen a fair number of developers interact with this naming, and I've never seen that. I've seen plenty of variants of `v_idx`, `vert_index`, `i_vert`, `vert_i`, all of which are clear, IMO. > But again, context depends a lot! In many places a simple `i` is fine. Existing code already uses `p_` / `p1,2,3` prefixes for positions - for curves, and I'm still not convinced it's practical to prevent it by poking developers not to use that abbreviation. Assigning short variables to vertex names in modifiers, sculpt code etc... is still going to be done, so how developers are likely to name these shouldn't be overlooked. >>Existing abbreviations v_co, v_no & v_index are an obvious fit > `v_co` and `v_no` aren't used much in the codebase. Generally I don't think their necessary at all, when compared to indexing arrays like `positions[vert_i]`. > When a single `Mesh` position is passed to a separate function, I'd argue it's usually as a generic 3D vector rather than something specific to meshes. As noted above, I think this is done enough that it shouldn't be ignored. > Also, I think there's a real benefit to choosing simple single-word naming. "positions" is a full word, it's easy to read and type when compared to abbreviations or multi-word naming. I don't find using abbreviations to be an issue as long as they're consistent and unambiguous.

Hans Goudey commented

2022-12-13 06:04:38 +01:00

I read through the patch again today. I still prefer Mesh.positions() and passing around spans called positions or mesh_positions in the few cases where it's unclear.

_co/_co is used a lot, but I do think it's okay to have different conventions for passing around a single location, which usually has different meaning than "the actual position of the vertex we're dealing with at this moment".

All in all, like you say, trying to police developers to use p1, co_1, co_a, pos_1, or position, etc. isn't going to work. What I think might work better though is to push developers to use spans and indices instead.
In the end there will be always be some difference in these more local field names. I think that's okay. To me the most important part is that we refer to the actual position attribute consistently. I also think that helps people form the right abstractions-- "It's just another attribute on the geometry".

I'm happy to put some time into clarifying things in the code if you think that's helpful.

I read through the patch again today. I still prefer `Mesh.positions()` and passing around spans called `positions` or `mesh_positions` in the few cases where it's unclear. `_co/_co` is used a lot, but I do think it's okay to have different conventions for passing around a single location, which usually has different meaning than "the actual position of the vertex we're dealing with at this moment". All in all, like you say, trying to police developers to use `p1`, `co_1`, `co_a`, `pos_1`, or `position`, etc. isn't going to work. What I think might work better though is to push developers to use spans and indices instead. In the end there will be always be some difference in these more local field names. I think that's okay. To me the most important part is that we refer to the actual position attribute consistently. I also think that helps people form the right abstractions-- "It's just another attribute on the geometry". I'm happy to put some time into clarifying things in the code if you think that's helpful.

Campbell Barton commented

2022-12-16 01:30:24 +01:00

If keeping co / v_co as an acceptable abbreviation for vertex coordinates then I'm fine to use the term positions.

Although I'd prefer to keep the vert/vertex prefix vert_positions matches vert_normals and vert_colors. Otherwise it's awkward if a function takes a positions & normals argument, it's not obvious what they're aligned to and that they belong to the same domain.

In some certain cases it may not matter for example a function might take poly-positions & poly-normals ... or vert-positions & vert-normals interchangeably, in those cases passing in positions & normals is fine, although I'd consider that a fairly exceptional case.

If keeping `co` / `v_co` as an acceptable abbreviation for vertex coordinates then I'm fine to use the term positions. Although I'd prefer to keep the vert/vertex prefix `vert_positions` matches `vert_normals` and vert_colors. Otherwise it's awkward if a function takes a `positions` & `normals` argument, it's not obvious what they're aligned to and that they belong to the same domain. In some certain cases it may not matter for example a function might take poly-positions & poly-normals ... or vert-positions & vert-normals interchangeably, in those cases passing in `positions` & `normals` is fine, although I'd consider that a fairly exceptional case.

Hans Goudey commented

2022-12-16 18:04:32 +01:00

Regarding vert_positions vs. positions, currently I don't think the vert_ prefix is helpful, because we only store one vertex position array on meshes, so there is no ambiguity.

Normals are different because we provide accessors for vertex normals, face normals, and corner/loop normals. I don't think we should use the raw "normals" word unless the context is very clear. But that confusion doesn't exist for positions, it's just the position attribute, which fundamentally has to have a unique name across all domains.

The fact that vert_colors or vert_normals have the prefix but positions doesn't doesn't seem like a problem to me.

That said, I'm okay compromising with this vert_ prefix on the accessor functions, it's only five letters anyway.

Regarding `vert_positions` vs. `positions`, currently I don't think the `vert_` prefix is helpful, because we only store one vertex position array on meshes, so there is no ambiguity. Normals are different because we provide accessors for vertex normals, face normals, and corner/loop normals. I don't think we should use the raw "normals" word unless the context is very clear. But that confusion doesn't exist for positions, it's just the position attribute, which fundamentally has to have a unique name across all domains. The fact that `vert_colors` or `vert_normals` have the prefix but `positions` doesn't doesn't seem like a problem to me. That said, I'm okay compromising with this `vert_` prefix on the accessor functions, it's only five letters anyway.

Evan Wilson commented

2023-01-06 09:11:38 +01:00

Added subscriber: @EAW

Hans Goudey commented

2023-01-10 18:43:58 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Hans Goudey closed this issue

2023-01-10 18:43:58 +01:00

Hans Goudey self-assigned this 2023-01-10 18:43:58 +01:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Mesh vertex internal naming (especially position data) #101688