Prepare custom data API for attribute sharing. #95842

New Issue

Jacques Lucke · 2022-02-17T17:17:18+01:00

Jacques Lucke commented

2022-02-17 17:17:18 +01:00

The custom data api in BKE_customdata.h has multiple methods to get a specific layer. For example CustomData_get_layer. The main issue is that these functions are not const correct. They always return a non-const void * and have no idea whether the caller wants to modify the data or not.

That is problematic when we want to use attribute sharing, because we have to make a copy of shared attributes when the caller wants to modify the attribute.

The solution is to split every CustomData_get_* method in two:

A *_for_read method that returns a const pointer, that the caller must never change.
A *_for_write method that returns a non-const pointer and internally makes a copy of the data if necessary.

With this refactor, the CustomData_duplicate_referenced_layer_* functions become redundant, because the *_for_write methods will make the copy already.

Unfortunately, the "convenience pointers" like Mesh.mvert make this system quite annoying and error prone:

The convenience pointers are not const. On a const-correctness level that implies that everyone with a non-const pointer to a mesh can also modify the vertex positions of that mesh. This is not true, because the array might be shared.
A first step in the right direction would be to make these pointers const. However, even with that, there is the issue that one would have to pass the mesh into *_for_write methods, so that the pointer can be updated if necessary.
A possibly better alternative would be to remove these convenience pointers completely and to provide a simple convenience api instead. E.g. there could be a MVert *BKE_mesh_mverts_for_write(Mesh *) and a const MVert *BKE_mesh_mvert_for_read(const Mesh *) method. This refactor is a bit ugly because the direct data access through convenience pointers is very common, but it might be worth it regardless.

The custom data api in `BKE_customdata.h` has multiple methods to get a specific layer. For example `CustomData_get_layer`. The main issue is that these functions are not const correct. They always return a non-const `void *` and have no idea whether the caller wants to modify the data or not. That is problematic when we want to use attribute sharing, because we have to make a copy of shared attributes when the caller wants to modify the attribute. The solution is to split every `CustomData_get_*` method in two: * A `*_for_read` method that returns a const pointer, that the caller must never change. * A `*_for_write` method that returns a non-const pointer and internally makes a copy of the data if necessary. With this refactor, the `CustomData_duplicate_referenced_layer_*` functions become redundant, because the `*_for_write` methods will make the copy already. Unfortunately, the "convenience pointers" like `Mesh.mvert` make this system quite annoying and error prone: * The convenience pointers are not const. On a const-correctness level that implies that everyone with a non-const pointer to a mesh can also modify the vertex positions of that mesh. This is not true, because the array might be shared. * A first step in the right direction would be to make these pointers const. However, even with that, there is the issue that one would have to pass the mesh into `*_for_write` methods, so that the pointer can be updated if necessary. * A possibly better alternative would be to remove these convenience pointers completely and to provide a simple convenience api instead. E.g. there could be a `MVert *BKE_mesh_mverts_for_write(Mesh *)` and a `const MVert *BKE_mesh_mvert_for_read(const Mesh *)` method. This refactor is a bit ugly because the direct data access through convenience pointers is very common, but it might be worth it regardless.

Jacques Lucke commented

2022-02-17 17:17:18 +01:00

Added subscriber: @JacquesLucke