[vlc-devel] [PATCH] Puzzle filter enhancement
Jean-Baptiste Kempf
jb at videolan.org
Thu Jan 17 12:13:18 CET 2013
On 17 Jan, vlcvboyer wrote :
> I've modified the original puzzle filter and added some additional features:
> - "puzzle style" piece shapes
> - pieces rotations and mirror
> - drag'n drop with mouse to manipulate pieces
> - a preview can be displayed in a corner
> - borders can be shown
> - auto solve (to make it easier...)
> - auto shuffle (...to increase difficulty)
Nice.
Well, wow is more accurate.
> Original puzzle filter is still available. You just have to disable
> "advanced mode" parameter.
Why ? I see no reason for this.
> AUTHORS | 1 +
> modules/gui/qt4/components/extended_panels.cpp | 10 +
> modules/gui/qt4/ui/video_effects.ui | 335 ++-
> modules/video_filter/puzzle.c | 3517
> ++++++++++++++++++++++--
Please split UI from puzzle.c modifications.
> mode change 100644 => 100755 AUTHORS
> mode change 100644 => 100755 modules/gui/qt4/components/extended_panels.cpp
> mode change 100644 => 100755 modules/gui/qt4/ui/video_effects.ui
> mode change 100644 => 100755 modules/video_filter/puzzle.c
Do not change the mode of the files, please.
> diff --git a/modules/video_filter/puzzle.c b/modules/video_filter/puzzle.c
> +#ifndef MAX
> +# define MAX(x,y) ((x>y) ? x : y)
> +#endif
> +#ifndef MIN
> +# define MIN(x,y) ((x<y) ? x : y)
> +#endif
No, use _MAX and _MIN
> +#define init_countdown(init_val) ( (MAX( 1, 30000 - init_val)/20) / 2 + (
> (unsigned) vlc_mrand48() ) % ( MAX( 1, ((30000 - init_val)/20) ) ) )
Static inline it.
> +struct filter_sys_t {
> + bool b_init;
> + bool b_bake_request;
> + bool b_shape_init;
> + param_t s_allocated;
> + param_t s_current_param;
> + param_t s_new_param;
No param_t* ?
> + bool b_change_param;
> +
> bool b_finished;
> + uint32_t i_done_count, i_tmp_done_count;
>
> - /* */
> - struct
> - {
> - atomic_flag b_uptodate;
> - atomic_bool b_blackslot;
> - atomic_uint i_cols;
> - atomic_uint i_rows;
> - } change;
> + bool b_shuffleRqst;
> + bool b_mouse_drag;
> + bool b_mouse_mvt;
> + int32_t i_mouse_drag_pce;
> + int32_t i_mouse_x, i_mouse_y;
> + int16_t i_pointed_pce;
> + int8_t i_mouse_action;
> +
> + uint32_t i_solve_acc_loop, i_solve_grp_loop, i_calc_corn_loop;
> + int32_t i_magnet_accuracy;
> + int32_t *pi_group_qty;
> +
> + int32_t *pi_order; //array which contains final pieces location (used
> in BASIC GAME MODE)
> + puzzle_array_t ***ps_puzzle_array; // array [row][col][plane] preset of
> location & size of each piece in the original image
> + piece_shape_t **ps_pieces_shapes; // array [each piece type
> (PCE_TYPE_NBR * negative * 4: top...)][each plane] of piece definition
> + piece_t *ps_pieces; // list [piece] of pieces data.
> + piece_t *ps_pieces_tmp; // used when sorting layers
> +
> + puzzle_plane_t *ps_desk_planes;
> + puzzle_plane_t *ps_pict_planes;
> +
> + uint8_t i_preview_pos;
> + int32_t i_selected;
> +
> + vlc_mutex_t lock, pce_lock;
> +
> + int32_t i_auto_shuffle_countdown_val, i_auto_solve_countdown_val;
> +
> + point_t **ps_bezier_pts_H;
> };
You should reorder for better repacking.
You should align comments a bit better.
> /**
> * Open the filter
> @@ -148,23 +392,76 @@ static int Open( vlc_object_t *p_this )
> config_ChainParse( p_filter, CFG_PREFIX, ppsz_filter_options,
> p_filter->p_cfg );
>
> + p_sys->b_init = false;
> + p_sys->b_shuffleRqst = true;
> + p_sys->b_change_param = true;
> + p_sys->b_shape_init = false;
> p_sys->pi_order = NULL;
> + p_sys->ps_desk_planes = NULL;
> + p_sys->ps_pict_planes = NULL;
> + p_sys->ps_puzzle_array = NULL;
> + p_sys->ps_pieces_shapes = NULL;
> + p_sys->ps_pieces = NULL;
> + p_sys->ps_pieces_tmp = NULL;
> + p_sys->pi_group_qty = NULL;
> + p_sys->b_mouse_drag = false;
> + p_sys->b_mouse_mvt = false;
> + p_sys->i_mouse_drag_pce = -1;
> + p_sys->i_pointed_pce = -1;
> + p_sys->i_magnet_accuracy = 3;
> + p_sys->i_mouse_x = 0;
> + p_sys->i_mouse_y = 0;
callocing would make the code shorter.
> + p_sys->s_new_param.i_rows =
> + var_CreateGetIntegerCommand( p_filter, CFG_PREFIX "rows" );
> + p_sys->s_new_param.i_cols =
> + var_CreateGetIntegerCommand( p_filter, CFG_PREFIX "cols" );
> + p_sys->s_new_param.b_blackslot =
> + var_CreateGetBoolCommand( p_filter, CFG_PREFIX "black-slot" );
> + p_sys->s_new_param.b_near =
> + var_CreateGetBoolCommand( p_filter, CFG_PREFIX "near" );
> + p_sys->s_new_param.i_border =
> + var_CreateGetIntegerCommand( p_filter, CFG_PREFIX "border" );
> + p_sys->s_new_param.b_preview =
> + var_CreateGetBoolCommand( p_filter, CFG_PREFIX "preview" );
> + p_sys->s_new_param.i_preview_size =
> + var_CreateGetIntegerCommand( p_filter, CFG_PREFIX "preview-size" );
> + p_sys->s_new_param.i_shape_size =
> + var_CreateGetIntegerCommand( p_filter, CFG_PREFIX "shape-size" );
> + p_sys->s_new_param.b_advanced =
> + var_CreateGetBoolCommand( p_filter, CFG_PREFIX "advanced-game" );
> + p_sys->s_new_param.i_auto_shuffle_speed =
> + var_CreateGetIntegerCommand( p_filter, CFG_PREFIX "auto-shuffle" );
> + p_sys->s_new_param.i_auto_solve_speed =
> + var_CreateGetIntegerCommand( p_filter, CFG_PREFIX "auto-solve" );
> + p_sys->s_new_param.i_rotate =
> + var_CreateGetIntegerCommand( p_filter, CFG_PREFIX "rotation" );
Why no Inherit?
> + p_sys->i_preview_pos = 0;// 0=top-left, top-right, bottom right,
> bottom-left
Is this comment at the right place?
> + p_sys->ps_bezier_pts_H = malloc( sizeof( point_t *) * SHAPES_QTY );
Check your malloc. You should check this on all your code.
> + if (p_sys->ps_desk_planes != NULL)
> + free(p_sys->ps_desk_planes);
> + if (p_sys->ps_pict_planes != NULL)
> + free(p_sys->ps_pict_planes);
> + if (p_sys->pi_order != NULL )
> + free( p_sys->pi_order );
free(NULL) is a no-op. You should check this on all your code.
> + p_sys->s_current_param.b_blackslot =
> p_sys->s_new_param.b_blackslot;
> + p_sys->s_current_param.b_near = p_sys->s_new_param.b_near
> || p_sys->s_new_param.b_blackslot;
> + p_sys->s_current_param.i_border = 0;
> + p_sys->s_current_param.b_preview = false;
> + p_sys->s_current_param.i_preview_size= 0;
> + p_sys->s_current_param.i_shape_size= 0;
> + p_sys->s_current_param.i_auto_shuffle_speed = 0;
> + p_sys->s_current_param.i_auto_solve_speed = 0;
> + p_sys->s_current_param.i_rotate = 0;
memset it?
> + int32_t i_group_ID = p_sys->ps_pieces[0].i_group_ID;
Why not a uint32_t btw?
> + if (p_sys->s_current_param.b_advanced) {
> + p_sys->s_allocated.i_piece_types = PIECE_TYPE_NBR;
> + } else {
> + p_sys->s_allocated.i_piece_types = 0;
> + }
Use ternary
> + p_sys->s_allocated.i_pieces_nbr = p_sys->s_allocated.i_rows *
> p_sys->s_allocated.i_cols;
> + p_sys->s_allocated.b_preview = p_sys->s_current_param.b_preview;
> + p_sys->s_allocated.i_preview_size =
> p_sys->s_current_param.i_preview_size;
> + p_sys->s_allocated.i_border = p_sys->s_current_param.i_border;
> + p_sys->s_allocated.b_blackslot = p_sys->s_current_param.b_blackslot;
> + p_sys->s_allocated.b_near = p_sys->s_current_param.b_near;
> + p_sys->s_allocated.i_shape_size = p_sys->s_current_param.i_shape_size;
> + p_sys->s_allocated.b_advanced = p_sys->s_current_param.b_advanced;
> + p_sys->s_allocated.i_auto_shuffle_speed =
> p_sys->s_current_param.i_auto_shuffle_speed;
> + p_sys->s_allocated.i_auto_solve_speed =
> p_sys->s_current_param.i_auto_solve_speed;
> + p_sys->s_allocated.i_rotate = p_sys->s_current_param.i_rotate;
Vertical aligning could help a bit the readability.
> + return 0;
VLC_SUCCESS maybe?
> + int32_t i;
> +
> + i = 0;
You can combine both lines :)
> +static void free_ps_pieces_shapes( filter_t *p_filter)
> +{
> + filter_sys_t *p_sys = p_filter->p_sys;
> +
> + if (p_sys->ps_pieces_shapes != NULL)
> + {
> + for (int32_t p = 0; p < p_sys->s_allocated.i_piece_types; p++)
> + {
> + for (uint8_t i_plane = 0; i_plane <
> p_sys->s_allocated.i_planes; i_plane++)
> + {
> + for (int32_t r = 0; r <
> p_sys->ps_pieces_shapes[p][i_plane].i_row_nbr; r++)
> + free(
> p_sys->ps_pieces_shapes[p][i_plane].ps_piece_shape_row[r].ps_row_section );
> + free(
> p_sys->ps_pieces_shapes[p][i_plane].ps_piece_shape_row );
> + }
> + free( p_sys->ps_pieces_shapes[p] );
> + }
> + free( p_sys->ps_pieces_shapes );
> + p_sys->ps_pieces_shapes = NULL;
> + }
> +
> + return;
> +}
On functions like this, you should invert your first if and return
directly there, reduce one level of indentation and remove the last
useless return.
> + memset ( p_sys->pi_group_qty, (int32_t) 0, sizeof( int32_t ) *
> (p_sys->s_allocated.i_pieces_nbr) );
Are you sure the cast is necessary?
> + {
> + if
> (p_sys->i_mouse_y == i_current_dst_y )
Please reduce indentation levels...
13 is a bit too much...
> + f_bez_x = ( 1 - f_sub_t ) * ( 1 - f_sub_t ) * ( 1 - f_sub_t ) *
> ps_pt[ 3 * i_main_t ].f_x
> + + 3 * f_sub_t * ( 1 - f_sub_t ) * ( 1 - f_sub_t ) *
> ps_pt[ 3 * i_main_t + 1 ].f_x
> + + 3 * f_sub_t * f_sub_t * ( 1 - f_sub_t ) * ps_pt[ 3 *
> i_main_t + 2 ].f_x
> + + f_sub_t * f_sub_t * f_sub_t * ps_pt[ 3 * i_main_t + 3
> ].f_x;
Is it possible to macro those?
> + ofp = fopen(sav_filename, "w");
No. Use VLC functions.
In general, you should increase a bit the readability of your code and
check for common errors.
Best regards,
--
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device
More information about the vlc-devel
mailing list