This commit makes the MeshVertex.normal property read-only.
In practice it already was, since the value could be overwritten at any
time if code requests updated normals, so this just clarifies that
position and avoids misleading users.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
Hi @Campbell Barton (campbellbarton), I was wondering if this was an okay to deprecate a function in the Python API, is there a standard practice here? The reports don't seem to show up in the info editor like I expected.
Comment Actions
Making this readonly seems fine, however I'm wary of removing calc_normals unless there is a way to guarantee the mesh is tagged for updating.
There are a handful of situations where the update callback won't run, one we often run into is F-Curves which don't run the RNA properties update callback (since it impacts performance too much).
I'd have assumed this could be forced using ID.update_tag but looking into it further it seems it's not possible.
This script showing the kinds of modifications to vertices that don't trigger automatic updates to normals: P2902
Suggest the following:
- Apply read-only normals part of this patch.
- Keep calc_normals working since making it a NOP could break existing scripts (even though ideally it wouldn't).
- Add an RNA flag so Python throws a deprecation warning when it's used.
- Make sure it's possible to use update_tag to tag the mesh so normals are recalculated when next accessed to account for the remaining cases where update functions don't run as they should (F-Curves for e.g.).
This way script authors won't run into cases where normals fail to update - and have to abuse the API to force an update (mesh.transform(Matrix()) for e.g.)
There are some cases we could ensure the update callback runs when it doesn't (foreach_set for e.g. seems straightforward to resolve).
Note another option we could consider is using a set callback on vertices that tags for updating, however I'm not so keen on this as it's likely to hurt performance for importers - although it could be tested to see what the difference is.
Comment Actions
- Only make .normal read-only in this patch.
Keep calc_normals working since making it a NOP could break existing scripts (even though ideally it wouldn't).
If so, I think it's already broken, because currently it doesn't tag normals dirty, it just recalculates them if they were tagged dirty elsewhere.
Good point about the need for some way to explicitly tag normals dirty.
I mostly agree with your plan, but I would propose a slightly different idea for tagging updates.
I think a current weak area in the C/C++ and Python APIs for meshes is that invalidating caches after changing data is mostly a guessing game.
Currently we have BKE_mesh_runtime_clear_geometry, BKE_mesh_runtime_clear_cache, BKE_mesh_tag_normals_dirty, BKE_mesh_runtime_free_data,
and in RNA: update, calc_loop_triangles, calc_normals, calc_normals_split, and update_tag.
I won't suggest changing or unifying all of that at the moment, but I think we could make things clearer by adding a way to just tell the mesh that the positions have changed in order to invalidate all necessary caches (including BVH trees, etc):
BKE_mesh_tag_positions_changed / mesh.tag_positions_changed().
I think that's more intuitive than a more generic update_tag function, and also allows more granular updates if only the positions changed but not the topology, etc.
Note another option we could consider is using a set callback on vertices that tags for updating, however I'm not so keen on this as it's likely to hurt performance for importers - although it could be tested to see what the difference is.
I'm guessing a call to BKE_mesh_normals_tag_dirty() in a set function for vertex positions would be negligible compared to anything else done in python, but yeah, it might be a bad idea to make the operation less explicit.