Good point about it being context dependent & not really suited for as generic of a class as Mesh. When you mention zero-cost, what overhead difference is there? I was under the impression that…
I don't know if I understand the prior history well enough to evaluate the scope or correctness of the change. The changes make sense in a vacuum though
Maybe reference the specific mesh caches that these correspond to? As a larger question, I think we should clearly define what type of data we do and dont want in SculptSession since that was one of the main tasks in the refactor - not saying that there's necessarily a better place for this.
Nit: The scoping here seems unnecessary; I think I'd opt to renaming the positions_eval name below or determining if we need to change the signature of this function to a MutableSpan
I know this isn't part of this PR, but wow this is a confusing name at first glance.
Just a note to verify testing with the duplicate object scenario mentioned in this crash to make sure this change doesn't cause regressions
I don't know if I understand the logic behind the if statements here & in the face_normals_cache_eval function
I don't see the value in this assert, was this left over from previous testing?
Maybe a member function on the Mesh object instead of this floating static? Depends on if you think we'll use this elsewhere.
Seems like this comment can be removed at this point.
Could we use deform_modifiers_active here? Or alternatively, if deform_modifiers_active is always false if this is empty and true if its populated, maybe lets make a public function so we don't do different kinds of checks everywhere.
I don't mind making the proposed changes, I think it makes the overall desired behavior more clear