GPv3: Line Art modifier migration #117028

Merged
Falk David merged 51 commits from ChengduLittleA/blender:gp3-new-lineart into main 2024-02-26 15:28:25 +01:00
Member

This is a working migration of the old Line Art modifier. All the features stays the same for the moment until we get everything ported to GPv3.

Note:

  • The modifier is using the exact same DNA structure as the old one, it's re-defined in a different name. At the moment all the variable names and placement after the ModifierData part should stay exactly the same until we do proper versioning of the modifier data and completely remove the GPv2 support.
  • Vertex weight transfer feature no longer supports name initial matching ("group" used to match "group1","group2" etc). Now it will only transfer vertex weight from source vertex groups that has the exact same name as specified.
This is a working migration of the old Line Art modifier. All the features stays the same for the moment until we get everything ported to GPv3. Note: - The modifier is using the exact same DNA structure as the old one, it's re-defined in a different name. At the moment all the variable names and placement after the `ModifierData` part should stay exactly the same until we do proper versioning of the modifier data and completely remove the GPv2 support. - Vertex weight transfer feature no longer supports name initial matching ("group" used to match "group1","group2" etc). Now it will only transfer vertex weight from source vertex groups that has the exact same name as specified.
YimingWu added the
Interest
Grease Pencil
Interest
Line Art
Module
Grease Pencil
labels 2024-01-11 14:53:59 +01:00
YimingWu added 4 commits 2024-01-11 14:54:09 +01:00
Falk David added this to the Grease Pencil project 2024-01-11 14:59:38 +01:00
YimingWu added 2 commits 2024-01-16 02:50:53 +01:00
YimingWu added 2 commits 2024-01-23 10:27:10 +01:00
YimingWu added 1 commit 2024-01-23 15:34:16 +01:00
YimingWu added 1 commit 2024-01-23 15:42:42 +01:00
YimingWu force-pushed gp3-new-lineart from f89943385e to 426523746c 2024-01-24 03:53:24 +01:00 Compare
YimingWu requested review from Falk David 2024-01-24 03:59:35 +01:00
YimingWu requested review from Hans Goudey 2024-01-24 03:59:35 +01:00
YimingWu added 1 commit 2024-01-24 03:59:50 +01:00
YimingWu added 1 commit 2024-01-24 05:23:40 +01:00
YimingWu force-pushed gp3-new-lineart from 525df0ae74 to d7cbbe3f6c 2024-01-24 05:26:37 +01:00 Compare
Falk David requested changes 2024-01-24 17:39:02 +01:00
Dismissed
Falk David left a comment
Member

Did a fist pass on this. Haven't touch everything yet.

Most concerning part to me right now is the LineartCache pointer that you store in the object-data runtime and how that interacts with the modifier stack. I'm pretty sure that this breaks some design principles.

Would be good if you could explain why this is needed and how it impacts performance.

Did a fist pass on this. Haven't touch everything yet. Most concerning part to me right now is the `LineartCache` pointer that you store in the object-data runtime and how that interacts with the modifier stack. I'm pretty sure that this breaks some design principles. Would be good if you could explain why this is needed and how it impacts performance.
@ -721,6 +721,8 @@ class GreasePencilRuntime {
/* The frame on which the object was evaluated (only valid for evaluated object). */
int eval_frame = 0;
struct ::LineartCache *lineart_cache;
Member

What's the use of this cache? Why does it need to be stored on the object-data?

What's the use of this cache? Why does it need to be stored on the object-data?
Member

So I think this pointer should be stored on ModifierEvalContext instead.

  • Every modifier gets this context passed in.
  • Lineart can generate its cache the first time it is executed and populate the cache.
  • Lineart modifiers after the first, can read from the cache.

The ModifierEvalContext is created in grease_pencil_evaluate_modifiers. This means that we can free the cache at the end of the function.

This makes the lifetime of this cache very clear: It only exists during modifier stack evaluation.

So I think this pointer should be stored on `ModifierEvalContext` instead. * Every modifier gets this context passed in. * Lineart can generate its cache the first time it is executed and populate the cache. * Lineart modifiers after the first, can read from the cache. The `ModifierEvalContext` is created in `grease_pencil_evaluate_modifiers`. This means that we can free the cache at the end of the function. This makes the lifetime of this cache very clear: It only exists during modifier stack evaluation.
Author
Member

That does sound good. Let me try putting it there instead. Thanks!

That does sound good. Let me try putting it there instead. Thanks!
ChengduLittleA marked this conversation as resolved
@ -891,0 +938,4 @@
if (md->type != eModifierType_GreasePencilLineart) {
return false;
}
LISTBASE_FOREACH (ModifierData *, gmd, &ob->modifiers) {
Member

gmd -> md

`gmd` -> `md`
ChengduLittleA marked this conversation as resolved
@ -891,0 +947,4 @@
}
}
/* If we reach here it means md is not in ob's modifier stack. */
BLI_assert(false);
Member

This assert doesn't do anything.

This assert doesn't do anything.
ChengduLittleA marked this conversation as resolved
@ -5423,0 +5681,4 @@
curves[writer.index_range().last() + 1] = up_to_point;
blender::float4x4 mat(gp_obmat_inverse);
drawing.strokes_for_write() = blender::ed::greasepencil::create_drawing_data(
Member

Modifiers shouldn't use code from the editors. You can define a static function in this file.

Modifiers shouldn't use code from the editors. You can define a `static` function in this file.
ChengduLittleA marked this conversation as resolved
@ -5491,2 +5761,4 @@
modifier_calculation_flags);
}
void MOD_lineart_gpencil_generate_v3(LineartCache *cache,
Member

Most of the parameters should be const if they are just passed as parameters.

Most of the parameters should be const if they are just passed as parameters.
ChengduLittleA marked this conversation as resolved
@ -5492,1 +5762,4 @@
}
void MOD_lineart_gpencil_generate_v3(LineartCache *cache,
Object *ob,
Member

const Object &

`const Object &`
ChengduLittleA marked this conversation as resolved
@ -5493,0 +5763,4 @@
void MOD_lineart_gpencil_generate_v3(LineartCache *cache,
Object *ob,
struct GreasePencilDrawing &drawing,
Member

bke::greasepencil::Drawing &

`bke::greasepencil::Drawing &`
ChengduLittleA marked this conversation as resolved
@ -5493,0 +5773,4 @@
uchar mask_switches,
uchar material_mask_bits,
uchar intersection_mask,
int16_t thickness,
Member

I think it would be worth is to switch this to a const float radius.

I think it would be worth is to switch this to a `const float radius`.
Author
Member

I'll keep DNA thickness as int16_t to keep modifier data layout exactly the same as the old one.

I'll keep DNA thickness as `int16_t` to keep modifier data layout exactly the same as the old one.
Member

Alright, I can agree to not change the behavior/UI.

Alright, I can agree to not change the behavior/UI.
ChengduLittleA marked this conversation as resolved
@ -5493,0 +5803,4 @@
float gp_obmat_inverse[4][4];
invert_m4_m4(gp_obmat_inverse, ob->object_to_world);
lineart_gpencil_generate_v3(cache,
Member

I'm not sure why it's necessary to nest this further.

I'm not sure why it's necessary to nest this further.
ChengduLittleA marked this conversation as resolved
@ -0,0 +706,4 @@
modifier_subpanel_register(region_type, "bake", "Bake", nullptr, bake_panel_draw, panel_type);
}
static void generate_strokes_actual(const ModifierData &md,
Member

There's no need for this to be a function.

There's no need for this to be a function.
ChengduLittleA marked this conversation as resolved
@ -0,0 +738,4 @@
lmd.calculation_flags);
}
static void generate_strokes(ModifierData &md, Depsgraph *depsgraph, Object &ob, GreasePencil &gpd)
Member

gpd -> grease_pencil

`gpd` -> `grease_pencil`
ChengduLittleA marked this conversation as resolved
@ -0,0 +740,4 @@
static void generate_strokes(ModifierData &md, Depsgraph *depsgraph, Object &ob, GreasePencil &gpd)
{
GreasePencilLineartModifierData &lmd = (GreasePencilLineartModifierData &)md;
Member

Use c++ casts

Use c++ casts
ChengduLittleA marked this conversation as resolved
@ -0,0 +743,4 @@
GreasePencilLineartModifierData &lmd = (GreasePencilLineartModifierData &)md;
/* This will check whether layer/materials are non-null. */
if (is_disabled(nullptr, &md, false)) {
Member

Why is this call needed here?

Why is this call needed here?
ChengduLittleA marked this conversation as resolved
@ -0,0 +759,4 @@
MOD_lineart_destroy_render_data_v3(&lmd);
}
else {
if (!(lmd.flags & LRT_GPENCIL_USE_CACHE)) {
Member

Looks like you're using the old flags here. This should be LINEART_GPENCIL_USE_CACHE. Same for other places.

Looks like you're using the old flags here. This should be `LINEART_GPENCIL_USE_CACHE`. Same for other places.
Author
Member

That does make a good point. I'm not so sure about the approach of this, since for now the modifier data part stays exactly the same for v3 and v2 modifiers, I thought maybe they should use the exact same flags for the moment (as defined in the old DNA) to avoid accidental changes causing flags to mismatch etc.

I pass the new modifier data to the calculation function by memcpy-ing it to the old modifier data, so everything has to stay the same for now, we can have a more complex wrapper that manually assign all the flags if we changed the layout of the modifier data of v3 but it's not necessary for what we are doing right now.

Do you suggest that we keep v2 and v3 symbols as separated as possible? (Althougth I did copy all the flags and changed their names to LINEART_ from LRT_ tho, just not decided yet)

That does make a good point. I'm not so sure about the approach of this, since for now the modifier data part stays exactly the same for v3 and v2 modifiers, I thought maybe they should use the exact same flags for the moment (as defined in the old DNA) to avoid accidental changes causing flags to mismatch etc. I pass the new modifier data to the calculation function by memcpy-ing it to the old modifier data, so everything has to stay the same for now, we can have a more complex wrapper that manually assign all the flags if we changed the layout of the modifier data of v3 but it's not necessary for what we are doing right now. Do you suggest that we keep v2 and v3 symbols as separated as possible? (Althougth I did copy all the flags and changed their names to `LINEART_` from `LRT_` tho, just not decided yet)
ChengduLittleA marked this conversation as resolved
@ -0,0 +769,4 @@
}
/* Ensure we have a frame in the selected layer to put line art result in. */
bke::greasepencil::Layer &got_layer = node->as_layer();
Member

got_layer -> layer

`got_layer` -> `layer`
ChengduLittleA marked this conversation as resolved
@ -0,0 +770,4 @@
/* Ensure we have a frame in the selected layer to put line art result in. */
bke::greasepencil::Layer &got_layer = node->as_layer();
got_layer.add_frame(0, 0, 0);
Member

This code seems a bit sketchy. I think you should first try and get a drawing and only insert a new one if there is not a drawing already. Something like

bke::greasepencil::Drawing &drawing = [&]() {
   if (bke::greasepencil::Drawing *drawing = grease_pencil.get_editable_drawing_at(layer, current_frame)) {
      return *drawing;
   }
   grease_pencil.insert_blank_frame(layer, current_frame, 0, 0);
   return *grease_pencil.get_editable_drawing_at(layer, current_frame);
}();
This code seems a bit sketchy. I think you should first try and get a drawing and only insert a new one if there is not a drawing already. Something like ```cpp bke::greasepencil::Drawing &drawing = [&]() { if (bke::greasepencil::Drawing *drawing = grease_pencil.get_editable_drawing_at(layer, current_frame)) { return *drawing; } grease_pencil.insert_blank_frame(layer, current_frame, 0, 0); return *grease_pencil.get_editable_drawing_at(layer, current_frame); }(); ```
Author
Member

Thanks for the suggestion, however should we clear the drawing if the drawing at that frame already exists?

Thanks for the suggestion, however should we clear the drawing if the drawing at that frame already exists?
Member

Seems like the rest of the code will overwrite the geometry anyway.

Seems like the rest of the code will overwrite the geometry anyway.
ChengduLittleA marked this conversation as resolved
@ -0,0 +826,4 @@
/*struct_name*/ "GreasePencilLineartModifierData",
/*struct_size*/ sizeof(GreasePencilLineartModifierData),
/*srna*/ &RNA_GreasePencilLineartModifier,
/*type*/ ModifierTypeType::Nonconstructive,
Member

ModifierTypeType::Constructive

`ModifierTypeType::Constructive`
Author
Member

But it will delete old geometry?

But it will delete old geometry?
Member

Right, might be fine to keep Nonconstructive then.

Right, might be fine to keep `Nonconstructive` then.
ChengduLittleA marked this conversation as resolved
Author
Member

@filedescriptor

The idea of using LineartCache in object runtime is that line art only needs to run once (if the use_cache options are enabled) for multiple line art modifiers, and those modifiers only selectively generate strokes from 1 line art result, it's the same as the GPv2 line art implementation which is also in gpd->runtime. Of course after GPv2 has been removed we can have new designs like nodes and stuff but at the moment it's just to move it directly to v3 without much changes otherwise it's gonna be confusing.

I'll first clean up these notes.

@filedescriptor The idea of using `LineartCache` in object runtime is that line art only needs to run once (if the `use_cache` options are enabled) for multiple line art modifiers, and those modifiers only selectively generate strokes from 1 line art result, it's the same as the GPv2 line art implementation which is also in `gpd->runtime`. Of course after GPv2 has been removed we can have new designs like nodes and stuff but at the moment it's just to move it directly to v3 without much changes otherwise it's gonna be confusing. I'll first clean up these notes.
YimingWu added 1 commit 2024-01-25 04:20:27 +01:00
Member
@ChengduLittleA See https://projects.blender.org/blender/blender/pulls/117028#issuecomment-1109578
YimingWu added 3 commits 2024-01-29 04:33:32 +01:00
Falk David requested changes 2024-01-29 11:29:36 +01:00
Dismissed
Falk David left a comment
Member

Did a pass on the eval context changes.

Did a pass on the eval context changes.
@ -162,1 +162,4 @@
ModifierApplyFlag flag;
/* Line Art modifier cache will exist until the end of modifier stack evaluation. */
struct ::LineartCache *lineart_cache;
Member

Would be good to default this to nullptr.
struct ::LineartCache *lineart_cache = nullptr;

Would be good to default this to `nullptr`. `struct ::LineartCache *lineart_cache = nullptr;`
ChengduLittleA marked this conversation as resolved
@ -1264,6 +1268,17 @@ GreasePencil *BKE_grease_pencil_copy_for_eval(const GreasePencil *grease_pencil_
return grease_pencil;
}
static struct ::LineartCache *init_lineart_cache()
Member

This function is not needed.

This function is not needed.
ChengduLittleA marked this conversation as resolved
@ -1267,0 +1273,4 @@
return MOD_lineart_init_cache();
}
static void clear_lineart_cache(struct ::LineartCache *lc)
Member

This function is also not needed.

This function is also not needed.
ChengduLittleA marked this conversation as resolved
@ -1277,3 +1292,3 @@
}
ModifierApplyFlag apply_flag = use_render ? MOD_APPLY_RENDER : MOD_APPLY_USECACHE;
const ModifierEvalContext mectx = {depsgraph, object, apply_flag};
LineartCache *lineart_cache = init_lineart_cache();
Member

I don't think we should initialize the lineart cache in all cases.

I don't think we should initialize the lineart cache in all cases.
Author
Member

Yeah it should only initialize when there's line art modifier present. Originally the cache is written into the grease pencil data structure but now in ModifierEvalContext, which is const so line art function can't change the pointer in there. That's why I put a separate init call before evaluation. I'll check modifier first then.

Yeah it should only initialize when there's line art modifier present. Originally the cache is written into the grease pencil data structure but now in `ModifierEvalContext`, which is `const` so line art function can't change the pointer in there. That's why I put a separate init call before evaluation. I'll check modifier first then.
Member

Ah it's ok for ModifierEvalContext to not be declared const here.

Ah it's ok for `ModifierEvalContext` to not be declared const here.
ChengduLittleA marked this conversation as resolved
@ -1278,2 +1293,3 @@
ModifierApplyFlag apply_flag = use_render ? MOD_APPLY_RENDER : MOD_APPLY_USECACHE;
const ModifierEvalContext mectx = {depsgraph, object, apply_flag};
LineartCache *lineart_cache = init_lineart_cache();
const ModifierEvalContext mectx = {depsgraph, object, apply_flag, lineart_cache};
Member

I'd prefer something like

const ModifierEvalContext mectx = {depsgraph, object, apply_flag};
if (grease_pencil_has_lineart_modifier(ob)) {
   mectx.lineart_cache = MOD_lineart_init_cache();
}
I'd prefer something like ``` const ModifierEvalContext mectx = {depsgraph, object, apply_flag}; if (grease_pencil_has_lineart_modifier(ob)) { mectx.lineart_cache = MOD_lineart_init_cache(); } ```
ChengduLittleA marked this conversation as resolved
@ -1298,2 +1322,4 @@
}
}
clear_lineart_cache(lineart_cache);
Member
if (mectx.lineart_cache != nullptr) {
   MOD_lineart_clear_cache(&mtx.lineart_cache);
}
``` if (mectx.lineart_cache != nullptr) { MOD_lineart_clear_cache(&mtx.lineart_cache); } ```
Author
Member

Can't do it exactly like this since mectx is const.

Can't do it exactly like this since mectx is const.
Member

Right, mectx can be declared non-const :)

Right, `mectx` can be declared non-const :)
Author
Member

The problem is that the evaluation call function is also const, we probably don't want to change this in all the places?

The problem is that the evaluation call function is also `const`, we probably don't want to change this in all the places?
Member

No. There is a couple of ways to deal with this. I propose the following:

  • We add a std::unique_ptr<LineartCache> lineart_cache; to the ModifierEvalContext.
  • Using std::unique_ptr means that we need to add a destructor to LineartCache. I guess that would just be MOD_lineart_clear_cache basically.
  • We add a function get to LineartCache that computes the cache lazily. E.g. the first time the function is called, the cache is populated and any subuquent calls will just return the cache directly.
  • This means that the ModifierEvalContext can still be const, because we're modifing something inside that pointer that is not const.
No. There is a couple of ways to deal with this. I propose the following: * We add a `std::unique_ptr<LineartCache> lineart_cache;` to the `ModifierEvalContext`. * Using `std::unique_ptr` means that we need to add a destructor to `LineartCache`. I guess that would just be `MOD_lineart_clear_cache` basically. * We add a function `get` to `LineartCache` that computes the cache lazily. E.g. the first time the function is called, the cache is populated and any subuquent calls will just return the cache directly. * This means that the `ModifierEvalContext` can still be const, because we're modifing something inside that pointer that is not const.
Author
Member

I see... But then this looks largely the same as:

  • Init an empty cache (which it just allocates memory).
  • Assign the cache to ModifierEvalContext.
  • Line art function would modify stuff inside that cache pointer.
  • Free cache (equivalent of deconstruct?)
I see... But then this looks largely the same as: - Init an empty cache (which it just allocates memory). - Assign the cache to `ModifierEvalContext`. - Line art function would modify stuff inside that cache pointer. - Free cache (equivalent of deconstruct?)
Member

Line art function would modify stuff inside that cache pointer.

I don't think that works if the ModifierEvalContext is passed as const. You wouldn't be able to change the pointer.

> Line art function would modify stuff inside that cache pointer. I don't think that works if the `ModifierEvalContext` is passed as `const`. You wouldn't be able to change the pointer.
Author
Member

Hummm so currently I just pre-create that cache and set the pointer inside ModifierEvalContext, and they can modify stuff inside. Do we still need to make it into a class and have constructor and destructor? 🤔

Hummm so currently I just pre-create that cache and set the pointer inside `ModifierEvalContext`, and they can modify stuff inside. Do we still need to make it into a class and have constructor and destructor? 🤔
ChengduLittleA marked this conversation as resolved
@ -911,6 +924,8 @@ LineartBoundingArea *MOD_lineart_get_parent_bounding_area(LineartData *ld, doubl
*/
LineartBoundingArea *MOD_lineart_get_bounding_area(LineartData *ld, double x, double y);
struct GreasePencilDrawing;
Member

Is this needed?

Is this needed?
Author
Member

Looks like it isn't.

Looks like it isn't.
ChengduLittleA marked this conversation as resolved
@ -7962,0 +8328,4 @@
"Match the beginning of vertex group names from mesh objects, match all when left empty");
RNA_def_property_update(prop, 0, "rna_Modifier_update");
// prop = RNA_def_property(srna, "vertex_group", PROP_STRING, PROP_NONE);
Member

Even though it might not do anything right now, this should be uncommented.

Even though it might not do anything right now, this should be uncommented.
Author
Member

Right this is for vertex weight transferring. I guess I'll keep the property there for now.

Right this is for vertex weight transferring. I guess I'll keep the property there for now.
ChengduLittleA marked this conversation as resolved
@ -17,6 +17,9 @@ set(INC
../render
../windowmanager
../../../intern/eigen
../gpencil_modifiers_legacy
Member

Are these needed?

Are these needed?
Author
Member

Those are needed for the calls to shared legacy line art functions.

Those are needed for the calls to shared legacy line art functions.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-01-29 12:04:00 +01:00
YimingWu added 1 commit 2024-01-29 12:10:03 +01:00
YimingWu added 1 commit 2024-01-29 12:21:02 +01:00
Hans Goudey requested changes 2024-01-30 04:33:10 +01:00
Dismissed
@ -554,1 +557,4 @@
/* Stores the maximum calculation range in the whole modifier stack for line art so the cache can
* cover everything that will be visible. */
typedef struct GreasePencilLineartLimitInfo {
Member

typedef is unnecessary in C++ code

`typedef` is unnecessary in C++ code
ChengduLittleA marked this conversation as resolved
@ -555,0 +558,4 @@
/* Stores the maximum calculation range in the whole modifier stack for line art so the cache can
* cover everything that will be visible. */
typedef struct GreasePencilLineartLimitInfo {
short edge_types;
Member

Use integer types with specific widths, int16_t or int8_t for example

Use integer types with specific widths, `int16_t` or `int8_t` for example
ChengduLittleA marked this conversation as resolved
@ -555,0 +568,4 @@
bool BKE_grease_pencil_has_lineart_modifier(const Object *ob);
GreasePencilLineartLimitInfo BKE_grease_pencil_get_lineart_modifier_limits(
const struct Object *ob);
Member

struct is unnecessary in C++

`struct` is unnecessary in C++
ChengduLittleA marked this conversation as resolved
@ -891,0 +905,4 @@
bool is_first = true;
LISTBASE_FOREACH (ModifierData *, md, &ob->modifiers) {
if (md->type == eModifierType_GreasePencilLineart) {
auto *lmd = reinterpret_cast<GreasePencilLineartModifierData *>(md);
Member

const ModifierData *,

`const ModifierData *,`
ChengduLittleA marked this conversation as resolved
@ -891,0 +948,4 @@
if (md->type != eModifierType_GreasePencilLineart) {
return false;
}
LISTBASE_FOREACH (ModifierData *, i_md, &ob->modifiers) {
Member

const ModifierData *

`const ModifierData *`
ChengduLittleA marked this conversation as resolved
@ -3547,0 +3554,4 @@
void MOD_lineart_wrap_modifier_v3(LineartGpencilModifierData *lmd_legacy,
GreasePencilLineartModifierData *lmd)
{
memcpy((reinterpret_cast<char *>(lmd)) + sizeof(ModifierData) +
Member

Personally I'd just copy the fields manually, this is just unnecessarily fragile and weird

Personally I'd just copy the fields manually, this is just unnecessarily fragile and weird
ChengduLittleA marked this conversation as resolved
@ -5015,3 +5051,1 @@
if (lmd->calculation_flags & LRT_USE_CUSTOM_CAMERA) {
if (!lmd->source_camera ||
(lineart_camera = DEG_get_evaluated_object(depsgraph, lmd->source_camera))->type !=
if (lmd.calculation_flags & LRT_USE_CUSTOM_CAMERA) {
Member

+1 for the pointer to reference change! But let's leave that sort of cleanup for separate cleanup commits

+1 for the pointer to reference change! But let's leave that sort of cleanup for separate cleanup commits
ChengduLittleA marked this conversation as resolved
@ -5493,0 +5786,4 @@
return;
}
blender::Array<blender::float3> positions(total_point_count);
Member

You can just create the bke::CurvesGeometry here and fill in the attributes directly below

You can just create the `bke::CurvesGeometry` here and fill in the attributes directly below
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-01-30 07:22:23 +01:00
YimingWu added 1 commit 2024-01-30 07:27:32 +01:00
YimingWu added 2 commits 2024-01-30 13:47:16 +01:00
YimingWu changed title from WIP: GPv3: Line Art modifier migration to GPv3: Line Art modifier migration 2024-01-31 02:30:15 +01:00
YimingWu added 1 commit 2024-01-31 02:40:34 +01:00
YimingWu added 1 commit 2024-01-31 03:33:03 +01:00
Author
Member

Updated to include vertex weight transferring feature, note that it doesn't work exactly the same way due to the change of attribute API. See PR description.

Updated to include vertex weight transferring feature, note that it doesn't work exactly the same way due to the change of attribute API. See PR description.
Author
Member

OMG why there are so many commits here after rebasing...

OMG why there are so many commits here after rebasing...
YimingWu force-pushed gp3-new-lineart from 69151b2f1d to 9b0a390a42 2024-02-05 11:23:39 +01:00 Compare
YimingWu added 1 commit 2024-02-05 11:23:54 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
64eec74b6d
Merge branch 'main' into gp3-new-lineart
Author
Member

Fixed the patch with a force push but not sure how to get rid of the erroneous commit message on this page...

Fixed the patch with a force push but not sure how to get rid of the erroneous commit message on this page...
Member

@ChengduLittleA Don't worry, that's a gitea thing..

@ChengduLittleA Don't worry, that's a gitea thing..
Member

@blender-bot build

@blender-bot build
YimingWu added 2 commits 2024-02-16 03:14:07 +01:00
Hans Goudey requested changes 2024-02-16 17:31:39 +01:00
Dismissed
@ -162,1 +162,4 @@
ModifierApplyFlag flag;
/* Line Art modifier cache will exist until the end of modifier stack evaluation. */
struct ::LineartCache *lineart_cache = nullptr;
Member

Adding the cache for a single modifier type doesn't really make sense here. This is a more general struct that shouldn't know about any one modifier type in particular.

Generally runtime data can be saved in a modifier data struct directly. For example: NodesModifierData::runtime. It would probably make sense to do the same thing here: When the first line art modifier executes, store the cache there, and access the first modifier's runtime data in the next modifiers.

Adding the cache for a single modifier type doesn't really make sense here. This is a more general struct that shouldn't know about any one modifier type in particular. Generally runtime data can be saved in a modifier data struct directly. For example: `NodesModifierData::runtime`. It would probably make sense to do the same thing here: When the first line art modifier executes, store the cache there, and access the first modifier's runtime data in the next modifiers.
Member

Didn't know about the runtime for modifiers. Indeed, it makes more sense to put the cache there.

Didn't know about the runtime for modifiers. Indeed, it makes more sense to put the cache there.
Author
Member

I now moved it to GreasePencilLineartModifierData.

I now moved it to `GreasePencilLineartModifierData`.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-18 05:20:46 +01:00
YimingWu added 1 commit 2024-02-18 05:25:14 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
6b5ac9df32
Merge branch 'main' into gp3-new-lineart
Author
Member

@blender-bot build

@blender-bot build
Member

The PR has lots of conflicts, could you update it to latest main?

The PR has lots of conflicts, could you update it to latest main?
YimingWu added 1 commit 2024-02-20 01:36:38 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
b3e71f2025
Merge branch 'main' into gp3-new-lineart
Author
Member

@blender-bot build

@blender-bot build
YimingWu added 1 commit 2024-02-21 15:11:05 +01:00
Hans Goudey requested changes 2024-02-21 15:37:58 +01:00
Dismissed
@ -1371,0 +1378,4 @@
GreasePencilLineartModifierData *first_lineart = BKE_grease_pencil_get_first_lineart_modifier(
object);
if (first_lineart) {
first_lineart->shared_cache = MOD_lineart_init_cache();
Member

Don't you only want to add this cache if there's a second line art modifier that will use the data?

Don't you only want to add this cache if there's a second line art modifier that will use the data?
Author
Member

Eh yeah but the line art code path expect the cache to be always available because the final result is in there even if there's only one line art modifier in the stack. And here it adds the cache for the first one, later if more line art modifiers exists, it will refer to this cache internally.

Eh yeah but the line art code path expect the cache to be always available because the final result is in there even if there's only one line art modifier in the stack. And here it adds the cache for the first one, later if more line art modifiers exists, it will refer to this cache internally.
Member

It feels like all of this logic could just be in the modify_geometry_set function of the lineart modifier.
Since this function (grease_pencil_evaluate_modifiers) is really only supposed to run the modifiers, having special handling for lineart in here is not ideal.
I propose the following:

  1. In the lineart modifiy_geometry_set call GreasePencilLineartModifierData *first_lineart = BKE_grease_pencil_get_first_lineart_modifier(ctx.object); to get the first lineart modifier.
  2. const bool is_first_lineart = (first_lineart == lmd);
  3. if we are in the first lineart modifier, init the cache
  4. Get the limits info with BKE_grease_pencil_get_lineart_modifier_limits(ctx.object);
  5. Call BKE_grease_pencil_set_lineart_modifier_limits(md, &info, is_first_lineart);
  6. If lmd is the last lineart modifier, call MOD_lineart_clear_cache(&first_lineart->shared_cache);
It feels like all of this logic could just be in the `modify_geometry_set` function of the lineart modifier. Since this function (`grease_pencil_evaluate_modifiers`) is really only supposed to run the modifiers, having special handling for lineart in here is not ideal. I propose the following: 1. In the lineart `modifiy_geometry_set` call `GreasePencilLineartModifierData *first_lineart = BKE_grease_pencil_get_first_lineart_modifier(ctx.object);` to get the first lineart modifier. 2. `const bool is_first_lineart = (first_lineart == lmd);` 3. if we are in the first lineart modifier, init the cache 4. Get the limits info with `BKE_grease_pencil_get_lineart_modifier_limits(ctx.object);` 5. Call `BKE_grease_pencil_set_lineart_modifier_limits(md, &info, is_first_lineart);` 6. If `lmd` is the last lineart modifier, call `MOD_lineart_clear_cache(&first_lineart->shared_cache);`
ChengduLittleA marked this conversation as resolved
@ -35,6 +35,7 @@
#include "BLI_linklist.h"
#include "BLI_listbase.h"
#include "BLI_math_base.hh"
Member

Same here, std::min and std::max instead

Same here, `std::min` and `std::max` instead
ChengduLittleA marked this conversation as resolved
@ -13,6 +13,8 @@
#include "BLI_math_vector.h"
#include "BLI_threads.h"
#include "BKE_grease_pencil.hh"
Member

Better to use forward declarations and avoid indirect header includes

Better to use forward declarations and avoid indirect header includes
ChengduLittleA marked this conversation as resolved
@ -3540,0 +3558,4 @@
LMD_WRAP(source_object);
LMD_WRAP(source_collection);
LMD_WRAP(target_material);
strcpy(lmd->source_vertex_group, lmd_legacy->source_vertex_group);
Member
`STRNCPY` https://developer.blender.org/docs/handbook/guidelines/best_practice_c_cpp/#avoid-unsafe-string-c-apis
ChengduLittleA marked this conversation as resolved
@ -5486,0 +5674,4 @@
bool inverse_silhouette = modifier_flags & LRT_GPENCIL_INVERT_SILHOUETTE_FILTER;
blender::Vector<LineartChainWriteInfo> writer = {};
Member

= {} is unnecessary, Vector has a default constructor

`= {}` is unnecessary, Vector has a default constructor
ChengduLittleA marked this conversation as resolved
@ -5486,0 +5803,4 @@
}
bke::CurvesGeometry new_curves;
new_curves.resize(total_point_count, stroke_count);
Member

bke::CurvesGeometry new_curves(total_point_count, stroke_count);

`bke::CurvesGeometry new_curves(total_point_count, stroke_count);`
ChengduLittleA marked this conversation as resolved
@ -5486,0 +5876,4 @@
point_opacities.finish();
stroke_materials.finish();
drawing.strokes_for_write() = new_curves;
Member

std::move(new_curves)

`std::move(new_curves)`
ChengduLittleA marked this conversation as resolved
@ -0,0 +157,4 @@
* 2) Shadow render buffer if 3rd stage reprojection is need for silhouette/lit/shaded region
* selection. Otherwise the shadow render buffer is deleted before this function returns.
*/
bool lineart_main_try_generate_shadow(struct Depsgraph *depsgraph,
Member

Better to do this file rename in a separate commit afterwards

Better to do this file rename in a separate commit afterwards
ChengduLittleA marked this conversation as resolved
@ -10,9 +10,12 @@
#include <climits>
#include <cstdlib>
#include "BLI_math_base.hh"
Member

Maybe someone has told you otherwise, but in this case it's better to just use std::max than include the full math base header.

Maybe someone has told you otherwise, but in this case it's better to just use `std::max` than include the full math base header.

That is too sade the fact that a lot of pretty common bli headers (like BLI_index_range.hh) have includes of many std headers. 90% of std functions can be added in any .cc file on whole blender even without check for required header and this will compile...

That is too sade the fact that a lot of pretty common bli headers (like `BLI_index_range.hh`) have includes of many std headers. 90% of std functions can be added in any `.cc` file on whole blender even without check for required header and this will compile...
Author
Member

Humm so does this mean it would be better for me to include std headers explicitly? Or leave it as-is?

Humm so does this mean it would be better for me to include std headers explicitly? Or leave it as-is?

In not really near future, modules would fix includes-leak issue, but for now you can just use std::max everywhere. I'd still would like to use internal blender wrappers.. but if this is another one additional header, and there is no any other such math functions to use...

In not really near future, modules would fix includes-leak issue, but for now you can just use std::max everywhere. I'd still would like to use internal blender wrappers.. but if this is another one additional header, and there is no any other such math functions to use...
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-22 01:03:26 +01:00
YimingWu added 1 commit 2024-02-22 01:04:20 +01:00
Author
Member

I'll rename MOD_lineart.h and lineart_intern.h into .hh afterwards.

I'll rename `MOD_lineart.h` and `lineart_intern.h` into `.hh` afterwards.
YimingWu added 1 commit 2024-02-22 07:18:50 +01:00
YimingWu added 1 commit 2024-02-22 07:21:57 +01:00
Author
Member

Renamed all old LRT_ flags into LINEART_ and put them all inside DNA_modifier_types.h so that the old gpencil dna file only includes the old modifier structure.

Renamed all old `LRT_` flags into `LINEART_` and put them all inside `DNA_modifier_types.h` so that the old gpencil dna file only includes the old modifier structure.
Falk David requested review from Lukas Tönne 2024-02-23 10:25:01 +01:00
Falk David requested changes 2024-02-23 10:54:40 +01:00
Dismissed
Falk David left a comment
Member

Added some more comments.

Added some more comments.
@ -871,6 +871,76 @@ void BKE_modifier_path_init(char *path, int path_maxncpy, const char *name)
BLI_path_join(path, path_maxncpy, blendfile_path[0] ? "//" : BKE_tempdir_session(), name);
}
GreasePencilLineartLimitInfo BKE_grease_pencil_get_lineart_modifier_limits(const Object *ob)
Member

Should be a reference here, since the function expects it to be != nullptr: const Object &object

Should be a reference here, since the function expects it to be `!= nullptr`: `const Object &object`
ChengduLittleA marked this conversation as resolved
@ -874,0 +893,4 @@
return info;
}
void BKE_grease_pencil_set_lineart_modifier_limits(ModifierData *md,
Member

Seems like this function should just get a GreasePencilLineartModifierData &lmd directly.

Seems like this function should just get a `GreasePencilLineartModifierData &lmd` directly.
ChengduLittleA marked this conversation as resolved
@ -874,0 +894,4 @@
}
void BKE_grease_pencil_set_lineart_modifier_limits(ModifierData *md,
const GreasePencilLineartLimitInfo *info,
Member

const GreasePencilLineartLimitInfo &info

`const GreasePencilLineartLimitInfo &info`
ChengduLittleA marked this conversation as resolved
@ -874,0 +931,4 @@
return false;
}
GreasePencilLineartModifierData *BKE_grease_pencil_get_first_lineart_modifier(const Object *ob)
Member

const Object &object

`const Object &object`
ChengduLittleA marked this conversation as resolved
@ -5486,0 +5796,4 @@
stroke_count++;
/* TODO: Debug random color feature not implemented for v3 yet. */
Member

Is this still true?

Is this still true?
Author
Member

Yes... But I haven't been really needing that for debug purposes... Also I'm not sure if GPv3 support vertex colors?

Yes... But I haven't been really needing that for debug purposes... Also I'm not sure if GPv3 support vertex colors?
Member

We do have vertex colors.

We do have vertex colors.
Author
Member

Do we have a equivalent of BKE_gpencil_stroke_set_random_color for v3? Or should I make a shared one somewhere? Looks like the original function is only needed inside old line art, so I think it's okay we don't include this thing since I don't really use it for debug anyway.

Do we have a equivalent of `BKE_gpencil_stroke_set_random_color` for v3? Or should I make a shared one somewhere? Looks like the original function is only needed inside old line art, so I think it's okay we don't include this thing since I don't really use it for debug anyway.
filedescriptor marked this conversation as resolved
@ -0,0 +733,4 @@
/* Ensure we have a frame in the selected layer to put line art result in. */
bke::greasepencil::Layer &layer = node->as_layer();
layer.add_frame(0, 0, 0);
Member

This line shouldn't be necessary. The logic below will ensure a keyframe exists.

This line shouldn't be necessary. The logic below will ensure a keyframe exists.
ChengduLittleA marked this conversation as resolved
@ -0,0 +759,4 @@
lmd.mask_switches,
lmd.material_mask_bits,
lmd.intersection_mask,
(float)lmd.thickness / 1000,
Member

float(lmd.thickness) / 1000.0f

`float(lmd.thickness) / 1000.0f`
ChengduLittleA marked this conversation as resolved
Lukas Tönne requested changes 2024-02-23 11:33:07 +01:00
Dismissed
@ -11,6 +11,7 @@ set(INC
../draw
../functions
../gpencil_modifiers_legacy
../gpencil_modifiers_legacy/intern/lineart
Member

These headers should be moved to a more visible place instead of adding internal folders as include paths.

These headers should be moved to a more visible place instead of adding internal folders as include paths.
ChengduLittleA marked this conversation as resolved
@ -869,1 +870,3 @@
void MOD_lineart_destroy_render_data(struct LineartGpencilModifierData *lmd);
void MOD_lineart_wrap_modifier_v3(const LineartGpencilModifierData *lmd_legacy,
GreasePencilLineartModifierData *lmd);
void MOD_lineart_unwrap_modifier_v3(LineartGpencilModifierData *lmd_legacy,
Member

Why have GPv3 functions in a legacy header, wouldn't it make more sense to move these into a new header?

Why have GPv3 functions in a legacy header, wouldn't it make more sense to move these into a new header?
Author
Member

Because the line art calculation code is shared between the two atm and those wrapping function is needed to pass data between them.

Because the line art calculation code is shared between the two atm and those wrapping function is needed to pass data between them.
Member

Ok, should be easy to clean up later.

Ok, should be easy to clean up later.
ChengduLittleA marked this conversation as resolved
@ -2990,0 +3057,4 @@
typedef struct GreasePencilLineartModifierData {
ModifierData modifier;
GreasePencilModifierInfluenceData influence;
Member

I think the line art modifier shouldn't be using this influence struct. The only thing it uses is the layer name, which can easily be a regular property. It doesn't utilize any of the other utility functions that make using this struct convenient for other modifiers.

I think the line art modifier shouldn't be using this influence struct. The only thing it uses is the layer name, which can easily be a regular property. It doesn't utilize any of the other utility functions that make using this struct convenient for other modifiers.
ChengduLittleA marked this conversation as resolved
YimingWu added 2 commits 2024-02-23 11:54:47 +01:00
Falk David requested changes 2024-02-23 12:22:42 +01:00
Dismissed
@ -0,0 +793,4 @@
if (is_first_lineart) {
mmd->shared_cache = MOD_lineart_init_cache();
info = BKE_grease_pencil_get_lineart_modifier_limits(ctx->object);
LISTBASE_FOREACH (ModifierData *, imd, &ctx->object->modifiers) {
Member

There is no need for this loop. You can just put this above the if

GreasePencilLineartLimitInfo info = BKE_grease_pencil_get_lineart_modifier_limits(ctx->object);
BKE_grease_pencil_set_lineart_modifier_limits(md, &info, is_first_lineart);
There is no need for this loop. You can just put this above the `if` ``` GreasePencilLineartLimitInfo info = BKE_grease_pencil_get_lineart_modifier_limits(ctx->object); BKE_grease_pencil_set_lineart_modifier_limits(md, &info, is_first_lineart); ```
Author
Member

I'm not sure tho, this needs to set limit info for all line art modifiers.

I'm not sure tho, this needs to set limit info for all line art modifiers.
Member

Yes, that's what it will do. When each lineart modifier executes, it sets the info first.

Yes, that's what it will do. When each lineart modifier executes, it sets the info first.
Author
Member

Wouldn't it be kinda unnecessary since the idea is that it gets the limits once and set for all modifiers, and this way each modifier had to get the limit one more time 😅.

Wouldn't it be kinda unnecessary since the idea is that it gets the limits once and set for all modifiers, and this way each modifier had to get the limit one more time 😅.
Author
Member

I now put that in the cache, it should work fine now.

I now put that in the cache, it should work fine now.
ChengduLittleA marked this conversation as resolved
Falk David reviewed 2024-02-23 12:27:21 +01:00
@ -58,6 +58,10 @@
#include "MEM_guardedalloc.h"
/* For Line Art modifier cache calls. */
Member

These are no longer needed. in fact the whole file can just be reverted git checkout main source/blender/blenkernel/intern/grease_pencil.cc

These are no longer needed. in fact the whole file can just be reverted `git checkout main source/blender/blenkernel/intern/grease_pencil.cc `
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-23 12:33:45 +01:00
YimingWu added 1 commit 2024-02-23 12:36:49 +01:00
YimingWu added 1 commit 2024-02-23 12:38:18 +01:00
Iliya Katushenock reviewed 2024-02-23 13:16:18 +01:00
@ -874,0 +879,4 @@
if (md->type == eModifierType_GreasePencilLineart) {
const auto *lmd = reinterpret_cast<const GreasePencilLineartModifierData *>(md);
if (is_first || (lmd->flags & MOD_LINEART_USE_CACHE)) {
info.min_level = std::min(int(info.min_level), int(lmd->level_start));

std::min(int(info.min_level), int(lmd->level_start));
->
std::min<int>(info.min_level, lmd->level_start);

`std::min(int(info.min_level), int(lmd->level_start));` -> `std::min<int>(info.min_level, lmd->level_start);`
ChengduLittleA marked this conversation as resolved
@ -0,0 +398,4 @@
const int level_start = RNA_int_get(ptr, "level_start");
const int level_end = RNA_int_get(ptr, "level_end");
if (use_multiple_levels) {
return (std::max(level_start, level_end) > 0);

return (std::max(level_start, level_end) > 0);
->
return std::max(level_start, level_end) > 0;

`return (std::max(level_start, level_end) > 0);` -> `return std::max(level_start, level_end) > 0;`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-23 13:24:36 +01:00
YimingWu added 1 commit 2024-02-23 14:19:14 +01:00
YimingWu added 1 commit 2024-02-23 14:22:20 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
88871bc621
cleanup unused file
Member

@blender-bot build

@blender-bot build
Falk David requested changes 2024-02-23 16:56:09 +01:00
Dismissed
Falk David left a comment
Member

Some more cleanup. I think this is getting close.

Some more cleanup. I think this is getting close.
@ -56,3 +65,4 @@
#include <algorithm> /* For `min/max`. */
using blender::int3;
using namespace blender;
Member

Where is this needed?

Where is this needed?
Author
Member

The curve generation part, it has bke and stuff in blender.

I guess I'll modify that to not use blender namespace globally.

The curve generation part, it has `bke` and stuff in `blender`. I guess I'll modify that to not use blender namespace globally.
ChengduLittleA marked this conversation as resolved
@ -8184,0 +8270,4 @@
RNA_define_lib_overridable(true);
prop = RNA_def_property(srna, "use_custom_camera", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "calculation_flags", LRT_USE_CUSTOM_CAMERA);
Member

Shouldn't the flags here be also renamed? LINEART_USE_CUSTOM_CAMERA ?

Shouldn't the flags here be also renamed? `LINEART_USE_CUSTOM_CAMERA` ?
ChengduLittleA marked this conversation as resolved
@ -0,0 +85,4 @@
return false;
}
ModifierData *imd = md.modifier.prev;
while (imd) {
Member

imd != nullptr

`imd != nullptr`
ChengduLittleA marked this conversation as resolved
@ -0,0 +100,4 @@
return false;
}
ModifierData *imd = md.modifier.next;
while (imd) {
Member

imd != nullptr

`imd != nullptr`
ChengduLittleA marked this conversation as resolved
@ -0,0 +158,4 @@
const ModifierUpdateDepsgraphContext *ctx,
const int mode)
{
if (!c) {
Member

Seems like the argument should be Collection &collection and the caller should do this check.

Seems like the argument should be `Collection &collection` and the caller should do this check.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-24 04:50:14 +01:00
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
d72cd20aa0
Cleanups.
Author
Member

@blender-bot build

@blender-bot build
Falk David approved these changes 2024-02-26 11:38:44 +01:00
Dismissed
Falk David left a comment
Member

Only got 2 comments, but this doesn't need another review round from me. Accepting.

Only got 2 comments, but this doesn't need another review round from me. Accepting.
@ -1811,0 +1819,4 @@
{
GreasePencilLineartModifierData *lmd = (GreasePencilLineartModifierData *)ptr->data;
CLAMP(value, 0, 128);
Member

math::clamp (or std::clamp) should be used here

`math::clamp` (or `std::clamp`) should be used here
ChengduLittleA marked this conversation as resolved
@ -1811,0 +1828,4 @@
{
GreasePencilLineartModifierData *lmd = (GreasePencilLineartModifierData *)ptr->data;
CLAMP(value, 0, 128);
Member

Same as above

Same as above
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-26 11:50:19 +01:00
Falk David requested changes 2024-02-26 11:55:12 +01:00
Dismissed
Falk David left a comment
Member

Found a bug while testing.

Found a bug while testing.
@ -5486,0 +5854,4 @@
int point_i = i + up_to_point;
float *point = (float *)&point_positions[point_i];
copy_v3_v3(point, eci->gpos);
point_radii.span[point_i] = thickness;
Member

This is not correct. Since we're writing to the radii, this needs to be thickness / 2.0f.

This is not correct. Since we're writing to the radii, this needs to be `thickness / 2.0f`.
Author
Member

Huh so now it's actually treating it as 2r instead of r ? Interesting...

Huh so now it's actually treating it as `2r` instead of `r` ? Interesting...
Member

thickness was 2 * radius before. It's just that we're storing the radius instead of the diameter now.

`thickness` was `2 * radius` before. It's just that we're storing the radius instead of the diameter now.
ChengduLittleA marked this conversation as resolved
Iliya Katushenock reviewed 2024-02-26 11:56:05 +01:00
@ -940,0 +968,4 @@
const int8_t source_type,
Object *source_object,
struct Collection *source_collection,
const int level_start,

const int level_start -> int level_start
This and the same for all other by-value trivial small types in forward declaration.

`const int level_start` -> `int level_start` This and the same for all other by-value trivial small types in forward declaration.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-26 12:02:03 +01:00
YimingWu added 1 commit 2024-02-26 12:03:13 +01:00
Falk David approved these changes 2024-02-26 12:21:33 +01:00
Lukas Tönne approved these changes 2024-02-26 12:41:24 +01:00
Falk David requested review from Hans Goudey 2024-02-26 12:42:50 +01:00
YimingWu added 1 commit 2024-02-26 13:31:07 +01:00
Hans Goudey approved these changes 2024-02-26 14:51:39 +01:00
Falk David merged commit 3d1cdfe2ca into main 2024-02-26 15:28:25 +01:00
Falk David referenced this issue from a commit 2024-02-26 15:28:25 +01:00
Sign in to join this conversation.
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117028
No description provided.