We store various lazily calculated caches on meshes, some of which
depend on the vertex positions staying the same. The current API to
invalidate these caches is a bit confusing. With an explicit set of
functions modeled after the functions in BKE_node_tree_update.h,
it becomes clear what function to call. This may become more important
if more lazy caches are added in the future.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
Hi, any thoughts about this idea? I feel this might be a more intuitive approach than trying to use functions like BKE_mesh_runtime_clear_geometry for this purpose.
Comment Actions
Some brainstorming...
- How much do we want to let BKE_mesh_tag_positions_changed know what changed? For example, if all vertices are translated, we might not want to re-calculate tessellation or normals.
- If BKE_mesh_tag_positions_changed ends up being used in situations when the positions don't change, it might be awkward/misleading. When we only want to clear allocated data related to positions. If this only happens a handful of situations a comment explaining why the function is called when positions didn't change is acceptable.
- We could use term vertices instead of positions (especially since non-location data is being moved out of vertices), otherwise coords could be used as this is already used e.g. BKE_mesh_vert_coords_alloc, BKE_mesh_foreach_mapped_vert_coords_get, BKE_mesh_vert_coords_apply.
| source/blender/makesrna/intern/rna_mesh_api.c | ||
|---|---|---|
| 302 | Could use convention used for GPU update_*_tag, e.g. update_vertices_tag. | |
Comment Actions
- Change RNA function name to update_positions_tag
I handled this case in BKE_mesh_translate already. Maybe there's a prettier way to do it, but I think it's pretty rare to translate all points by the same amount.
- If BKE_mesh_tag_positions_changed ends up being used in situations when the positions don't change, it might be awkward/misleading. When we only want to clear allocated data related to positions. If this only happens a handful of situations a comment explaining why the function is called when positions didn't change is acceptable.
Hmm, yes. Though I'm not sure why that would happen. Were you thinking of a specific situation?
- We could use term vertices instead of positions (especially since non-location data is being moved out of vertices), otherwise coords could be used as this is already used e.g. BKE_mesh_vert_coords_alloc, BKE_mesh_foreach_mapped_vert_coords_get, BKE_mesh_vert_coords_apply.
I'd like to avoid "vertices" because it could refer to any attribute on the point/vertex domain, like the vertex hide flag, or some other runtime cache. Changing one of these values shouldn't necessarily influence the others.
The "positions" term is meant to correspond to the position attribute name-- the name used in the UI. I do realize this conflicts with the term "coords"-- I do think "coords" is pretty vague though, and will become unnecessary in the longer term when MVert is replaced by float3.
Comment Actions
How much do we want to let BKE_mesh_tag_positions_changed know what changed? For example, if all vertices are translated, we might not want to re-calculate tessellation or normals.
I think we don't have to let BKE_mesh_tag_positions_changed know anything extra. If there are special cases like that all vertices have been moved by the same amount, a function like BKE_mesh_tag_positions_changed_uniformly. We could even add that second function now, if there are places where we can use it. Internally, it might or might not do the same thing as BKE_mesh_tag_positions_changed for now.
This is also similar to how the BKE_node_tree_update API works. Some of the update tags are handled the same internally, but having different functions allows for optimizations later on, and also makes things more clear on the call-site.
If BKE_mesh_tag_positions_changed ends up being used in situations when the positions don't change, it might be awkward/misleading.
It just shouldn't be called when positions haven't changed. If it is, then there is potential for future optimization.
We could use term vertices instead of positions (especially since non-location data is being moved out of vertices), otherwise coords could be used as this is already used e.g. BKE_mesh_vert_coords_alloc, BKE_mesh_foreach_mapped_vert_coords_get, BKE_mesh_vert_coords_apply.
As Hans, I also prefer using position explicitly since vertex data is much more generic.
| source/blender/editors/sculpt_paint/sculpt.c | ||
|---|---|---|
| 5454 | unrelated change | |
| source/blender/makesrna/intern/rna_mesh_api.c | ||
| 302 | I do wonder why this function should exist in rna. I'd assume that this is called automatically when a vertex position is changed. Requiring addon developers to call this method is kind of a big change. | |
| source/blender/makesrna/intern/rna_mesh_api.c | ||
|---|---|---|
| 302 | I would be fine calling BKE_mesh_tag_positions_changed in the update function for MeshVertex.position, and even removing this so we don't start requiring calling it manually. Would you also expect it to be called automatically when the future generic position attribute is modified? To make that work we might want to refactor the C-based attribute API to use the new API internally (if that's possible). | |
Comment Actions
- Add a uniform position change tag function
- Tag positions update in MeshVertex.co set function
- Remove Mesh.update_positions_tag for now
| source/blender/makesrna/intern/rna_mesh.c | ||
|---|---|---|
| 309 ↗ | (On Diff #51717) | No, good idea. |
Comment Actions
Instead of mixing names in the API, best use coords instead of positions. If these are renamed that can be proposed separately, otherwise it's not obvious what the convention is and makes the API harder to memorize & use.
Comment Actions
Okay, that's fine with me. I'll propose renaming them to "positions" when T93602: Struct of Arrays Refactor for Mesh Vertices is finished.