Page MenuHome

Curves: Validate data when writing to some builtin attributes
AbandonedPublic

Authored by Hans Goudey (HooglyBoogly) on Jul 26 2022, 8:35 PM.

Details

Summary

The curves data type relies on valid values in attributes like
resolution or handle_type_left, and will crash or assert otherwise.
This commit adds clamping when writing to these attributes with the
attribute API (note that the Python/RNA API is still separate for now).
That is done with a virtual array type that applies a function to a value
before writing it to a span. One important detail is that this virtual
array can't tell the rest of the API that it's a span internally, or the
set functions will be bypassed.

The lower level accessors on CurvesGeometry do not have this
clamping, since they mostly have trusted inputs. In order to avoid
the performance cost with the attribute API, in the future a "trusted"
argument could be added to receive a regular "span" virtual array.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jul 26 2022, 8:35 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 54080.
Hans Goudey (HooglyBoogly) created this revision.

Fix is_same

source/blender/blenkernel/intern/attribute_access_intern.hh
294

Couldn't this just used the derived-span implementation instead of a new one?

source/blender/blenkernel/intern/attribute_access_intern.hh
294

Yeah, technically it could. Personally I'm not a fan of the semantics though, it's not a "derived" type, and the reading isn't affected. So I'd guess I'd rather not, but still would if you thought it was important.
There may be other avenues to reduce duplication:

  • The "array utils" header to deduplicate some of the materialize implementations
  • Using a separate "read is span" and "write is span" in CommonVArrayInfo, then maybe it could be a subclass of VArrayImpl_For_Span.
  • Maybe something else?
source/blender/blenkernel/intern/attribute_access_intern.hh
294

I think it should use the existing implementation. Don't see a strong reason not to use that now.
I see that as a kind of derived type. Also, if we could benefit from "read is not affected", we could add that optimization to the existing type as well. Other builtin attributes might benefit from that as well.

Use existing virtual array implementation

source/blender/blenkernel/intern/attribute_access_intern.hh
226

This function doesn't seem to do anything related to clamping.

Rename to make_write_attribute_with_validation

Hans Goudey (HooglyBoogly) marked 3 inline comments as done.Jul 27 2022, 6:32 PM

Doesn't actually fix the Stored Named Attribute node case yet, probably because it uses AttributeInitMove... Besides that, seems fine.

Hans Goudey (HooglyBoogly) planned changes to this revision.Jul 30 2022, 6:02 AM

I feel like I should figure out how to solve that issue before committing this

Closing in favor of D15990. I'm much happier with how that idea turned out. It doesn't use the "correctness by default" approach, but I think that's okay in this case.