blender
/
blender
136
412
Fork 596
Code Issues 6.3k Pull Requests 775 Projects 19 Wiki Activity

Fix #117997: Crash trying to copy vertex group attributes as spans #119212

Merged
Lukas Tönne merged 6 commits from LukasTonne/blender:fix-curve-to-mesh-dvert-attributes into main 2024-03-11 20:52:24 +01:00
Conversation 3 Commits 6 Files Changed 1 +24 -3
1 changed files with 24 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
source/blender/blenkernel/intern/curve_to_mesh_convert.cc
View File

@ -376,12 +376,27 @@ static GSpan evaluate_attribute(const GVArray &src,
const CurvesGeometry &curves,
Vector<std::byte> &buffer)
{
if (curves.is_single_type(CURVE_TYPE_POLY) && src.is_span()) {
return src.get_internal_span();
/* Poly curves evaluated points match the curve points, no need to interpolate. */
if (curves.is_single_type(CURVE_TYPE_POLY)) {
if (src.is_span()) {
return src.get_internal_span();
}
buffer.reinitialize(curves.points_num() * src.type().size());
src.materialize(buffer.data());
GMutableSpan eval{src.type(), buffer.data(), curves.points_num()};
LukasTonne marked this conversation as resolved Outdated
Hans Goudey commented 2024-03-08 16:41:03 +01:00
Outdated
Review
Copy Link

ensure_can_interpolate_to_evaluated should be called at some higher level before all this interpolation needs to happen

`ensure_can_interpolate_to_evaluated` should be called at some higher level before all this interpolation needs to happen
return eval;
}
if (src.is_span()) {
buffer.reinitialize(curves.evaluated_points_num() * src.type().size());
LukasTonne marked this conversation as resolved
Hans Goudey commented 2024-03-08 16:41:32 +01:00
Review
Copy Link

GVArraySpan is a utility to do this

`GVArraySpan` is a utility to do this
Lukas Tönne commented 2024-03-11 10:46:26 +01:00
Review
Copy Link

I don't see how this would work: GVArraySpan can wrap the input src array but is not mutable (can't materialize into it). GVMutableArraySpan is mutable but can't be simply constructed from the immutable src buffer.

I don't see how this would work: `GVArraySpan` can wrap the input `src` array but is not mutable (can't materialize into it). `GVMutableArraySpan` is mutable but can't be simply constructed from the immutable `src` buffer.
Iliya Katushenock commented 2024-03-11 10:58:31 +01:00
Review
Copy Link

But this is src buffer and should not be mutable, no?

But this is *src* buffer and should not be mutable, no?
Lukas Tönne commented 2024-03-11 11:05:06 +01:00
Review
Copy Link

Ok, looked into the constructor and it actually does a materialize internally. That wasn't clear to me.

Ok, looked into the constructor and it actually does a `materialize` internally. That wasn't clear to me.
GMutableSpan eval{src.type(), buffer.data(), curves.evaluated_points_num()};
LukasTonne marked this conversation as resolved
Hans Goudey commented 2024-03-08 16:42:09 +01:00
Review
Copy Link

Seems worth also having a fast path for curves.is_single_type(CURVE_TYPE_POLY) here. The materialize can happen directly into the buffer.

Seems worth also having a fast path for `curves.is_single_type(CURVE_TYPE_POLY)` here. The `materialize` can happen directly into the `buffer`.
Lukas Tönne commented 2024-03-08 16:46:03 +01:00
Review
Copy Link

I don't think we can directly materialize into the output buffer because of the interpolate_to_evaluated call. The input array is per control point, the output is per evaluated point.

I don't think we can directly materialize into the output buffer because of the `interpolate_to_evaluated` call. The input array is per control point, the output is per evaluated point.
Hans Goudey commented 2024-03-08 16:49:04 +01:00
Review
Copy Link

When curves.is_single_type(CURVE_TYPE_POLY) is true, there is the same number of control point and evaluated points. That optimization is done just above, except that it requires is_span() there.

When `curves.is_single_type(CURVE_TYPE_POLY)` is true, there is the same number of control point and evaluated points. That optimization is done just above, except that it requires `is_span()` there.
Lukas Tönne commented 2024-03-11 09:33:40 +01:00
Review
Copy Link

Ok, makes sense. I'll add a comment though, because i don't think it's obvious why this works for poly curves.

Ok, makes sense. I'll add a comment though, because i don't think it's obvious why this works for poly curves.
curves.interpolate_to_evaluated(src.get_internal_span(), eval);
return eval;
}
GVArraySpan src_buffer(src);
buffer.reinitialize(curves.evaluated_points_num() * src.type().size());
GMutableSpan eval{src.type(), buffer.data(), curves.evaluated_points_num()};
curves.interpolate_to_evaluated(src.get_internal_span(), eval);
curves.interpolate_to_evaluated(src_buffer, eval);
return eval;
}
@ -843,6 +858,9 @@ Mesh *curve_to_mesh_sweep(const CurvesGeometry &main,
Vector<std::byte> eval_buffer;
/* Make sure curve attributes can be interpolated. */
main.ensure_can_interpolate_to_evaluated();
build_mesh_positions(curves_info, offsets, eval_buffer, *mesh);
mesh->tag_overlapping_none();
@ -909,6 +927,9 @@ Mesh *curve_to_mesh_sweep(const CurvesGeometry &main,
return true;
});
/* Make sure profile attributes can be interpolated. */
profile.ensure_can_interpolate_to_evaluated();
const AttributeAccessor profile_attributes = profile.attributes();
profile_attributes.for_all([&](const AttributeIDRef &id, const AttributeMetaData meta_data) {
if (main_attributes.contains(id)) {