duplicate is some terminology specific to the neighbourhood API.

Suggest something more from graphs: adjacent_grid_corner_point_index_on_edge.

Also, add in the comment that the input point_index is to correspond to grid corner.

Also worth re-iterating that adjacent edge will have separate points for grid corners in the middle of the edge.

Don't do such complicated logic. Is hard to follow, hard to understand, and is prone to errors.

The idea of early call of subdiv_ccg_neighbors_init is to initialize storage. We can ignore the corner case when we are overshooting by 2 points and change the logic to capacity initialization. The object storage is big enough for regular meshes, and it is hard to create an exact non-manifold mesh where it will measurably faster to go into trouble and calculate exact amount of neighbours.

That keeping in mind, lets split the storage initialization from subdiv_ccg_neighbors_init into subdiv_ccg_neighbors_init_capacity. The former one will call subdiv_ccg_neighbors_init_capacity and assign number of neighbors (so, no user space is broken). The subdiv_ccg_neighbors_init_capacity will initialzie storage to either in-object or heap-allocated to hold given capacity.

Do it in a separate patch (or, at least, separate commit in the patch).

After that use subdiv_ccg_neighbors_init_capacity() here with the worst case number of neighbours. Then you can incrementally fill in the r_neighbors->coords[num_neighbors++] = fool.

This will eliminate this tricky logic in initialization, and will remove rather obscure logic about duplicate_i.

This will avoid the documented "add duplicates at the end of the neighbor coordinates". Not sure if it's important since it's hard to tell where was the cut-off point in the result where duplicates begin.

So if the order is not important, the comment is to be updated.
Otherwise, it's still possible to simplify logic of maintaining the duplicate_i.

There will be corners of two grids adjacent to point_index_duplicate, and here you seems to only add one.

I can move the storage initialization to a separate function, but I don't see how that will simplify things here. The current function is initializing num_duplicates, which is needed to check in the iterator macro if an index is a duplicate. Even if I initialize storage to the maximum value, I will still need to assign num_duplicates manually by checking is_corner and them add all duplicates after that index.

This adds the corner of the grid from point_index_duplicate of each one of the faces, the other corner for grid that are not the face of the original grid are added in the previous line, right?

If the elements require different treatment, why are they stored in the same continuous array? This seems very weak design decision to me. Better way would be to either split the storage arrays, or to have flags stored in the neighbour coordinate.

Without going into deeper changes, just initialize counters once, avoiding this spaghetti of if statements.

But I don't think that is possible without refactoring the storage and the iterator. For the iterator to work, both the number of duplicates and neighbors need to be exact, there can not be uninitialized indices in the array. (see how neighbor_iterator.is_duplicate is initialized in the SCULPT_VERTEX_DUPLICATES_AND_NEIGHBORS_ITER_BEGIN macro)

I'm not sure what you are implying with this fear of refactoring. Strictly speaking, it is not that simple to development without doing refactor to some extent.
You can not sell this to me as an argument for having fragile spaghetti code with high level of copy-paste. As I've said in the previous comment, there is a way to make code way easier: Without going into deeper changes, just initialize counters once, avoiding this spaghetti of if statements. It would literally take you less time to just follow the request and implement it than to argue in the comment.

For the iterator to work, both the number of duplicates and neighbors need to be exact

This is an incorrect statement. Number of duplicates can not match the number of neighbors: inner point of a grid will have 4 neighbors but 0 duplicates.

Please formulate statements more clear.

Ah, it is in the loop, so indeed seems fine.
I've also did a quick patch to dump the thing to verify. Attached in the comment just in case.

Add an assert that the actual number of neighbors and duplicates matched the initialized ones (another reason to initialize counters once, and use them without spaghetti statements).

What I mean with "need to be exact" is that you need to initialize the array with the exact number of neighbors and duplicates that it will contain. You can't initialize it with 4 duplicates if you are going to fill only 2 and leave 2 invalid indices because the iterator will take them into account and it will iterate over those 2 invalid indices. Even if we can remove the if (is_corner) by allocating the array with a separate function to its maximum capacity, there still need to be an if(is_corner) somewhere else to initialize the correct duplicates count, so the code structure is going to be the same

• Description of more or less ideal world. **

There is a difference in size and capacity. Size is how many elements you have in the container, capacity is how much you can carry in the container.

You generally want to avoid code pattern which goes as following:

• Calculate number of elements, involving some non-trivial calculation
• Allocate array
• Fill in the array, following the same non-trivial logic that during calculation.

That hope is what always lets you down, Is not uncommon to have the code flow get out of sync, causing very hard to find bugs. So unless your estimate is often an order of magnitude off, you initialize container to size 0, capacity of maximum elements, and fill in the container, increasing its size with every element added. This solves the issue of having duplicated logic which you always need to worry about.

You also should not use same storage for two semantically different object types. You either split them, or make the items to be same nature.

So here you can go one of the following ways:

1. Use separate arrays for neighbors and duplicates.
2. Make the API to return array of CCGNeighborCoord, which will effectively be a subclass of CCGGridCoord but with explicit is_duplicate flag.

What it allows you to solve is:

• The strict coupling to exact memory layout
• You never mis-use the entity without easy way to see that you did that

In this specific case it opens the doors for:

• Having simple capacity-based initialization
• Allows to get rid of rather cryptic i + 2, duplicate_i + 2 indexing: you will just be filling arrays in as you have new data for them.

This type of change should not take more than 30-60min, and it will save a lot of time next time some issue is discovered in the related area. So personally i don't know why not to just go and implement things this way.

• Description of better and acceptable way that the current state of the branch. **

Consolidate calculation of duplicates and neoghbor coordinates, streamline the code flow:

const int num_neighbors = num_adjacent_faces + 2;

int num_duplicates = 0;
if (num_duplicates) {
  num_duplicates += num_adjacent_faces;
  if (is_corner) {
    /* <Description why this is needed.> */
    num_duplicates += num_adjacent_faces;
  }
}

subdiv_ccg_neighbors_init(r_neighbors, num_neighbors, num_total_duplicates);

No else branch, no duplicate call to subdiv_ccg_neighbors_init, every line and branch is focused on doing one thing.

Allows you to also verify (BLI_assert) at the end of the function.
Allows you to move count calculation to an utility function, have even more`const` qualifiers.