Initial Grease Pencil 3.0 stage #106848

Merged
Falk David merged 224 commits from filedescriptor/blender:grease-pencil-v3 into main 2023-05-30 11:14:22 +02:00
6 changed files with 99 additions and 109 deletions
Showing only changes of commit fc78ab38ba - Show all commits

View File

@ -77,51 +77,22 @@ class AntiAliasing {
const float *sizeinv = DRW_viewport_invert_size_get();
const float4 metrics = {sizeinv[0], sizeinv[1], size[0], size[1]};
if (anti_aliasing_enabled_) {
/* Stage 1: Edge detection. */
PassSimple &pass = edge_detect_ps_;
pass.init();
pass.framebuffer_set(&edge_detect_fb_);
pass.state_set(DRW_STATE_WRITE_COLOR);
pass.shader_set(shaders_.static_shader_get(ANTIALIASING_EDGE_DETECT));
pass.bind_texture("colorTex", &color_tx);
pass.bind_texture("revealTex", &reveal_tx);
pass.push_constant("viewportMetrics", metrics);
pass.push_constant("lumaWeight", luma_weight_);
pass.clear_color(float4(0.0f));
pass.draw_procedural(GPU_PRIM_TRIS, 1, 3);
}
if (anti_aliasing_enabled_) {
/* Stage 2: Blend Weight/Coord. */
PassSimple &pass = blend_weight_ps_;
pass.init();
anti_aliasing_pass(color_tx, reveal_tx, metrics);

I would totally split the code into smaller functions. I do not see what are we winning from such inlined code.

I would totally split the code into smaller functions. I do not see what are we winning from such inlined code.
pass.framebuffer_set(&blend_weight_fb_);
pass.state_set(DRW_STATE_WRITE_COLOR);
pass.shader_set(shaders_.static_shader_get(ANTIALIASING_BLEND_WEIGHT));
pass.bind_texture("edgesTex", &edge_detect_tx_);
pass.bind_texture("areaTex", smaa_area_tx_);
pass.bind_texture("searchTex", smaa_search_tx_);
pass.push_constant("viewportMetrics", metrics);
pass.clear_color(float4(0.0f));
pass.draw_procedural(GPU_PRIM_TRIS, 1, 3);
}
{
/* Stage 3: Resolve. */
PassSimple &pass = resolve_ps_;
pass.init();
pass.framebuffer_set(&output_fb_);
pass.state_set(DRW_STATE_WRITE_COLOR | DRW_STATE_BLEND_CUSTOM);
pass.shader_set(shaders_.static_shader_get(ANTIALIASING_RESOLVE));
/** \note use color_tx as dummy if AA is diabled. */
pass.bind_texture("blendTex", anti_aliasing_enabled_ ? &blend_weight_tx_ : &color_tx);
pass.bind_texture("colorTex", &color_tx);
pass.bind_texture("revealTex", &reveal_tx);
pass.push_constant("doAntiAliasing", anti_aliasing_enabled_);
pass.push_constant("onlyAlpha", draw_wireframe_);
pass.push_constant("viewportMetrics", metrics);
pass.draw_procedural(GPU_PRIM_TRIS, 1, 3);
}
/* Resolve pass. */
PassSimple &pass = resolve_ps_;
pass.init();
pass.framebuffer_set(&output_fb_);
pass.state_set(DRW_STATE_WRITE_COLOR | DRW_STATE_BLEND_CUSTOM);
pass.shader_set(shaders_.static_shader_get(ANTIALIASING_RESOLVE));
/** \note use color_tx as dummy if AA is diabled. */
pass.bind_texture("blendTex", anti_aliasing_enabled_ ? &blend_weight_tx_ : &color_tx);
pass.bind_texture("colorTex", &color_tx);
pass.bind_texture("revealTex", &reveal_tx);
pass.push_constant("doAntiAliasing", anti_aliasing_enabled_);
pass.push_constant("onlyAlpha", draw_wireframe_);
pass.push_constant("viewportMetrics", metrics);
pass.draw_procedural(GPU_PRIM_TRIS, 1, 3);
}
void draw(Manager &manager, GPUTexture *dst_color_tx)
@ -134,9 +105,7 @@ class AntiAliasing {
edge_detect_tx_.acquire(render_size, GPU_RG8);
edge_detect_fb_.ensure(GPU_ATTACHMENT_NONE, GPU_ATTACHMENT_TEXTURE(edge_detect_tx_));
manager.submit(edge_detect_ps_);
}
if (anti_aliasing_enabled_) {
blend_weight_tx_.acquire(render_size, GPU_RGBA8);
blend_weight_fb_.ensure(GPU_ATTACHMENT_NONE, GPU_ATTACHMENT_TEXTURE(blend_weight_tx_));
manager.submit(blend_weight_ps_);
@ -149,6 +118,40 @@ class AntiAliasing {
DRW_stats_group_end();
}
private:
void anti_aliasing_pass(TextureFromPool &color_tx,
TextureFromPool &reveal_tx,
const float4 metrics)
{
if (!anti_aliasing_enabled_) {
return;
}
/* Stage 1: Edge detection. */
edge_detect_ps_.init();
edge_detect_ps_.framebuffer_set(&edge_detect_fb_);
edge_detect_ps_.state_set(DRW_STATE_WRITE_COLOR);
edge_detect_ps_.shader_set(shaders_.static_shader_get(ANTIALIASING_EDGE_DETECT));
edge_detect_ps_.bind_texture("colorTex", &color_tx);
edge_detect_ps_.bind_texture("revealTex", &reveal_tx);
edge_detect_ps_.push_constant("viewportMetrics", metrics);
edge_detect_ps_.push_constant("lumaWeight", luma_weight_);

Why this extra check is needed?

Why this extra check is needed?
edge_detect_ps_.clear_color(float4(0.0f));
edge_detect_ps_.draw_procedural(GPU_PRIM_TRIS, 1, 3);
/* Stage 2: Blend Weight/Coord. */
blend_weight_ps_.init();
blend_weight_ps_.framebuffer_set(&blend_weight_fb_);
blend_weight_ps_.state_set(DRW_STATE_WRITE_COLOR);
blend_weight_ps_.shader_set(shaders_.static_shader_get(ANTIALIASING_BLEND_WEIGHT));
blend_weight_ps_.bind_texture("edgesTex", &edge_detect_tx_);
blend_weight_ps_.bind_texture("areaTex", smaa_area_tx_);
blend_weight_ps_.bind_texture("searchTex", smaa_search_tx_);
blend_weight_ps_.push_constant("viewportMetrics", metrics);
blend_weight_ps_.clear_color(float4(0.0f));
blend_weight_ps_.draw_procedural(GPU_PRIM_TRIS, 1, 3);
}
};
} // namespace blender::greasepencil

View File

@ -116,7 +116,7 @@ class ObjectModule {
const bool is_stroke_order_3d = false; /* TODO */
bool do_material_holdout = false;
bool do_layer_blending = false;
bool object_has_vfx = false; // TODO vfx.object_has_vfx(gpd);
bool object_has_vfx = false; // TODO: vfx.object_has_vfx(gpd);
uint material_offset = materials_.object_offset_get();
for (auto i : IndexRange(BKE_object_material_count_eval(object))) {
@ -152,8 +152,6 @@ class ObjectModule {
GPUVertBuf *color_tx = DRW_cache_grease_pencil_color_buffer_get(object, current_frame_);
GPUBatch *geom = DRW_cache_grease_pencil_get(object, current_frame_);
// grease_pencil.foreach_visible_drawing(current_frame_, [&](GreasePencilDrawing & /*drawing*/)
// {
/* TODO(fclem): Pass per frame object matrix here. */
ResourceHandle handle = manager.resource_handle(object_ref);
gpObject &ob = objects_buf_.get_or_resize(handle.resource_index());
@ -164,18 +162,20 @@ class ObjectModule {
ob.material_offset = material_offset;
if (do_layer_blending) {
/* TODO: Do layer blending. */
// for (const LayerData &layer : frame.layers) {
// UNUSED_VARS(layer);
// if (has_blending(layer)) {
// object_subpass.framebuffer_set(*vfx_fb.current());
// }
// UNUSED_VARS(layer);

Typically dead code is discouraged.
Not sure what is it part of, so can't really give strong suggestions.

Typically dead code is discouraged. Not sure what is it part of, so can't really give strong suggestions.
// if (has_blending(layer)) {
// object_subpass.framebuffer_set(*vfx_fb.current());
// }
/* TODO(fclem): Only draw subrange of geometry for this layer. */
object_subpass.draw(geom, handle);
// if (has_blending(layer)) {
// layer_blend_sync(object_ref, object_subpass);
// }
/* TODO: Do layer blending. */
// if (has_blending(layer)) {
// layer_blend_sync(object_ref, object_subpass);
// }
// }
}
else {
@ -184,8 +184,8 @@ class ObjectModule {
object_subpass.bind_texture("gp_col_tx", color_tx);
object_subpass.draw(geom, handle);
}
// });
/* TODO: Do object VFX. */
#if 0
Review

Add a comment what is this about, or remove.

Add a comment what is this about, or remove.
if (object_has_vfx) {
VfxContext vfx_ctx(object_subpass,
@ -246,29 +246,6 @@ class ObjectModule {
{
return objects_buf_.size() > 0;
}
private:
// static float4 frame_tint_get(const bGPdata *gpd, const bGPDframe *gpf, int /* current_frame
// */)
// {
// /* TODO(fclem): Onion color should rely on time and or frame id and not on runtime.onion_id.
// * This should be evaluated at draw time as it is just a way of displaying the data. */
// const bool use_onion_custom_col = (gpd->onion_flag & GP_ONION_GHOST_PREVCOL) != 0;
// const bool use_onion_fade = (gpd->onion_flag & GP_ONION_FADE) != 0;
// const bool use_next_col = gpf->runtime.onion_id > 0.0f;
// const float *onion_col_custom = (use_onion_custom_col) ?
// (use_next_col ? gpd->gcolor_next : gpd->gcolor_prev) :
// U.gpencil_new_layer_col;
// float4 tint = {UNPACK3(onion_col_custom), 1.0f};
// tint[3] = use_onion_fade ? (1.0f / abs(gpf->runtime.onion_id)) : 0.5f;
// tint[3] *= gpd->onion_factor;
// tint[3] = (gpd->onion_factor > 0.0f) ? clamp_f(tint[3], 0.1f, 1.0f) :
// clamp_f(tint[3], 0.01f, 1.0f);
// return tint;
// }
};
} // namespace blender::greasepencil

Same as above.

Same as above.

View File

@ -41,7 +41,7 @@ ShaderModule::ShaderModule()
const char *name = static_shader_create_info_name_get(eShaderType(i));
if (name == nullptr) {
std::cerr << "GPencil: Missing case for eShaderType(" << i
<< ") in static_shader_create_info_name_get().";
<< ") in static_shader_create_info_name_get()." << std::endl;

<< std::endl to put new line and flush the output buffer.

Also, why not to assert the details in the static_shader_create_info_name_get() ? It is a bit weird to print an error about mistake in some other code.

P.S. I think we should implement Glog style of checkers: DCHECK_NE(name, nullptr) << "GPencil: Missing case for eShaderType(" << i << ") in static_shader_create_info_name_get()."

`<< std::endl` to put new line and flush the output buffer. Also, why not to assert the details in the `static_shader_create_info_name_get()` ? It is a bit weird to print an error about mistake in some other code. P.S. I think we should implement Glog style of checkers: `DCHECK_NE(name, nullptr) << "GPencil: Missing case for eShaderType(" << i << ") in static_shader_create_info_name_get()."`
BLI_assert(0);
}
const GPUShaderCreateInfo *create_info = GPU_shader_create_info_get(name);
@ -105,7 +105,8 @@ GPUShader *ShaderModule::static_shader_get(eShaderType shader_type)
shaders_[shader_type] = GPU_shader_create_from_info_name(shader_name);
if (shaders_[shader_type] == nullptr) {
fprintf(stderr, "GPencil: error: Could not compile static shader \"%s\"\n", shader_name);
std::cerr << "GPencil: error: Could not compile static shader \"" << shader_name << "\""

Pick a side between fprintf and std::cerr ;)

Pick a side between `fprintf` and `std::cerr` ;)
<< std::endl;
}
BLI_assert(shaders_[shader_type] != nullptr);
}

View File

@ -184,17 +184,14 @@ static void grease_pencil_draw_mode_enter(bContext *C)
wmMsgBus *mbus = CTX_wm_message_bus(C);
Object *ob = CTX_data_active_object(C);
BKE_paint_ensure(scene->toolsettings, (Paint **)&scene->toolsettings->gp_paint);
// GpPaint *grease_pencil_paint = scene->toolsettings->gp_paint;
GpPaint *grease_pencil_paint = scene->toolsettings->gp_paint;
BKE_paint_ensure(scene->toolsettings, (Paint **)&grease_pencil_paint);
ob->mode = OB_MODE_PAINT_GPENCIL;
/* Setup cursor color. BKE_paint_init() could be used, but creates an additional brush. */
// Paint *paint = BKE_paint_get_active_from_paintmode(scene, PAINT_MODE_GPENCIL);
// copy_v3_v3_uchar(paint->paint_cursor_col, PAINT_CURSOR_GREASE_PENCIL);
// paint->paint_cursor_col[3] = 128;
// ED_paint_cursor_start(&grease_pencil_paint->paint, GREASE_PENCIL_mode_poll_view3d);
/* TODO: Setup cursor color. BKE_paint_init() could be used, but creates an additional brush. */
/* TODO: Call ED_paint_cursor_start(...) */

What's this commented out code is about?

What's this commented out code is about?
paint_init_pivot(ob, scene);
/* Necessary to change the object mode on the evaluated object. */

View File

@ -46,6 +46,11 @@ struct PaintOperationExecutor {
Object *obact = CTX_data_active_object(&C);
Object *ob_eval = DEG_get_evaluated_object(depsgraph, obact);
Review

It seems a bit weird to modifier the evaluated object here. Is that purposeful? Or am I missing something?

It seems a bit weird to modifier the evaluated object here. Is that purposeful? Or am I missing something?
Review

Yes, this is done on purpose so that the extra copy from orig to eval for rendering is not needed every update. Once drawing is done, the stroke is copied from the eval buffer to orig.

Yes, this is done on purpose so that the extra copy from orig to eval for rendering is not needed every update. Once drawing is done, the stroke is copied from the eval buffer to orig.
Review

Okay, makes sense. This definitely deserves a comment, since typically changing the evaluated data isn't a good idea.

Okay, makes sense. This definitely deserves a comment, since typically changing the evaluated data isn't a good idea.
/**
* Note: We write to the evaluated object here, so that the additional copy from orig -> eval
* is not needed for every update. After the stroke is done, the result is written to the
* original object.
*/
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(ob_eval->data);
if (!grease_pencil.runtime->has_active_layer()) {
/* TODO: create a new layer. */

View File

@ -180,7 +180,6 @@ typedef struct GreasePencilLayerMask {
struct GreasePencilLayerMask *next, *prev;
/**
* The name of the layer that is the mask.
* \note Null-terminated.
*/

Pretty sure null-terminated goes without saying for char * pointers in DNA, I don't think it's worth mentioning here

Pretty sure null-terminated goes without saying for `char *` pointers in DNA, I don't think it's worth mentioning here

Not sure about dynamic names for strings. It is not something typically used in the DNA.

Not sure about dynamic names for strings. It is not something typically used in the DNA.

It's been done more recently I think. It's properly integrated with RNA now too, to avoid that boilerplate. It's nice not to have to worry about choosing a future-proof length.

It's been done more recently I think. It's properly integrated with RNA now too, to avoid that boilerplate. It's nice not to have to worry about choosing a future-proof length.
char *layer_name;
/**
@ -235,13 +234,11 @@ typedef struct GreasePencilLayer {
struct Object *parent;
/**
* Parent sub-object info. E.g. the name of a bone if the parent is an armature.
* \note Null-terminated.
*/
char *parsubstr;
/**
* Name of a view layer. If used, the layer is only rendered on the specified view layer. Not
* used for viewport rendering.
* \note Null-terminated.
*/
char *viewlayer_name;
/**
@ -413,20 +410,10 @@ typedef struct GreasePencil {
GreasePencilDrawingBase **drawing_array;
int drawing_array_size;
char _pad[4];
#ifdef __cplusplus
void read_drawing_array(BlendDataReader *reader);
void write_drawing_array(BlendWriter *writer);
void free_drawing_array();
#endif
/* Only used for storage in the .blend file. */
GreasePencilLayerTreeStorage layer_tree_storage;
#ifdef __cplusplus
void save_layer_tree_to_storage();
void load_layer_tree_from_storage();
void read_layer_tree_storage(BlendDataReader *reader);
void write_layer_tree_storage(BlendWriter *writer);
void free_layer_tree_storage();
#endif
/**

Best to follow the style guidelines and put all methods at the bottom.
I think that's best since you can easily see what's actually stored in the class without digging through a bunch of methods.

Best to follow the style guidelines and put all methods at the bottom. I think that's best since you can easily see what's actually stored in the class without digging through a bunch of methods.
* An array of materials.
*/
@ -446,17 +433,37 @@ typedef struct GreasePencil {
*/
GreasePencilRuntimeHandle *runtime;
#ifdef __cplusplus
/* GreasePencilDrawingBase array functions. */
void read_drawing_array(BlendDataReader *reader);
void write_drawing_array(BlendWriter *writer);
void free_drawing_array();
/* GreasePencilLayerTreeStorage functions. */
void save_layer_tree_to_storage();
void load_layer_tree_from_storage();
void read_layer_tree_storage(BlendDataReader *reader);
void write_layer_tree_storage(BlendWriter *writer);
void free_layer_tree_storage();
/* Drawings read/write access. */
blender::Span<GreasePencilDrawingBase *> drawings() const;

It probably shouldn't be possible to get a span of non-const pointers from a const GreasePencil:

Span<const GreasePencilDrawingBase *>

It probably shouldn't be possible to get a span of non-const pointers from a const `GreasePencil`: `Span<const GreasePencilDrawingBase *>`
blender::MutableSpan<GreasePencilDrawingBase *> drawings_for_write();
void add_empty_drawings(int add_size);
void remove_drawing(int index);
void foreach_visible_drawing(int frame,
blender::FunctionRef<void(GreasePencilDrawing &)> function);
/* Root group read/write access. */

This sort of API function (dealing with one index at a time) can be a bit dangerous since it can easily lead to quadratic behavior if it's called many times. I wonder if a remove_drawings(IndexMask drawings_to_remove) function would work a bit better

This sort of API function (dealing with one index at a time) can be a bit dangerous since it can easily lead to quadratic behavior if it's called many times. I wonder if a `remove_drawings(IndexMask drawings_to_remove)` function would work a bit better
const blender::bke::greasepencil::LayerGroup &root_group() const;
blender::bke::greasepencil::LayerGroup &root_group_for_write();
/* Layers read/write access. */
blender::Span<const blender::bke::greasepencil::Layer *> layers() const;
blender::Span<blender::bke::greasepencil::Layer *> layers_for_write();
void add_empty_drawings(int add_size);
void remove_drawing(int index);
void foreach_visible_drawing(int frame,
blender::FunctionRef<void(GreasePencilDrawing &)> function);
/* For debugging purposes. */
void print_layer_tree();
#endif
} GreasePencil;