GPv3: Apply Modifier #124543
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#124543
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "filedescriptor/blender:gpv3-apply-modifier"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This implements the
Apply Modifier
operator for GPv3.Applies the modifier to the current frame. Drawings at the current frame will be updated.
If the result of the modifier creates new layers, the layers will be added and a drawing
at the current frame will be inserted.
Applying a modifier will run the following steps:
modifiy_geometry_set
function of the modifier with this geometry set.strokes_for_write()
to avoid copy 8aa1a52f5974bcc48345
tod17c36c547
WIP: GPv3: Apply Modifierto GPv3: Apply Modifier@blender-bot build
@ -98,1 +98,4 @@
template<typename T, typename IndexT>
inline void scatter(const VArray<T> &src,
const Span<IndexT> indices,
I do not think indices should be templated.
Maybe @HooglyBoogly can comment on why he added this in
6935cd0798
.I can remove the template but maybe there is a reason I don't know about.
@ -1148,0 +1180,4 @@
grease_pencil_result.attributes_for_write().remove_anonymous();
/* Get the original material pointers from the result geometry. */
VectorSet<Material *> all_result_materials;
original_materials
@ -1148,0 +1184,4 @@
for (Material *eval_material :
Span{grease_pencil_result.material_array, grease_pencil_result.material_array_num})
{
BLI_assert(eval_material->id.orig_id != nullptr);
Do we still able to have nullptr materials in empty slots?
@ -1148,0 +1192,4 @@
for (const int mat_i : IndexRange(grease_pencil_orig.material_array_num)) {
Material *material = grease_pencil_orig.material_array[mat_i];
const int map_index = all_result_materials.index_of_try(material);
BLI_assert(map_index != -1);
Does this imply that you can not add new materials to the GP in geometry nodes?
It's the opposite I think: We expect the evaluated geometry to have the materials of the original geometry (maybe more).
I mean, materials sets of the original object and of geometry which is result of the geometry nodes can completely do not intersect at all\
I tested this and it seems to work without hitting the assert.
@ -1148,0 +1205,4 @@
Drawing *drawing_eval = grease_pencil_result.get_drawing_at(*layer_eval, eval_frame);
if (drawing_eval) {
/* Anonymous attributes shouldn't be available on original geometry. */
drawing_eval->strokes_for_write().attributes_for_write().remove_anonymous();
Think you should split this loop body in separate steps\
Great to have applying modifiers working! I don't really have an opinion on the design-level, anything that generally works is a great start.
@ -1148,0 +1152,4 @@
if (mti->modify_geometry_set == nullptr) {
return false;
}
GreasePencil &grease_pencil_orig = *static_cast<GreasePencil *>(ob->data);
This should happen for all these if statement cases later, but I think this would be easier to read if it was split to a separate function
@ -1148,0 +1263,4 @@
bke::MutableAttributeAccessor attributes =
drawing.strokes_for_write().attributes_for_write();
bke::SpanAttributeWriter<int> material_indices =
attributes.lookup_or_add_for_write_span<int>("material_index", bke::AttrDomain::Curve);
There should be some case where we don't have to add the material index attribute, probably worth implementing that?
I'm not sure what that case could be
I'm thinking of when there are no materials, or when the attribute didn't exist on the original or evaluated geometry
@ -1148,0 +1273,4 @@
}
/* Copy layer attributes from the result to the original. */
bke::scatter_attributes(grease_pencil_result.attributes(),
We discussed just using a
attributes.for_all
loop rather than adding a new API function in chat. Just writing it here for history's sake.@ -988,0 +1129,4 @@
if (meta_data.data_type == CD_PROP_STRING) {
return true;
}
const GAttributeReader src = src_attributes.lookup(id, AttrDomain::Layer);
const GVArraySpan src = *src_attributes.lookup(id, AttrDomain::Layer);
Then you can remove the new scatter array utils function
@ -988,0 +1139,4 @@
using T = decltype(dummy);
array_utils::scatter(
src.varray.typed<T>(), eval_to_orig_layer_indices_map.as_span(), dst.span.typed<T>());
src.varray.type().default_construct_indices(dst.span.data(), unmapped_original_layers);
This should use
type.fill_construct_indices(type.default_value(), dst.span.data(), mask);
fill_construct_indices
7605bcd4a6@blender-bot build
@ -988,0 +1089,4 @@
for (Material *eval_material :
Span{grease_pencil_result.material_array, grease_pencil_result.material_array_num})
{
BLI_assert(eval_material->id.orig_id != nullptr);
Material slots can be null.
Ok then I'll just turn this into an
if
.I think we mean
eval_material
can be null@ -988,0 +1098,4 @@
for (const int mat_i : IndexRange(grease_pencil_orig.material_array_num)) {
Material *material = grease_pencil_orig.material_array[mat_i];
const int map_index = original_materials.index_of_try(material);
BLI_assert(map_index != -1);
It's possible that the generated grease pencil contains other materials.
Ok then I'll do the same as above: turn it into an
if
.If you have multiple sets of mateirals you should merge them\
When I apply the modifier in the attached file, I somehow still see Suzanne in the viewport.
@ -988,0 +1027,4 @@
for (const int layer_eval_i : grease_pencil_result.layers().index_range()) {
const Layer *layer_eval = grease_pencil_result.layer(layer_eval_i);
/* Check if the original geometry has a layer with the same name. */
TreeNode *node_orig = grease_pencil_orig.find_node_by_name(layer_eval->name());
Looks like this may be incorrect if there is a group with the same name already.
@ -988,0 +1060,4 @@
drawing_eval->strokes_for_write().attributes_for_write().remove_anonymous();
}
Drawing *drawing_orig = grease_pencil_orig.get_drawing_at(*layer_orig, eval_frame);
if (drawing_orig && drawing_eval) {
What happens when
drawing_orig
is null? Is the drawing just ignored?Yes. In that case the layer doesn't have a visible drawing at the current frame, so there's nothing to do.
@ -988,0 +1089,4 @@
for (Material *eval_material :
Span{grease_pencil_result.material_array, grease_pencil_result.material_array_num})
{
if (eval_material->id.orig_id != nullptr) {
eval_material
may be null.@ -988,0 +1095,4 @@
}
/* Build material indices mapping. */
Array<int> material_indices_map(grease_pencil_orig.material_array_num);
Would be good to mention what is mapped to what. I originally assumed that you map indices used on the result data to indices on the original data, but it seems to be the other way around?
@blender-bot build
I don't think this PR is working as expected. When you apply a modifier, you must apply to all frames.
For example, if you have an animation and you want increase the thickness, you want increase in all frames, not only current. I have attached a videos with GPv2 and GPv3 apply.