GPv3: Fill texture coordinates system #119303
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.
Blocks
#119050 GPv3: Texture offset modifier
blender/blender
Reference: blender/blender#119303
Loading…
Reference in New Issue
No description provided.
Delete Branch "casey-bianco-davis/blender:GPv3-fill-texture-coordinates"
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 is implements the system texture coordinates for GPv3.
This pull request adds:
Cleanup comments\
@ -868,0 +878,4 @@
*/
void set_texture_matrix(blender::bke::greasepencil::Drawing &drawing,
int curve_i,
const blender::float4x2);
const blender::float4x2
->const blender::float4x2 &
orblender::float4x2
@ -868,0 +885,4 @@
*/
void transfer_texture_matrices(const blender::bke::greasepencil::Drawing &src,
blender::bke::greasepencil::Drawing &dst,
const blender::Span<int> dst_to_src_curve);
const blender::Span<int> dst_to_src_curve
->blender::Span<int> dst_to_src_curve
@ -868,0 +887,4 @@
blender::bke::greasepencil::Drawing &dst,
const blender::Span<int> dst_to_src_curve);
void transfer_texture_matrices(const blender::bke::greasepencil::Drawing &src,
blender::bke::greasepencil::Drawing &dst,
Order should be:
src, dst_to_src, dst
@ -1461,0 +1473,4 @@
using namespace blender::math;
const bke::CurvesGeometry &curves = drawing.strokes();
const offset_indices::OffsetIndices<int> points_by_curve = curves.points_by_curve();
offset_indices::OffsetIndices<int>
->OffsetIndices<int>
@ -1461,0 +1483,4 @@
return float4x2::identity();
}
const float3 pt0 = positions[points.first()];
pt0
->point_0
orpoint_a
@ -1461,0 +1489,4 @@
/* Local X axis (p0 -> p1) */
const float3 locx = normalize(pt1 - pt0);
/* Local Y axis (cross to normal/x axis). */
const float3 locy = normalize(cross(normal, locx));
This looks like better to call them as
normal
andtangent
I personally disagree, There already is a variable called
normal
and it for the direction outside of the stroke plane, where as both of these two vectors are in the stroke's plane.Although, I was wondering about naming them
locu
andlocv
This values are not locations, so i don't like
loc
naming.Good point, what about
dir
,direction
orbasic
?As i can see, there is 2 perpendicular vector, and this is the reason why i'd treat them as
normal
andtangent
.But this can be
base_normal
andbase_tangent
if you'd like that.I think just making the names a bit longer makes this explicit:
local_y
andlocal_x
. So there is no confusion withlocation
.@ -1461,0 +1544,4 @@
*/
/* Apply scale. */
textmat = from_scale<float2x2>(uv_scale_inv) * textmat;
const float3x2 textmat = rotation * from_scale<float2x2>(uv_scale_inv) + from_transform();
?const float3x2 textmat = rotation * from_scale<float2x2>(uv_scale_inv) + from_transform();
Does not quite work because the rotation times scale part is 2x2 where the transform part is 3x2 (order would also be wrong) and the
from_location
also includes a identity which would have to be removed.It could be written as a one liner as:
But I don't think this is as clear of what it does. What does @filedescriptor think?
I think the multi-line version is fine, no need to make this more compact.
That being said, I'm not really sure what happens when you multiple a 2x2 with a 3x2 matrix. The first matrix would need to have another column to make this work naively. Is the 2x2 scale matrix implicitly converted into a MatView?
Multiplying a 2x2 with a 3x2 is just fine, no conversions happen, blender uses the reverse row column convention so this is
2x2*3x2 = 2x3
, normal matrix multiplication is(AxB)*(BxC) = (AxC)
but bender uses(BxA)*(CxB) = (AxC)
@ -1461,0 +1693,4 @@
blender::bke::greasepencil::Drawing &dst,
const blender::IndexMask &dst_to_src_curve)
{
dst_to_src_curve.foreach_index([&](const int64_t index, const int64_t pos) {
Can not be parallel (
GrainSize(4096)
)?Added some comments on the
set_texture_matrix
andget_texture_matrix
functions.@ -868,0 +869,4 @@
* Returns the matrix that transforms from a 3D point in layer-space to a 2D point in
* texture-space for the stroke `curve_i`
*/
blender::float4x2 get_texture_matrix(const blender::bke::greasepencil::Drawing &drawing,
I think this function should not be a free function, but a member function on the
Drawing
class.Here is what I would propose:
Span<float4x2> texture_matrices() const;
curve_plane_normals()
, add amutable SharedCache<Vector<float4x2>> curve_texture_matrices;
that is lazily computed and returned (as aSpan
).tag_texture_matrices_changed()
function that is called intag_positions_changed()
that callsthis->runtime->curve_texture_matrices.tag_dirty();
@ -868,0 +876,4 @@
* Sets the matrix the that transforms from a 3D point in layer-space to a 2D point in
* texture-space for the stroke `curve_i`
*/
void set_texture_matrix(blender::bke::greasepencil::Drawing &drawing,
Similar to aboce, I think we should put this function in the
Drawing
.void set_texture_matrices(const VArray<float4x2> &matrices, const IndexMask &selection);
that computes and sets the matrices.tag_texture_matrices_changed()
at the end.@ -868,0 +883,4 @@
/*
* Copy the texture matrices from `src` to `dst` for the strokes in `dst_to_src_curve`
*/
void transfer_texture_matrices(const blender::bke::greasepencil::Drawing &src,
These seem to be unused. Any reason to have them in this patch?
@ -1461,0 +1605,4 @@
using namespace blender;
const float4x2 strokemat = get_local_to_stroke_matrix(drawing, curve_i);
const float3x2 textmat = get_stroke_to_texture_matrix(drawing.strokes(), curve_i);
I think the convention is to use
tex
for textures andtext
for, well, text.texmat
etc. is more consistent with other parts of Blender.@ -1461,0 +1662,4 @@
* Our problem has the form of: `X = A * Y`
* We can solve for `A` using: `A = X * B`
*
* Where `B` is the Right-sided inverse, calculated as:
If i'm not mistaken this is the Moore-Penrose pseudo inverse? Would be good to mention this name as a search term for others.
Some cleanup comments.
@ -415,0 +536,4 @@
return this->runtime->curve_texture_matrices.data().as_span();
}
static void set_stroke_to_texture_matrix(CurvesGeometry &curves, int curve_i, float3x2 texmat)
You can make this a
lambda
inDrawing::set_texture_matrices
. This way, theSpanAttributeWriter
s can be retrieved outside theselection.foreach_index
loop (and call.finish()
after the loop).@ -415,0 +585,4 @@
const float4x2 texspace = matrices[pos];
/*
* WORKAROUND: This algorithm could work with floats but is prone to numerical error.
Not sure this is really a workaround. Saying
We do the computation using doubles to avoid numerical precision errors
is fine imo.@ -159,0 +172,4 @@
const float2 center = float2(0.5f, 0.5f);
const float2 uv_scale_inv = math::safe_rcp(uv_scale);
const float2 d = maxv - minv;
d
->diagonal
?@ -159,0 +173,4 @@
const float2 uv_scale_inv = math::safe_rcp(uv_scale);
const float2 d = maxv - minv;
const float s = sin(uv_rotation);
s
->sin_rotation
c
->cos_rotation
@ -159,0 +175,4 @@
const float2 d = maxv - minv;
const float s = sin(uv_rotation);
const float c = cos(uv_rotation);
const float2x2 rot = float2x2(float2(c, s), float2(-s, c));
rot
->rotation
@ -159,0 +177,4 @@
const float c = cos(uv_rotation);
const float2x2 rot = float2x2(float2(c, s), float2(-s, c));
float3x2 texmat = float3x2::identity();
texmat
->texture_matrix
Some more cleanup comments. It's getting close imo. @HooglyBoogly Could you maybe take a look at this? Since you were also in the meeting about this.
@ -418,0 +479,4 @@
const float c = cos(uv_rotation);
const float2x2 rot = float2x2(float2(c, s), float2(-s, c));
float3x2 texmat = float3x2::identity();
This should also be
texture_matrix
like inget_legacy_stroke_to_texture_matrix
. I see other uses oftexmat
as well, please change them too.@ -159,0 +222,4 @@
const float3 pt3 = float3(point3->x, point3->y, point3->z);
/* Local X axis (p0 -> p1) */
const float3 locx = normalize(pt1 - pt0);
Like in
get_legacy_stroke_to_texture_matrix
better to name thislocal_x
to be a bit more clear.@ -159,0 +226,4 @@
/* Point vector at 3/4 */
const float3 v3 = totpoints == 2 ? pt3 * 0.001f : pt3;
const float3 loc3 = v3 - pt0;
I think the
v3
variable is not necessary.const float3 local_3 = (totpoints == 2) ? (pt3 * 0.001f) - pt0 : pt3 - pt0;
And maybe add a comment why we multiply by
0.001f
here.@ -418,0 +463,4 @@
using namespace blender::math;
const AttributeAccessor attributes = curves.attributes();
const VArray<float> uv_rotations = *attributes.lookup_or_default<float>(
It's very important not to look up attributes for every curve index. That should be done once before the parallel loop.
Same with
get_local_to_stroke_matrix
. Better to add the data the function needs as arguments. If it has lots of arguments, so be it-- it's using a lot of data, it's okay for the signature to reflect that.@ -418,0 +481,4 @@
float3x2 texmat = float3x2::identity();
/*
* The order in which the three transforms are applied, has been carefully chosen to be easy to
Comma unnecessary here
applied, has
->applied has
@ -418,0 +547,4 @@
SpanAttributeWriter<float> uv_rotations = attributes.lookup_or_add_for_write_span<float>(
"uv_rotation",
AttrDomain::Curve,
AttributeInitVArray(VArray<float>::ForSingle(0.0f, curves.curves_num())));
0 is already the default for new attributes, you can just leave this part out. Same with
uv_translations
@ -418,0 +557,4 @@
AttrDomain::Curve,
AttributeInitVArray(VArray<float2>::ForSingle(float2(1.0f, 1.0f), curves.curves_num())));
auto set_stroke_to_texture_matrix = [&](int curve_i, float3x2 texmat) {
If this lambda is only used once, I'm not sure it really helps. I'd just write the code directly where it's used. That or exact it to a separate function (rather than a lambda)
@ -418,0 +589,4 @@
*
* i.e.
* # # # # # # # #
* We need # # # # Instead of # # # #
Could you extract this to a function so the logic and comment doesn't need to be duplicated?
Some more cleanup comments
@ -370,0 +468,4 @@
const float4x2 legacy_texture_matrix = get_legacy_texture_matrix(gps);
/* Ensure that everything is up to date. */
drawing.tag_positions_changed();
I don't think this tag will do anything. The positions should already be tagged.
The positions where up to date, but the normals where not.
@ -553,10 +555,11 @@ static void grease_pencil_geom_batch_ensure(Object &object,
int point_i,
int idx,
float length,
float4x2 texture_matrix,
const float4x2 &
Tested this locally (with !118664) and the conversion works great. I got only one concern for how it behaves while drawing.
@ -486,1 +491,4 @@
float3 u_dir;
float3 v_dir;
float3 origin = float3(0.0f);
I think the origin needs to be based on the start sample (first point). Otherwise you might draw a fill where the texture is way out of place. This would also be consistent with GPv2. I do like how the texture is not rotated based on the first segment but rather sticks to the drawing plane. This (minus the origin) seems like a good improvement to keep.
Thanks, this looks correct to me now. Will accept, but note that I'd like to land this after !118664 so that the rendering works as well.
GPv3: Fill texture coordinates system.to GPv3: Fill texture coordinates system@blender-bot build
@blender-bot build
@blender-bot build
@ -74,2 +77,4 @@
void tag_topology_changed();
/*
* Returns the matrices that transforms from a 3D point in layer-space to a 2D point in
matrices that transforms
->matrices that transform
@ -76,0 +82,4 @@
*/
Span<float4x2> texture_matrices() const;
/*
* Sets the matrices the that transforms from a 3D point in layer-space to a 2D point in
Same
@ -429,0 +436,4 @@
using namespace blender::math;
const CurvesGeometry &curves = drawing.strokes();
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
These should all be retrieved just once rather than per curve as well. We just really don't want searching for attributes to show up in profiles. And for any hot loop like this, we should do the best we can to give the compiler all the tools to optimize it. Retrieving the input data at every iteration makes that much harder.
Looks like the function signature should be:
I would just pass the sliced positions then, to avoid also passing the index range.
The code returns
float4x2::identity()
in casepoints.size() <= 2
. It also usespoints.first()
. Doesn't feel like we should pull all that logic out of the function.points.size() <= 2
becomescurve_positions.size() <= 2
andpoints.first()
becomescurve_positions.first()
. That seems like an improvement to me TBHSure that seems a bit nicer indeed
@ -429,0 +568,4 @@
AttrDomain::Curve,
AttributeInitVArray(VArray<float2>::ForSingle(float2(1.0f, 1.0f), curves.curves_num())));
selection.foreach_index([&](const int64_t curve_i, const int64_t pos) {
Could this be parallel? (
GrainSize(256)
or so?)@blender-bot build