GPv3: Layer Parenting/Transforms #117247
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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#117247
Loading…
Reference in New Issue
No description provided.
Delete Branch "filedescriptor/blender:gpv3-layer-parenting"
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 layer parenting and layer transforms.
Implementation details:
@brecht @HooglyBoogly I'd like to get some feedback on the implementation of this before I go any further. The PR is marked WIP, but it is in a working state.
From the devtalk topic I understood you wanted to write the transform to attributes instead of affecting the strokes immediately. Is this an intermediate step towards that, or was there a change in plans?
@brecht This is just implementing the layer parenting. The layer transforms would have to be implemented on top of this (somehow).
It's just confusing to me to have a modifier named Transform, which applies the parenting transform to the strokes. From the name, you'd expect it to work like the Transform Geometry node, with translation/rotation/scale inputs.
So it would be good if you could give context like, this modifier is intended to gain functionality X, to be placed at the start/end/middle of the modifier stack, work as a virtual modifier node visible to users. Or if those things are unknown still, to mention that.
@ -38,0 +40,4 @@
static void rna_grease_pencil_dependency_update(Main *bmain, Scene * /*scene*/, PointerRNA *ptr)
{
DEG_id_tag_update(&rna_grease_pencil(ptr)->id, ID_RECALC_TRANSFORM);
This should be
ID_RECALC_GEOMETRY
, since here the transform is getting applied to the geometry.ID_RECALC_TRANSFORM
is for when the object matrix is affected.@ -38,0 +42,4 @@
{
DEG_id_tag_update(&rna_grease_pencil(ptr)->id, ID_RECALC_TRANSFORM);
DEG_relations_tag_update(bmain);
WM_main_add_notifier(NC_OBJECT | ND_PARENT, rna_grease_pencil(ptr));
Same here, this doesn't seem like the appropriate modifier if the mechanism is different. Probably
NC_GPENCIL | NA_EDITED
is better.@brecht Right, I was sort of following the way parenting works for
Object
where it creates a virtual armature modifier. My idea was that we could then also use this modifier to port the grease pencil offset modifier. This way, we don't just have a modifier that's only used for the single purpose of layer parenting.It doesn't really make sense to me to have the same modifier do parenting and be a replacement for the offset modifier. Parenting is something you want to apply exactly once, and at a particular position in the stack. So then you need to add some boolean to say if the modifier does parenting or not, which I don't think would be very clear.
@brecht Alright, in that case, I can maybe rename this modifier to be specific to parenting.
And I suppose the offset modifier (maybe should be named transform since it also changes roatation/scale) could be used to apply the layer transforms? Maybe with a checkbox that switches between using layer attributes and a single value (similar to how it works with geometry nodes modifiers).
If the transform attributes are a built-in feature that most grease pencil geometry nodes are expected to understand, similar to instance/object transforms, then I don't think applying them should be done in the offset modifier.
Most of the time users shouldn't be thinking of applying them at all, because as far as they are concerned they are already being applied automatically at some point before rendering.
The way I would imagine is more that you have a Transform modifier, that can either apply directly to the strokes or write to the transform attributes. This is what users use if they want to procedurally apply some transformation. Might have options to work in layer space or object space.
And then you could have an Apply Transform modifier that applies any transform attributes to the strokes. Maybe this modifier includes the parenting functionality. This is what gets applied automatically at the end of the modifier stack, or what a user may use manually if for some reason they want to bring all stroke vertex positions into the same space.
@ -0,0 +71,4 @@
}
return float4x4(float4x4_view(parent.object_to_world));
}();
drawing->strokes_for_write().transform(parent_matrix);
I think this should clear the parent pointer in the layer, so that any following modifier does not apply the parenting transform a second time.
Although I think you may want to write the parenting information to the transform attributes before any other modifiers, so that this transform can be taken into account. Which would mean you need 3 distinct modifiers. (Writing the parenting into the transform attributes doesn't necessarily need to be an actual modifier, could just be a piece of code that runs before modifiers too).
WIP: GPv3: Layer Parentingto WIP: GPv3: Layer Parenting/TransformsWIP: GPv3: Layer Parenting/Transformsto GPv3: Layer Parenting/Transforms@blender-bot build
This is mostly in a working state. I still have this issue of the transforms not updating when e.g. moving a parent and then right-clicking to cancel.
@blender-bot build
I tried to check if the transform attribute names match other things in geometry nodes. But it seems that for instances, it stores a 4x4 matrix instead, and not as an attribute.
So I'm curious about what the geometry nodes team thinks is the right way to store this, if and how it should be consistent with the rest of the design.
CC @JacquesLucke
@ -1219,0 +1375,4 @@
const float4x4 layer_matrix = math::from_loc_rot_scale<float4x4, math::EulerXYZ>(
translations[layer_i], rotations[layer_i], scales[layer_i]);
drawing->strokes_for_write().transform(layer_matrix);
}
I would expect this to remove or zero the transform attributes. So that later in the stack or nodes you can transform and apply again, without duplication.
The only reason the instance transforms are not stored as normal attribute in
CustomData
is that we don't have this custom data type yet. Once we have it, we want to move the transforms to an attribute inCustomData
.Any opinion on storing a matrix like instances vs. storing decomposed translation, rotation and scale as in this PR?
Some factors are:
I only choose the decomposed option because of the 1to1 correlation with the user interface. Storing it as a matrix (if it would be possible in custom data) would mean that the values in the UI could unexpectedly change (e.g. changing the scale can affect the rotation and vice versa)
If it would be stored as a matrix attribute in geometry nodes, then the properties would be regular DNA which get converted to a matrix attribute. In the same place the parent matrix gets read.
Regular objects and bones work the same way basically. The DNA properties get converted to a matrix early, which is what is then used in constraints and modifiers.
For instances we used the decomposed storage originally, but it became clear fairly quickly that we have to store the matrix instead. Otherwise the transformation would result in unexpected results because shear/skew is not supported but happens often when rotating and scaling. It's not clear to me why the situation would be different here.
Ok from what I understand, we cannot store matrices in custom data yet. So would the idea be to store
all in DNA? The three fields for the RNA properties and the matrix for actual calculations?
Will see if parts of #116166 can be merged before continuing this PR.
I think this is fine. The matrix will be runtime only data, so it is easy to remove from DNA once there is a better way to store it.
Updated the code and description:
I noticed that current files will initialize the scale to 0, because there is no versioning. Should I add versioning for this?
@blender-bot build
Looks fairly straightforward actually, especially since these places already have to deal with a transform, now it's just a different one.
@ -335,2 +327,4 @@
LayerTransformData trans_data_;
/* Runtime transform data. */
mutable SharedCache<float4x4> local_transform_;
mutable
is unnecessary I think,SharedCache
is "const" because it's threadsafe IIRC. But anyway, is it really worth putting this in a cache? It's just creating a matrix.I'd suggest just calculating it on demand, and only adding caching if you observe it improving performance in a real situation.
@ -242,2 +244,2 @@
DrawingTransforms::DrawingTransforms(const Object &grease_pencil_ob)
template<typename T>
static blender::MutableSpan<T> get_mutable_span_attribute(CustomData &custom_data,
Are these changes still necessary? Seems this isn't used
@ -686,2 +698,4 @@
this->opacity = other.opacity;
this->parent = other.parent;
BLI_strncpy(
STRNCPY
@ -264,9 +265,10 @@ bool select_box(const ViewContext &vc,
bool select_lasso(const ViewContext &vc,
bke::CurvesGeometry &curves,
Span<float3> deformed_positions,
float4x4 projection_matrix,
const reference (applies elsewhere in the PR too, I won't write it everywhere though)
@ -714,2 +713,2 @@
const float obmat[4][4],
float r_pmat[4][4])
blender::float4x4 ED_view3d_ob_project_mat_get_from_obmat(const RegionView3D *rv3d,
const blender::float4x4 obmat)
Nice, but how about committing this separately and switching to using the new math API inside? Also
obmat
should be a referenceCreated !117938
@ -153,9 +155,10 @@ static void recalcData_curves(TransInfo *t)
void curve_populate_trans_data_structs(TransDataContainer &tc,
blender::bke::CurvesGeometry &curves,
const blender::float4x4 &matrix,
matrix
->transform
@ -295,0 +296,4 @@
* Layer parent object. Can be an armature in which case the `parsubstr` is the bone name.
*/
struct Object *parent;
char parsubstr[64];
How about using
char *
to take up less space when it isn't used? (and to simplify assignment, etc.)@ -278,0 +277,4 @@
const Span<float3> positions = curves.positions();
MutableSpan<float3> positions_slice = edit_points.slice(points);
threading::parallel_for(curves.points_range(), 1024, [&](const IndexRange range) {
for (const int point_i : range) {
Could you split this inner loop to a function called
copy_transformed_positions
? I'm trying to use identical functions like that for when we get the time to extract that to a separate file or something.Sure! Would you want that as part of
BKE_curves.hh
?No, just local to this file
Can be a separate task: Add conversion for layer transforms in
grease_pencil_convert_legacy.cc
.Apart from that didn't spot anything that wasn't already mentioned.
@LukasTonne Since this was just 5 more lines, I added it to this PR.
Design is fine, will leave code review to others.
@blender-bot build
@ -769,3 +761,3 @@
const IndexMask &mask,
const bke::AttrDomain selection_domain,
const Span<int2> coords,
const Span<int2> lasso_coords,
Commit this name change separately too maybe?
Sure :)