From d9829c2f870ba2f3fe2154253d01cbcbbfad4724 Mon Sep 17 00:00:00 2001 From: Germano Cavalcante Date: Thu, 6 Jul 2023 18:21:36 +0200 Subject: [PATCH] View 3D: Refactor code to split functions and remove duplication No functional changes. Previously, each navigation operation had its own method for restoring the 3D View's state upon cancellation. However, this approach did not provide significant advantages. The number of affected variables was too small to justify considering it for performance optimization, and the cancellation code was duplicated across different operations. To address this, the present commit unifies all restore code into a single function: - `void viewops_data_state_restore(ViewOpsData *vod);` As a result, the `viewmove_apply_reset` function has been removed. Moreover, since the state now has a dedicated restore function, it seemed appropriate to create a function for capturing the state as well. Therefore, the code responsible for capturing the state (previously scattered within `viewops_data_init_navigation`) has been extracted to create the function: - `void viewops_data_state_capture(ViewOpsData *vod);` During this separation process, a potential error in the code was identified, where the state may not be fully restored. Although this issue persists, a comment has been added to clarify this situation. Pull Request: https://projects.blender.org/blender/blender/pulls/109760 --- .../editors/space_view3d/view3d_navigate.cc | 145 +++++++++++++----- .../editors/space_view3d/view3d_navigate.h | 45 +++--- .../space_view3d/view3d_navigate_dolly.c | 4 +- .../space_view3d/view3d_navigate_move.c | 3 +- .../space_view3d/view3d_navigate_roll.c | 4 +- .../space_view3d/view3d_navigate_rotate.c | 16 +- .../space_view3d/view3d_navigate_zoom.c | 11 +- 7 files changed, 134 insertions(+), 94 deletions(-) diff --git a/source/blender/editors/space_view3d/view3d_navigate.cc b/source/blender/editors/space_view3d/view3d_navigate.cc index e50c955a83d..1bcdd2f3f82 100644 --- a/source/blender/editors/space_view3d/view3d_navigate.cc +++ b/source/blender/editors/space_view3d/view3d_navigate.cc @@ -530,6 +530,99 @@ static void viewops_data_init_context(bContext *C, ViewOpsData *vod) vod->rv3d = static_cast(vod->region->regiondata); } +static void viewops_data_state_capture(ViewOpsData *vod) +{ + Depsgraph *depsgraph = vod->depsgraph; + View3D *v3d = vod->v3d; + RegionView3D *rv3d = vod->rv3d; + + /* Set the view from the camera, if view locking is enabled. + * we may want to make this optional but for now its needed always. + * NOTE: This changes the value of `rv3d->dist`. */ + ED_view3d_camera_lock_init(depsgraph, v3d, rv3d); + + copy_v3_v3(vod->init.ofs, rv3d->ofs); + copy_v3_v3(vod->init.ofs_lock, rv3d->ofs_lock); + vod->init.camdx = rv3d->camdx; + vod->init.camdy = rv3d->camdy; + vod->init.camzoom = rv3d->camzoom; + vod->init.dist = rv3d->dist; + copy_qt_qt(vod->init.quat, rv3d->viewquat); + + vod->init.persp = rv3d->persp; + vod->init.view = rv3d->view; + vod->init.view_axis_roll = rv3d->view_axis_roll; +} + +void viewops_data_state_restore(ViewOpsData *vod) +{ + /* DOLLY, MOVE, ROTATE and ZOOM. */ + { + /* For Move this only changes when offset is not locked. */ + /* For Rotate this only changes when rotating around objects or last-brush. */ + /* For Zoom this only changes when zooming to mouse position. */ + /* Note this does not remove auto-keys on locked cameras. */ + copy_v3_v3(vod->rv3d->ofs, vod->init.ofs); + } + + /* MOVE and ZOOM. */ + { + /* For Move this only changes when offset is not locked. */ + /* For Zoom this only changes when zooming to mouse position in camera view. */ + vod->rv3d->camdx = vod->init.camdx; + vod->rv3d->camdy = vod->init.camdy; + } + + /* MOVE. */ + { + if ((vod->rv3d->persp == RV3D_CAMOB) && !ED_view3d_camera_lock_check(vod->v3d, vod->rv3d)) { + // vod->rv3d->camdx = vod->init.camdx; + // vod->rv3d->camdy = vod->init.camdy; + } + else if (ED_view3d_offset_lock_check(vod->v3d, vod->rv3d)) { + copy_v2_v2(vod->rv3d->ofs_lock, vod->init.ofs_lock); + } + else { + // copy_v3_v3(vod->rv3d->ofs, vod->init.ofs); + if (RV3D_LOCK_FLAGS(vod->rv3d) & RV3D_BOXVIEW) { + view3d_boxview_sync(vod->area, vod->region); + } + } + } + + /* ZOOM. */ + { + vod->rv3d->camzoom = vod->init.camzoom; + } + + /* ROTATE and ZOOM. */ + { + /** + * For Rotate this only changes when orbiting from a camera view. + * In this case the `dist` is calculated based on the camera relative to the `ofs`. + */ + /* Note this does not remove auto-keys on locked cameras. */ + vod->rv3d->dist = vod->init.dist; + } + + /* ROLL and ROTATE. */ + { + /* Note this does not remove auto-keys on locked cameras. */ + copy_qt_qt(vod->rv3d->viewquat, vod->init.quat); + } + + /* ROTATE. */ + { + vod->rv3d->persp = vod->init.persp; + vod->rv3d->view = vod->init.view; + vod->rv3d->view_axis_roll = vod->init.view_axis_roll; + } + + /* NOTE: there is no need to restore "last" values (as set by #ED_view3d_lastview_store). */ + + ED_view3d_camera_lock_sync(vod->depsgraph, vod->v3d, vod->rv3d); +} + static void viewops_data_init_navigation(bContext *C, const wmEvent *event, const eV3D_OpMode nav_type, @@ -537,6 +630,8 @@ static void viewops_data_init_navigation(bContext *C, ViewOpsData *vod) { Depsgraph *depsgraph = vod->depsgraph; + ARegion *region = vod->region; + View3D *v3d = vod->v3d; RegionView3D *rv3d = vod->rv3d; eViewOpsFlag viewops_flag = viewops_flag_from_prefs(); @@ -597,7 +692,8 @@ static void viewops_data_init_navigation(bContext *C, vod->use_dyn_ofs = false; } - vod->init.persp = rv3d->persp; + + viewops_data_state_capture(vod); if (viewops_flag & VIEWOPS_FLAG_PERSP_ENSURE) { if (ED_view3d_persp_ensure(depsgraph, vod->v3d, vod->region)) { @@ -607,16 +703,7 @@ static void viewops_data_init_navigation(bContext *C, } } - /* set the view from the camera, if view locking is enabled. - * we may want to make this optional but for now its needed always */ - ED_view3d_camera_lock_init(depsgraph, vod->v3d, vod->rv3d); - vod->init.persp_with_auto_persp_applied = rv3d->persp; - vod->init.view = rv3d->view; - vod->init.view_axis_roll = rv3d->view_axis_roll; - vod->init.dist = rv3d->dist; - vod->init.camzoom = rv3d->camzoom; - copy_qt_qt(vod->init.quat, rv3d->viewquat); copy_v2_v2_int(vod->init.event_xy, event->xy); copy_v2_v2_int(vod->prev.event_xy, event->xy); @@ -630,14 +717,8 @@ static void viewops_data_init_navigation(bContext *C, } vod->init.event_type = event->type; - copy_v3_v3(vod->init.ofs, rv3d->ofs); - copy_qt_qt(vod->curr.viewquat, rv3d->viewquat); - copy_v3_v3(vod->init.ofs_lock, rv3d->ofs_lock); - vod->init.camdx = rv3d->camdx; - vod->init.camdy = rv3d->camdy; - if (viewops_flag & VIEWOPS_FLAG_ORBIT_SELECT) { float ofs[3]; if (view3d_orbit_calc_center(C, ofs) || (vod->use_dyn_ofs == false)) { @@ -675,32 +756,35 @@ static void viewops_data_init_navigation(bContext *C, /* find a new ofs value that is along the view axis * (rather than the mouse location) */ closest_to_line_v3(dvec, vod->dyn_ofs, my_pivot, my_origin); - vod->init.dist = rv3d->dist = len_v3v3(my_pivot, dvec); + rv3d->dist = len_v3v3(my_pivot, dvec); negate_v3_v3(rv3d->ofs, dvec); } else { - const float mval_region_mid[2] = {float(vod->region->winx) / 2.0f, - float(vod->region->winy) / 2.0f}; + const float mval_region_mid[2] = {float(region->winx) / 2.0f, float(region->winy) / 2.0f}; - ED_view3d_win_to_3d(vod->v3d, vod->region, vod->dyn_ofs, mval_region_mid, rv3d->ofs); + ED_view3d_win_to_3d(v3d, region, vod->dyn_ofs, mval_region_mid, rv3d->ofs); negate_v3(rv3d->ofs); } negate_v3(vod->dyn_ofs); + + /* XXX: The initial state captured by `viewops_data_state_capture` is being modified here. + * This causes the state when canceling a navigation operation to not be fully restored. */ + vod->init.dist = rv3d->dist; copy_v3_v3(vod->init.ofs, rv3d->ofs); } } /* For dolly */ const float mval[2] = {float(event->mval[0]), float(event->mval[1])}; - ED_view3d_win_to_vector(vod->region, mval, vod->init.mousevec); + ED_view3d_win_to_vector(region, mval, vod->init.mousevec); { int event_xy_offset[2]; add_v2_v2v2_int(event_xy_offset, event->xy, vod->init.event_xy_offset); /* For rotation with trackball rotation. */ - calctrackballvec(&vod->region->winrct, event_xy_offset, vod->init.trackvec); + calctrackballvec(®ion->winrct, event_xy_offset, vod->init.trackvec); } { @@ -904,23 +988,6 @@ void viewmove_apply(ViewOpsData *vod, int x, int y) ED_region_tag_redraw(vod->region); } -void viewmove_apply_reset(ViewOpsData *vod) -{ - if ((vod->rv3d->persp == RV3D_CAMOB) && !ED_view3d_camera_lock_check(vod->v3d, vod->rv3d)) { - vod->rv3d->camdx = vod->init.camdx; - vod->rv3d->camdy = vod->init.camdy; - } - else if (ED_view3d_offset_lock_check(vod->v3d, vod->rv3d)) { - copy_v2_v2(vod->rv3d->ofs_lock, vod->init.ofs_lock); - } - else { - copy_v3_v3(vod->rv3d->ofs, vod->init.ofs); - if (RV3D_LOCK_FLAGS(vod->rv3d) & RV3D_BOXVIEW) { - view3d_boxview_sync(vod->area, vod->region); - } - } -} - /** \} */ /* -------------------------------------------------------------------- */ diff --git a/source/blender/editors/space_view3d/view3d_navigate.h b/source/blender/editors/space_view3d/view3d_navigate.h index 4dc0b2ee777..4c5383a1e4d 100644 --- a/source/blender/editors/space_view3d/view3d_navigate.h +++ b/source/blender/editors/space_view3d/view3d_navigate.h @@ -113,9 +113,27 @@ typedef struct ViewOpsData { /** Viewport state on initialization, don't change afterwards. */ struct { - float dist; - float camzoom; - float quat[4]; + + /** These variables reflect the same in #RegionView3D. */ + + float ofs[3]; /* DOLLY, MOVE, ROTATE and ZOOM. */ + float ofs_lock[2]; /* MOVE. */ + float camdx, camdy; /* MOVE and ZOOM. */ + float camzoom; /* ZOOM. */ + float dist; /* ROTATE and ZOOM. */ + float quat[4]; /* ROLL and ROTATE. */ + char persp; /* ROTATE. */ + char view; /* ROTATE. */ + char view_axis_roll; /* ROTATE. */ + + /** + * #RegionView3D.persp set after auto-perspective is applied. + * If we want the value before running the operator, add a separate member. + */ + char persp_with_auto_persp_applied; + + /** The ones below are unrelated to the state of the 3D view. */ + /** #wmEvent.xy. */ int event_xy[2]; /** Offset to use when #VIEWOPS_FLAG_USE_MOUSE_INIT is not set. @@ -123,32 +141,15 @@ typedef struct ViewOpsData { int event_xy_offset[2]; /** #wmEvent.type that triggered the operator. */ int event_type; - float ofs[3]; - /** #RegionView3D.ofs_lock */ - float ofs_lock[2]; + /** Initial distance to 'ofs'. */ float zfac; - /** Camera offset. */ - float camdx, camdy; - /** Trackball rotation only. */ float trackvec[3]; /** Dolly only. */ float mousevec[3]; - /** - * #RegionView3D.persp set after auto-perspective is applied. - * If we want the value before running the operator, add a separate member. - */ - char persp_with_auto_persp_applied; - /** #RegionView3D.persp set after before auto-perspective is applied. */ - char persp; - /** #RegionView3D.view */ - char view; - /** #RegionView3D.view_axis_roll */ - char view_axis_roll; - /** Used for roll */ struct Dial *dial; } init; @@ -212,7 +213,6 @@ void view3d_navigate_cancel_fn(struct bContext *C, struct wmOperator *op); void calctrackballvec(const struct rcti *rect, const int event_xy[2], float r_dir[3]); void viewmove_apply(ViewOpsData *vod, int x, int y); -void viewmove_apply_reset(ViewOpsData *vod); void view3d_orbit_apply_dyn_ofs(float r_ofs[3], const float ofs_old[3], const float viewquat_old[4], @@ -235,6 +235,7 @@ ViewOpsData *viewops_data_create(bContext *C, const wmEvent *event, const eV3D_OpMode nav_type, const bool use_cursor_init); +void viewops_data_state_restore(ViewOpsData *vod); void VIEW3D_OT_view_all(struct wmOperatorType *ot); void VIEW3D_OT_view_selected(struct wmOperatorType *ot); diff --git a/source/blender/editors/space_view3d/view3d_navigate_dolly.c b/source/blender/editors/space_view3d/view3d_navigate_dolly.c index 7b4ed2bf755..28853d408fc 100644 --- a/source/blender/editors/space_view3d/view3d_navigate_dolly.c +++ b/source/blender/editors/space_view3d/view3d_navigate_dolly.c @@ -163,9 +163,7 @@ static int viewdolly_modal(bContext *C, wmOperator *op, const wmEvent *event) break; } case VIEW_CANCEL: { - /* Note this does not remove auto-keys on locked cameras. */ - copy_v3_v3(vod->rv3d->ofs, vod->init.ofs); - ED_view3d_camera_lock_sync(vod->depsgraph, vod->v3d, vod->rv3d); + viewops_data_state_restore(vod); ret = OPERATOR_CANCELLED; break; } diff --git a/source/blender/editors/space_view3d/view3d_navigate_move.c b/source/blender/editors/space_view3d/view3d_navigate_move.c index af7a7950146..a4728700339 100644 --- a/source/blender/editors/space_view3d/view3d_navigate_move.c +++ b/source/blender/editors/space_view3d/view3d_navigate_move.c @@ -73,8 +73,7 @@ int viewmove_modal_impl(bContext *C, break; } case VIEW_CANCEL: { - viewmove_apply_reset(vod); - ED_view3d_camera_lock_sync(vod->depsgraph, vod->v3d, vod->rv3d); + viewops_data_state_restore(vod); ret = OPERATOR_CANCELLED; break; } diff --git a/source/blender/editors/space_view3d/view3d_navigate_roll.c b/source/blender/editors/space_view3d/view3d_navigate_roll.c index 84c168c1b42..c5baabe36ac 100644 --- a/source/blender/editors/space_view3d/view3d_navigate_roll.c +++ b/source/blender/editors/space_view3d/view3d_navigate_roll.c @@ -140,9 +140,7 @@ static int viewroll_modal(bContext *C, wmOperator *op, const wmEvent *event) break; } case VIEW_CANCEL: { - /* Note this does not remove auto-keys on locked cameras. */ - copy_qt_qt(vod->rv3d->viewquat, vod->init.quat); - ED_view3d_camera_lock_sync(vod->depsgraph, vod->v3d, vod->rv3d); + viewops_data_state_restore(vod); ret = OPERATOR_CANCELLED; break; } diff --git a/source/blender/editors/space_view3d/view3d_navigate_rotate.c b/source/blender/editors/space_view3d/view3d_navigate_rotate.c index 6f079e9fc81..b7293610961 100644 --- a/source/blender/editors/space_view3d/view3d_navigate_rotate.c +++ b/source/blender/editors/space_view3d/view3d_navigate_rotate.c @@ -317,21 +317,7 @@ int viewrotate_modal_impl(bContext *C, break; } case VIEW_CANCEL: { - /* Note this does not remove auto-keys on locked cameras. */ - copy_qt_qt(vod->rv3d->viewquat, vod->init.quat); - /* The offset may have change when rotating around objects or last-brush. */ - copy_v3_v3(vod->rv3d->ofs, vod->init.ofs); - /* The dist may have changed when orbiting from a camera view. - * In this case the `dist` is calculated based on the camera relative to the `ofs`. */ - vod->rv3d->dist = vod->init.dist; - - vod->rv3d->persp = vod->init.persp; - vod->rv3d->view = vod->init.view; - vod->rv3d->view_axis_roll = vod->init.view_axis_roll; - - /* NOTE: there is no need to restore "last" values (as set by #ED_view3d_lastview_store). */ - - ED_view3d_camera_lock_sync(vod->depsgraph, vod->v3d, vod->rv3d); + viewops_data_state_restore(vod); ret = OPERATOR_CANCELLED; break; } diff --git a/source/blender/editors/space_view3d/view3d_navigate_zoom.c b/source/blender/editors/space_view3d/view3d_navigate_zoom.c index a11fa3e702b..4b942c25930 100644 --- a/source/blender/editors/space_view3d/view3d_navigate_zoom.c +++ b/source/blender/editors/space_view3d/view3d_navigate_zoom.c @@ -369,16 +369,7 @@ int viewzoom_modal_impl(bContext *C, break; } case VIEW_CANCEL: { - /* Note this does not remove auto-keys on locked cameras. */ - vod->rv3d->dist = vod->init.dist; - /* The offset may have change when zooming to mouse position. */ - copy_v3_v3(vod->rv3d->ofs, vod->init.ofs); - vod->rv3d->camzoom = vod->init.camzoom; - /* Zoom to mouse position in camera view changes these values. */ - vod->rv3d->camdx = vod->init.camdx; - vod->rv3d->camdy = vod->init.camdy; - - ED_view3d_camera_lock_sync(vod->depsgraph, vod->v3d, vod->rv3d); + viewops_data_state_restore(vod); ret = OPERATOR_CANCELLED; break; }