Many nodes operate on all the instances that are passed into them. For example,
the Subdivision Surface node subdivides the mesh at the root but also instanced
meshes. This works well for most nodes, but there are a few nodes were the old
`modify_geometry_sets` function was not very well defined and it was tricky to
use correctly.
The fundamental problem was that the behavior is not obvious when a node creates
or modifies instances and how those are integrated with the already existing
instances.
This patch solves this with the following changes:
* Remove the old `GeometrySet::modify_geometry_sets` and related
`*_during_modify` methods.
* Add a new `blender::geometry::foreach_real_geometry` function that is similar
to the old `modify_geometry_sets` but has a more well-defined interface:
* It never passes instances into the callback. So existing instances can't be
modified with it.
* The callback is allowed to create new instances. This will automatically be
merged back with potentially already existing instances. The callback does
not have to worry about accidentally invalidating existing instances like
before.
* A few existing usages used `modify_geometry_sets` to actually modify existing
instances (usually just removing attributes). Those can't use the new
`foreach_real_geometry`, so they just get a custom simple recursive
implementation instead of using a generic function.
Pull Request: https://projects.blender.org/blender/blender/pulls/143898
As part of the addition of free normals, the join geometry and realize
instances nodes were updated to properly join custom normals. When one
mesh input had tangent space normals and another had no custom normals,
I chose to use free custom normals for the output mesh since that has
drastically better performance. However, it turns out users get into
that situation much more often than I expected, and because many areas
still don't support free custom normals very well, and their presense
isn't obvious, this causes confusion.
This commit changes this code to output tangent space custom normals
whenever any of the input meshes have tangent space custom normals.
That also maintains the most information for propagation later, since the
"default" status of (0,0) custom normals is maintained.
Fixes#143368
Pull Request: https://projects.blender.org/blender/blender/pulls/143498
When transforming a geometry, we often apply the transposed inverse
to normals / custom normal data. However, that matrix can still contain
scale from the original matrix. That scale has to be removed so we can
avoid also scaling the normals.
I used the opportunity to remove the duplication between mesh and curves
processing of the custom normals, and to formalize an optimization to
skip the final normalization of each vector if the transform is such
that it isn't necessary. The new functions don't fit beautifully into
their public headers, but I don't know of a better place for them.
Pull Request: https://projects.blender.org/blender/blender/pulls/142896
The "curve_type" attribute in curves geometry is built-in and only valid with
a `int8` type on the `Curves` attribute domain. Adding it with a different type
on instance geometry is fine though, but causes invalid attribute writer access
when realizing the instances.
Pull Request: https://projects.blender.org/blender/blender/pulls/142218
The code added to postprocess the manifold result to remove
extra vertices introduced on edges had a logic bug in it.
It would sometimes dissolve a vertex from a triangle.
I thought that would be very rare and just repeated a vertex to
fill out the triangle, but that leads to a mesh that doesn't validate.
Anyway, my logic to prevent a single vertex from being dissolved from
a triangle was wrong (if the triangle was seen second while looking).
I fixed that, and then the even rarer case of two vertices being
dissolved from a quad showed up. I have prevented all such cases
by a loop that disables all vertex dissolving in faces where it
would leave less than three vertices.
Also added an assert (so, debug-mode only) that the mesh produced
by manifold boolean is valid.
The case of subtracing a plane is handled specially as the
plane is not manifold, but the library has a TrimByPlane function.
The special handling code did not deal with the empty result case
properly.
Also, there was no error return checking from Manifold in this special
case, so that was added.
Also, the general error handling just assumed that any Manifold error
on the inputs was a "not manifold" error, which while probably true,
should not be assumed, so that was fixed too.
To resolve, simply pass the matrix to `is_plane` and transform the
points before creating a plane from them.
NOTE: we also have a crash in main when the plane is "outside" the
target, will report that separately.
Pull Request: https://projects.blender.org/blender/blender/pulls/142336
Fix for unreported issue with Grease Pencil interpolation tool: on cyclic curves
the last point is interpolated between the end points of the curve, especially
noticeable with sequence interpolation.
This required handling a corner case in the curve sample mapping function.
This function is complex and hard to verify with the operator alone, leading to
frequent issues and discovery of yet more corner cases. For this reason i
refactored the sampling function and added new unit tests.
This should help avoid regressions and make it clear how the function is
expected to behave in various corner cases.
The `sample_curve_padded` function has been moved into the geometry module,
since the `sculpt_paint` module does not have tests yet and is intended mostly
for higher-level operator code. The function has been split to separate out the
"reverse" sampling mode, which reduces complexity. Reverse sampling is done by
first reversing the input curve points, doing regular sampling, and then
reversing the resulting samples.
The function can now sample to larger or smaller sample arrays:
- Larger output arrays have a point aligned with each source point as before,
with the rest of the points evenly distributed over the source curve.
This ensures that the output curve matches the source as closely as possible,
especially for poly curves.
- Smaller output arrays are uniformly sampled along the length of the source
curve.
Pull Request: https://projects.blender.org/blender/blender/pulls/141946
This commit moves the freestyle edge and face mark tags to become
generic attributes, similar to other changes over the past years. The
attributes are called "freestyle_edge" and "freestyle_face", and they're
now propagated like regular boolean attributes.
Compatibility wise, forward and backward blend file compatibility are
maintained (for forward compatibility this is implemented a bit
differently than in the past because of the ongoing `AttributeStorage`
transition). In the Python API, `use_freestyle_mark` has been removed;
the attribute API should be used instead (just like bevel weights).
The BMesh (`freestyle`) accessors are removed too.
The conversions benefit from the fact that bit-wise, the old structs are
the same as `bool`, so we can convert to the old and new formats without
reallocating arrays.
Pull Request: https://projects.blender.org/blender/blender/pulls/141996
Change `eCustomDataType` to `bke::AttrType` for uses of the attribute
API (the `AttributeAccessor` one anyway). I didn't touch any values that
might be saved in files; those should be handled on a case by case basis.
Part of #122398
Pull Request: https://projects.blender.org/blender/blender/pulls/141301
This commit moves Curves and Grease Pencil to use `AttributeStorage`
instead of `CustomData`, except for vertex groups. This PR mostly
involves extending the changes from the above commit for point clouds
to generalize to other geometry types.
This is mostly straightforward, though a couple non-trivial places of
note are the joining of Grease Pencil objects (`merge_attributes`), the
"default render fallback" UV for curves objects which was previously
unused at the UI level and just ended up being the first attribute, and
the `update_curve_types()` call in the curves versioning function.
Similar to:
- fa03c53d4a
- f74e304b00
Part of #122398.
Pull Request: https://projects.blender.org/blender/blender/pulls/140936
In a fix to manifold boolean, commit a20f367379, the code
sometimes dissolved vertices in triangles and then didn't remap
those vertices. This prevents the dissolve in the first place.
The boolean modifier Exact solver has a solver option "Materials"
with choices "Index Based" and "Transfer". The former uses
only materials that were in the first operand object/mesh.
The Transfer option copies new materials as needed from other
object/mesh operands and uses those on the pieces of faces from
those operands that survive into the output.
Users very often use boolean to cut away from a main mesh, and
in such cases usually don't care about the materials on the cutter
operand, and don't want materials from them transferred, so the
"Index Based" choice is the default in the modifier.
It was regarded as in important bug/lack that the new Manifold
solver did not have such an option, so this commit adds one.
The Boolean Geometry Node at the moment does not have an option
and always uses the "Transfer" method, for all three solvers.
It is a matter of discussion whether such an option should be added
in the node also, so this commit does not include such a change.
The Manifold solver, up to this point, ignored the material_remaps
argument and relied on the realize_instance code to remap the
materials (it uses the Transfer strategy).
This change overrides that remapping with the explicit mapping
handed in through the API, if the mapping has non-zero size.
Since the old way (ignoring the mapping argument) worked fine for
the Boolean Geometry Node, I changed that code to make the map
have size zero in the node, in the case that the solver is Manifold.
This is a little hacky but I couldn't think of anything much better.
Long term it might be nice to have the internal boolean API not take
in remaps at all, but rather a remapping strategy choice. One thing
that makes that difficult right now is that the modifier can get
materials from either the object or the mesh (at least that used
to be true) and the internal boolean api only knows about meshes.
Another thing that would have made this task easier (for me) would
be to have realize_instances take in a material mapping strategy
as a parameter.
The code added in commit ffc204d1fa to dissolve redundant 2-edged
vertices after a manifold boolean assumed that after dissolving such
vertices a valid face would remain. This is not true of the face
started out degenerate (all vertices on the same line).
Fixed by checking for such cases and in any case not creating
any faces with less than three vertices.
The code added in commit ffc204d1fa to dissolve redundant 2-edged
vertices after a manifold boolean assumed that after dissolving such
vertices a valid face would remain. This is not true of the face
started out degenerate (all vertices on the same line).
Fixed by checking for such cases and in any case not creating
any faces with less than three vertices.
The calls to `to_geometry_set` in this file can create a temporary
Instances struct for collections. That instances component will contain
two attributes, which are currently referenced in the attributes map
even after the temporary compoment storage goes out of scope. A simple
fix is to avoid adding these attributes to the map in the first place.
An alternative that would also be more efficient would be to handle each
instance reference type explicitly, without converting it to a temporary
geometry set. That seems to significantly complicate the code though;
for now it doesn't seem worth it.
Pull Request: https://projects.blender.org/blender/blender/pulls/140999
This is discussed more in PR #140773.
The cause of the breakage was the change of the Manifold library
version from 3.0.1 to 3.1.0. That change is very positive otherwise
because we can remove the "use runids" workaround to prevent bad
face merging, and that removal is also part of this commit.
Removing that changes the time to do a big sphere-sphere test
from 660ms to 340ms.
The problem that needed fixing is that the new library version appears
not to do some aggressive simplification that the old version did,
and as a result, when we dissolve triangulation edges after the boolean
is done, it sometimes leaves valence-2 vertices on original edges.
To fix that, new code detects and then dissolves such vertices.
This is discussed more in PR #140773.
The cause of the breakage was the change of the Manifold library
version from 3.0.1 to 3.1.0. That change is very positive otherwise
because we can remove the "use runids" workaround to prevent bad
face merging, and that removal is also part of this commit.
Removing that changes the time to do a big sphere-sphere test
from 660ms to 340ms.
The problem that needed fixing is that the new library version appears
not to do some aggressive simplification that the old version did,
and as a result, when we dissolve triangulation edges after the boolean
is done, it sometimes leaves valence-2 vertices on original edges.
To fix that, new code detects and then dissolves such vertices.
Small constant trivial referencing objects should be passed by value.
Current state of code most likely legacy from the times there was an optional strings.
Pull Request: https://projects.blender.org/blender/blender/pulls/138058
Replaces pointer based EXPECT_EQ_ARRAY with EXPECT_EQ_SPAN in most cases
as they already used spans (or span compatible datastructures).
Currently EXPECT_EQ_ARRAY only takes in one size variable and doesn't
compare the number of elements between arguments (requiring an
additional line to do so).
This should make the code cleaner and safer. Goal is also to promote
the use Spans in new test code.
Pull Request: https://projects.blender.org/blender/blender/pulls/140340
The curve interpolation operator write uninitialized data to vertex
attributes if there is more than one interpolated curve pair.
This is because _partial writes_ to `VArraySpan` wrappers only work
if the original VArray is already a span or if the wrapper span is
fully initialized with the original data beforehand.
In the case of the curve interpolation tool for Grease Pencil the
interpolation is invoked for each curve pair separately, creating a
new output attribute `VArraySpan` wrapper each time. This wrapper
is only filled for the curve pair in question and writes uninitialized
data to all the other curves' vertex weight attributes.
To prevent this from happening the simple solution is to use
`lookup_or_add_for_write_span` which initializes the entire span.
This causes quite a lot of unnecessary copying, but that is acceptable
for the Grease Pencil interpolation tool. The alternative is to change
the tool so that the destination GSpanAttributeWriter is only created
once, but that is a much bigger change.
Pull Request: https://projects.blender.org/blender/blender/pulls/140283
Adds a Shape Method parameter to the UV Pack Islands node, enabling
artists to choose between faster packing and more efficient space
utilization.
Those are the three shape method options:
* Bounding Box: Fastest, less efficient space usage
* Convex Hull: Balanced performance and efficiency
* Exact Shape: Optimal packing, higher computational cost
This change consolidates arguments in `uv_parametrizer_pack()`. Now it
accept ` UVPackIsland_Params` instead of many different separate options.
This also makes it easier to expose more options in the future.
Pull Request: https://projects.blender.org/blender/blender/pulls/139110
This moves `PointCloud` to use the recently added `AttributeStorage`
at runtime. Mainly this involves implementing the higher level attribute
API on top, and implementing the RNA API as well. The attribute RNA type
is now backed by either CustomDataLayer or bke::Attribute. For now the
new code is specific to point clouds but next steps can reuse it for
Grease Pencil layer attributes, curves, and eventually meshes.
Point cloud attributes no longer have a name length limit.
Internally, the `AttributeStorage` API is extended with a few additions:
- The data structs have static constructors for convenience.
- A few functions give index-based access to attributes
- A "rename" function is added.
The `Attribute` RNA type now exposes a `storage_type` property.
For now the "single value" option is still unused at runtime, and
accessing the single value data isn't implemented yet.
Pull Request: https://projects.blender.org/blender/blender/pulls/139165
Current strategy to deal with operators not supporting custom NURBS
knots is to fall back to calculated knots for curves of the custom mode
but with no `CurvesGeometry::custom_knots` allocated. Such curves are
the result of operators that copy only `Point` and `Curve` domains. This
way the problem is only postponed. It is not possible to add new custom
knot curves to such `CurvesGeometry` as custom knot offsets are
calculated all together and there is no way to distinguish between old
curves with lost knots and new ones. This is more a future problem.
The actual problem in `main` can be shown with an attached blend file
(see PR) by applying `Subdivide` to some points and then adding new
`Bezier` curve to the same object. This particular problem could be
addressed somewhere in `realize_instances.cc` but the actual problem
would persist.
This PR handles custom knots in all places where `BKE_defgroup_copy_list`
is iused, and where `bke::curves::copy_only_curve_domain` is called.
Here the assumption is made that only these places can copy custom knots
modes without copying custom knots. Depending on operator logic knots are
handled most often in one of two ways:
- `bke::curves::nurbs::copy_custom_knots`:
copies custom knots for all curves excluding `selection`. Knot modes
for excluded curves are altered from the custom mode to calculated.
This way only curves modified by the operator will loose custom knots.
- `bke::curves::nurbs::update_custom_knot_modes;`
alters all curves to calculated mode.
In some places (e.g. `reorder.cc`) it is possible to deal with knots
without side effects.
PR also adds `BLI_assert` in `load_curve_knots` function to check if
`CurvesGeometry::custom_knots` exists for custom mode curves. Thus
versioning code is needed addressing the issue in files in case such
already exists.
Pull Request: https://projects.blender.org/blender/blender/pulls/139554
Adds a new operator in Grease Pencil edit mode to convert between curve
types. This acts as a replacment for the `Set Curve Type` operator as
the new operator better aligns with previous workflows and artist
expectations. Specifically using a threshold to adjust how well the
resulting curves fit to the original.
It can be found in the `Stroke` > `Convert Type` menu.
This operator aims at keeping visual fidelity between the curves. When
converting to a non-poly curve type, there's a `threshold` parameter
that dictates how closley the shapes will match (a value of zero meaning
an almost perfect match, and higher values will result in less accuracy
but lower control point count).
The conversion to `Catmull-Rom` does not do an actual curve fitting.
For now, this will resample the curves and then do an adaptive
simplification of the line (using the threshold parameter)
to simulate a curve fitting.
The `Set Curve Type` operator is no longer exposed in the
`Stroke` menu.
This also adds a new `geometry::fit_curves` function.
The function will fit a selection of curves to bézier curves. The
selected curves are treated as if they were poly curves.
The `thresholds` virtual array is the error threshold distance
for each curve that the fit should be within. The size of the virtual
array is assumed to have the same size as the total number of
input curves.
The `corners` virtual array allows specific input points to be treated
as sharp corners. The resulting bezier curve will have this point and
the handles will be set to "free".
There are two fitting methods:
* **Split**: Uses a least squares solver to find the control
points (faster, but less accurate).
* **Refit**: Iteratively removes knots with the least error starting
with a dense curve (slower, more accurate fit).
Co-authored-by: Casey Bianco-Davis <caseycasey739@gmail.com>
Co-authored-by: Hans Goudey <hans@blender.org>
Pull Request: https://projects.blender.org/blender/blender/pulls/137808
`blender::geometry::fillet_curves` should check for situations where empty
curves are passed in (this could happen in geometry nodes) and in those
cases it should not run.
Pull Request: https://projects.blender.org/blender/blender/pulls/139787
