Transform: support navigation #105764

Merged
Germano Cavalcante merged 1 commits from mano-wii/blender:transform_navigate into main 2023-05-19 23:45:32 +02:00

Design Task: #106008 (Support navigation for transform operators)

Transform: upport navigation while transforming in 3D view

Implements a new option in keymap settings:

  • "Navigate during Transform"

image

Note that with the option enabled, the keymap changes as follows:

Modal Map (Blender): Conflict: Current: New:
Increase Proportional Influence VIEW3D_OT_zoom Wheel Down Alt Wheel Down
Decrease Proportional Influence VIEW3D_OT_zoom Wheel Up Alt Wheel Up
Adjust Proportional Influence VIEW3D_OT_rotate Mouse/Trackpad Pan Alt Mouse/Trackpad Pan
Increase Max AutoIK Chain Length VIEW3D_OT_zoom Wheel Down Alt Wheel Down
Decrease Max AutoIK Chain Length VIEW3D_OT_zoom Wheel Up Alt Wheel Up
Automatic Constraint VIEW3D_OT_rotate Middle Mouse Alt Middle Mouse
Automatic Constraint Plane VIEW3D_OT_move Shift Middle Mouse Shift Alt Middle Mouse

As suggested in the design task, currently only 3 navigation operators are supported:

  • VIEW3D_OT_zoom,
  • VIEW3D_OT_rotate,
  • VIEW3D_OT_move

This patch only affects the following transform operators:

  • Translation,
  • Rotation,
  • Resize,
  • Edge Slide (from G+G),
  • Vert Slide (from G+G),

Navigation is not available when transforming with Release Confirm


Builds

https://builder.blender.org/download/patch/PR105764

Design Task: #106008 (Support navigation for transform operators) Transform: upport navigation while transforming in 3D view Implements a new option in keymap settings: - "Navigate during Transform" ![image](/attachments/f695f15d-489d-44f4-915d-b58eebfd6360) Note that with the option enabled, the keymap changes as follows: |Modal Map (Blender):| Conflict: | Current: | New: |---|---|---|--- | Increase Proportional Influence | VIEW3D_OT_zoom | Wheel Down | Alt Wheel Down | Decrease Proportional Influence | VIEW3D_OT_zoom | Wheel Up | Alt Wheel Up | Adjust Proportional Influence | VIEW3D_OT_rotate | Mouse/Trackpad Pan | Alt Mouse/Trackpad Pan | Increase Max AutoIK Chain Length | VIEW3D_OT_zoom | Wheel Down | Alt Wheel Down | Decrease Max AutoIK Chain Length | VIEW3D_OT_zoom | Wheel Up | Alt Wheel Up | Automatic Constraint | VIEW3D_OT_rotate | Middle Mouse | Alt Middle Mouse | Automatic Constraint Plane | VIEW3D_OT_move | Shift Middle Mouse | Shift Alt Middle Mouse As suggested in the design task, currently only 3 navigation operators are supported: - VIEW3D_OT_zoom, - VIEW3D_OT_rotate, - VIEW3D_OT_move This patch only affects the following transform operators: - Translation, - Rotation, - Resize, - Edge Slide (from G+G), - Vert Slide (from G+G), Navigation is not available when transforming with `Release Confirm` --- ## Builds https://builder.blender.org/download/patch/PR105764
120 KiB
Germano Cavalcante added the
Interest
User Interface
label 2023-03-14 15:27:04 +01:00
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105764) when ready.
Germano Cavalcante force-pushed transform_navigate from 05170c3810 to d2693be72d 2023-03-17 18:53:53 +01:00 Compare
Germano Cavalcante force-pushed transform_navigate from d2693be72d to 01dbdf1df8 2023-03-17 21:01:58 +01:00 Compare

Is there a design for a non-experimental implementation? There are hacks here and it's not clear to me what the design would look like without them. The main questions I see looking at this code:

How to get non-conflicting keybindings for transform modal and navigation?

The proposed solution seems to be to configure the keymap in such a way that they are compatible, which is done by changing some transform modal bindings. For custom keymaps it is up to the user to make sure the few navigation operators are compatible with transform.

But I also see there is conflict handling in modalkeymap_update_invoke. It seems to automatically figure out modifier keys that can be added to navigation operators if they conflict?

That seems rather unpredictable. What was the intent behind this? Are there conflicts that can not be resolved well in the default keymap, and some navigation keybindings have to be different when used during transform? Or is it to make things more automatic for custom keymaps?

How to get the WM event system to handle these additional key bindings?

The patch right now adds entries to the transform modal map with an operator, so that WM event handling as usual will recognize them as modal events. This solution is a hack that will need to be solved differently.

The most similar solution could be to add support for an operator making a dynamically allocated customized modal map at runtime when invoked. This is something that the WM could support.

Potentially this could be done generally in the WM, to add a given set of operators to a modal keymap to create a new temporary one. Some of the keymap handling in the 3D navigation code at least seems like it belongs more in the WM code but also has some hardcoded logic regarding the transform modal.

How to invoke the navigation operators from the transform operator?

This seems to be done by making some common functions called by both the operator callbacks and this new mechanism, bypassing the operator mechanism.

Maybe there is some way to use the actual operator callbacks with some changes elsewhere, but I haven't looked close enough to see if that's possible. Maybe it's ok roughly as is but needs some tweaks to make the implementation more clear.

Is there a design for a non-experimental implementation? There are hacks here and it's not clear to me what the design would look like without them. The main questions I see looking at this code: ### How to get non-conflicting keybindings for transform modal and navigation? The proposed solution seems to be to configure the keymap in such a way that they are compatible, which is done by changing some transform modal bindings. For custom keymaps it is up to the user to make sure the few navigation operators are compatible with transform. But I also see there is conflict handling in `modalkeymap_update_invoke`. It seems to automatically figure out modifier keys that can be added to navigation operators if they conflict? That seems rather unpredictable. What was the intent behind this? Are there conflicts that can not be resolved well in the default keymap, and some navigation keybindings have to be different when used during transform? Or is it to make things more automatic for custom keymaps? ### How to get the WM event system to handle these additional key bindings? The patch right now adds entries to the transform modal map with an operator, so that WM event handling as usual will recognize them as modal events. This solution is a hack that will need to be solved differently. The most similar solution could be to add support for an operator making a dynamically allocated customized modal map at runtime when invoked. This is something that the WM could support. Potentially this could be done generally in the WM, to add a given set of operators to a modal keymap to create a new temporary one. Some of the keymap handling in the 3D navigation code at least seems like it belongs more in the WM code but also has some hardcoded logic regarding the transform modal. ### How to invoke the navigation operators from the transform operator? This seems to be done by making some common functions called by both the operator callbacks and this new mechanism, bypassing the operator mechanism. Maybe there is some way to use the actual operator callbacks with some changes elsewhere, but I haven't looked close enough to see if that's possible. Maybe it's ok roughly as is but needs some tweaks to make the implementation more clear.
Author
Member

Thanks for the comments @brecht!

I confess that there was no Design task. Many decisions were considered in chat.

But now I created a Design task. Any changes can be proposed there. #106008


How to get non-conflicting keybindings for transform modal and navigations?

It seems to automatically figure out modifier keys
What was the intent behind this?

After considering the downside of messing with users' muscle memory, and the fact that we can be less restrictive with experimental features, it seemed convenient to implement this feature as experimental.

In fact we could simply overwrite the user's keymap. But it takes an operator to do this, and since we have different default keymaps and the user can have custom keymaps, it also seemed convenient to resolve conflicts automatically.


How to get the WM event system to handle these additional key bindings?

I'm not sure what these additional key bindings refer to.

The TRANSFORM_OT_modalkeymap_update operator replaces each conflicting modal kmi with another non-conflicting kmi. The user can then save changes to the Preferences if they prefer.

This TRANSFORM_OT_modalkeymap_update is something intended to be removed after this feature is no longer experimental.

The patch also implements the ed_view3d_kmi_navigation_fill_array function in ViewOpsData *ED_view3d_navigation_init(bContext *C). The data stored in ViewOpsData * is something that already exists for navigation operators. But now an array of kmis has been added so we can detect which navigation operation was called.

A user can modify the keymap while using Blender, so it's important to redetect what they are.


How to invoke the navigation operators from the transform operator?

WM_event_match was exposed.

To perform the navigation operation the solution is:

  • get the current event in the operator (TRANSFORM_OT_ or any other)
  • check if it matches (WM_event_match) to some kmi from the kmis array in ViewOpsData*
  • If it matches, call the corresponding _invoke if it is not in modal mode or the corresponding _modal. (See view3d_navigation_invoke and view3d_navigation_modal).
Thanks for the comments @brecht! I confess that there was no Design task. Many decisions were considered in chat. But now I created a Design task. Any changes can be proposed there. #106008 --- > ### How to get non-conflicting keybindings for transform modal and navigations? > It seems to automatically figure out modifier keys > What was the intent behind this? After considering the downside of messing with users' muscle memory, and the fact that we can be less restrictive with experimental features, it seemed convenient to implement this feature as experimental. In fact we could simply overwrite the user's keymap. But it takes an operator to do this, and since we have different default keymaps and the user can have custom keymaps, it also seemed convenient to resolve conflicts automatically. --- > ### How to get the WM event system to handle these additional key bindings? I'm not sure what these additional key bindings refer to. The `TRANSFORM_OT_modalkeymap_update` operator replaces each conflicting modal kmi with another non-conflicting kmi. The user can then save changes to the Preferences if they prefer. This `TRANSFORM_OT_modalkeymap_update` is something intended to be removed after this feature is no longer experimental. The patch also implements the `ed_view3d_kmi_navigation_fill_array` function in `ViewOpsData *ED_view3d_navigation_init(bContext *C)`. The data stored in `ViewOpsData *` is something that already exists for navigation operators. But now an array of `kmi`s has been added so we can detect which navigation operation was called. A user can modify the keymap while using Blender, so it's important to redetect what they are. --- > ### How to invoke the navigation operators from the transform operator? `WM_event_match` was exposed. To perform the navigation operation the solution is: - get the current event in the operator (TRANSFORM_OT_ or any other) - check if it matches (`WM_event_match`) to some kmi from the kmis array in `ViewOpsData*` - If it matches, call the corresponding `_invoke` if it is not in modal mode or the corresponding `_modal`. (See `view3d_navigation_invoke` and `view3d_navigation_modal`).
Germano Cavalcante force-pushed transform_navigate from 01dbdf1df8 to 299ae73e89 2023-03-23 15:25:13 +01:00 Compare
Germano Cavalcante requested review from Brecht Van Lommel 2023-03-23 15:36:00 +01:00
Germano Cavalcante requested review from Campbell Barton 2023-03-23 15:36:00 +01:00
Author
Member

I carefully studied the solution (I noticed that merging the navigation operators into one wasn't going to make much difference).

I also split the branch into 3 commits:

  • View 3D: new utility to provide navigation in operators
  • Transform: support navigation (no experimental; default keymaps have been edited)
  • Experimental: make navigation while transforming experimental (add the 2 operators and undo keymap changes)

And I simplified the code (removing unrelated cleanups) and added more documentation to it.

(Advantages and disadvantages are mentioned in the description of the first commit).
(I noticed some regressions due to old bad rebases - fixed).

I carefully studied the solution (I noticed that merging the navigation operators into one wasn't going to make much difference). I also split the branch into 3 commits: - View 3D: new utility to provide navigation in operators - Transform: support navigation (no experimental; default keymaps have been edited) - Experimental: make navigation while transforming experimental (add the 2 operators and undo keymap changes) And I simplified the code (removing unrelated cleanups) and added more documentation to it. (Advantages and disadvantages are mentioned in the description of the first commit). (I noticed some regressions due to old bad rebases - fixed).

How to get non-conflicting keybindings for transform modal and navigations?

It seems to automatically figure out modifier keys
What was the intent behind this?

After considering the downside of messing with users' muscle memory, and the fact that we can be less restrictive with experimental features, it seemed convenient to implement this feature as experimental.

In fact we could simply overwrite the user's keymap. But it takes an operator to do this, and since we have different default keymaps and the user can have custom keymaps, it also seemed convenient to resolve conflicts automatically.

My question was about how keymap configuration should work, ignoring compatibility for a moment. I can see a few ways:

  1. The transform modal map contains items for navigation operators, which the default keymaps can then set and users can customize.
  2. A new modal map is added for navigation during modal operators, that would be used for the transform operator and potentially other operators in the future.
  3. Blender automatically uses items from the 3D viewport keymap and uses those during transform operators, no change to any keymap configuration from the user point of view.

It seems you are assuming it must be (1). If you have to have different keybinding for navigation with and without transform, then that or (2) may be necessary. If not, it's adding unnecessary complexity and work for users to configure bindings in two or more places.

So for me the first question is, do we need to have different navigation keybindings with and without transform, or can they be the same? If they can't be the same, what are the conflicts in the default keymap?

How to get the WM event system to handle these additional key bindings?

..

The answer to this depends on the right design for the previous question.

How to invoke the navigation operators from the transform operator?

..

Right, I can see how the implementation was done, I was more question if it could be done in a way that doesn't involve leaking implementation internals of WM event into the 3D viewport navigation code.

Both to keep the code clean, and because we may want the same in 2D editors.

> > ### How to get non-conflicting keybindings for transform modal and navigations? > > It seems to automatically figure out modifier keys > > What was the intent behind this? > > After considering the downside of messing with users' muscle memory, and the fact that we can be less restrictive with experimental features, it seemed convenient to implement this feature as experimental. > > In fact we could simply overwrite the user's keymap. But it takes an operator to do this, and since we have different default keymaps and the user can have custom keymaps, it also seemed convenient to resolve conflicts automatically. My question was about how keymap configuration should work, ignoring compatibility for a moment. I can see a few ways: 1) The transform modal map contains items for navigation operators, which the default keymaps can then set and users can customize. 2) A new modal map is added for navigation during modal operators, that would be used for the transform operator and potentially other operators in the future. 3) Blender automatically uses items from the 3D viewport keymap and uses those during transform operators, no change to any keymap configuration from the user point of view. It seems you are assuming it must be (1). If you have to have different keybinding for navigation with and without transform, then that or (2) may be necessary. If not, it's adding unnecessary complexity and work for users to configure bindings in two or more places. So for me the first question is, do we need to have different navigation keybindings with and without transform, or can they be the same? If they can't be the same, what are the conflicts in the default keymap? > > ### How to get the WM event system to handle these additional key bindings? > .. The answer to this depends on the right design for the previous question. > > ### How to invoke the navigation operators from the transform operator? > .. Right, I can see how the implementation was done, I was more question if it could be done in a way that doesn't involve leaking implementation internals of WM event into the 3D viewport navigation code. Both to keep the code clean, and because we may want the same in 2D editors.
Author
Member

For the items mentioned in "how keymap configuration should work", the closest to the current solution is the 3 (Blender automatically uses items from the 3D viewport keymap ...)

To better understand the conflicts with the transform's modal keymap, let's consider one of the cases: The WHEELUPMOUSE (mouse scrool).

Conflict:

  • For navigation in 3D View: WHEELUPMOUSE performs Zoom
  • For transform operations: WHEELUPMOUSE performs "PROPORTIONAL_SIZE_UP"

Solution in patch:

  • Change the keymap item for "PROPORTIONAL_SIZE_UP" to use Alt + WHEELUPMOUSE instead WHEELUPMOUSE

With the current patch, what happens if we don't make this change in the modal keymap:

  • "PROPORTIONAL_SIZE_UP" is performed but Zoom is completely ignored in the 3D View.

With a small change we could make both operations run. But this is not desirable.

For the items mentioned in "how keymap configuration should work", the closest to the current solution is the `3` (Blender automatically uses items from the 3D viewport keymap ...) To better understand the conflicts with the transform's modal keymap, let's consider one of the cases: The `WHEELUPMOUSE` (mouse scrool). **Conflict:** - For navigation in 3D View: `WHEELUPMOUSE` performs Zoom - For transform operations: `WHEELUPMOUSE` performs "PROPORTIONAL_SIZE_UP" **Solution in patch:** - Change the keymap item for "PROPORTIONAL_SIZE_UP" to use `Alt` + `WHEELUPMOUSE` instead `WHEELUPMOUSE` **With the current patch, what happens if we don't make this change in the modal keymap:** - "PROPORTIONAL_SIZE_UP" is performed but Zoom is completely ignored in the 3D View. With a small change we could make both operations run. But this is not desirable.

Ok, then for the actual implementation that can land in main, the keymap changes should get made in keymap_data/blender_default.py and keymap_data/industry_compatible_data.py instead of through TRANSFORM_OT_modalkeymap_update, with review and approval by the UI team?

If we don't want to enable this by default yet (because we may want to postpone breaking keymap changes to 4.0) or allow preserving compatibility, then still it seems better to implement this as a keymap option.

Ok, then for the actual implementation that can land in main, the keymap changes should get made in `keymap_data/blender_default.py` and `keymap_data/industry_compatible_data.py` instead of through `TRANSFORM_OT_modalkeymap_update`, with review and approval by the UI team? If we don't want to enable this by default yet (because we may want to postpone breaking keymap changes to 4.0) or allow preserving compatibility, then still it seems better to implement this as a keymap option.
Author
Member

Yes, the TRANSFORM_OT_modalkeymap_update operator is just to enable this feature as Experimental.
When we activate this operator in the Experimental tab, it displays a popup that shows all the conflicts that will be resolved automatically:
image


If we don't want to enable this by default yet (because we may want to postpone breaking keymap changes to 4.0) or allow preserving compatibility, then still it seems better to implement this as a keymap option.

I think it needs a more elaborate design. Would it be something like this?
image

Yes, the `TRANSFORM_OT_modalkeymap_update` operator is just to enable this feature as Experimental. When we activate this operator in the `Experimental` tab, it displays a popup that shows all the conflicts that will be resolved automatically: ![image](/attachments/286faa98-4ce6-4c34-a651-af66a575c2e8) --- > If we don't want to enable this by default yet (because we may want to postpone breaking keymap changes to 4.0) or allow preserving compatibility, then still it seems better to implement this as a keymap option. I think it needs a more elaborate design. Would it be something like this? ![image](/attachments/3f1d6dcb-8979-4d84-8641-063c91f1a22c)
121 KiB
168 KiB

If we don't want to enable this by default yet (because we may want to postpone breaking keymap changes to 4.0) or allow preserving compatibility, then still it seems better to implement this as a keymap option.

I think it needs a more elaborate design. Would it be something like this?

Just a single checkbox should be enough I think. If someone wants to fine tune things they can edit the keymap.

> > If we don't want to enable this by default yet (because we may want to postpone breaking keymap changes to 4.0) or allow preserving compatibility, then still it seems better to implement this as a keymap option. > > I think it needs a more elaborate design. Would it be something like this? Just a single checkbox should be enough I think. If someone wants to fine tune things they can edit the keymap.
Germano Cavalcante force-pushed transform_navigate from 299ae73e89 to ce4277f062 2023-03-24 18:15:46 +01:00 Compare
Author
Member

Added a single checkbox to update the keymaps and avoid conflicts.
image

If the user does not enable this option they will not notice the new feature unless they try to navigate 3D View with the few cases where there is no conflict (such as Zoom without proportional editing).

Added a single checkbox to update the keymaps and avoid conflicts. ![image](/attachments/5041d640-da98-4bc0-919d-121ff22357d5) If the user does not enable this option they will not notice the new feature unless they try to navigate 3D View with the few cases where there is no conflict (such as Zoom without proportional editing).
Germano Cavalcante force-pushed transform_navigate from ce4277f062 to b07cc42feb 2023-03-24 21:07:38 +01:00 Compare
Germano Cavalcante force-pushed transform_navigate from b07cc42feb to b226303639 2023-03-27 06:34:27 +02:00 Compare
Author
Member

I forced a branch update which promotes more localized changes.
The changes are:

  • Add "allow_navigation" property to the following transform operators:
    • transform.translate,
    • transform.rotate,
    • transform.resize,
    • transform.edge_slide,
    • transform.vert_slide,
    • transform.transform

When this property is not enabled, it prevents the user from navigating during transform operations even if there is no keymap conflict.

Another change is:

  • Remove keymap changes for the "Industry Compatible" configuration.

This configuration does not implement shortcuts for the main transform operators, leaving only those that come from tools and menus that are activated with "release_confirm", (thus not being able to have navigation).


With these changes, the option Enable navigation and resolve keymap conflicts, when disabled, does not bring any functional changes compared to current behavior without navigation.

And since the only 5 chosen operators are affected, this change is unlikely to bring unpredictable results to the others (Skin Resize, To Sphere, Shear, Bend, Shrink/Fatten, Tilt, Trackball, Push/Pull, Crease, Bone Size, Bone Envelope, Bone Envelope Distance, Curve Shrink/Fatten, Grease Pencil Shrink/Fatten, Bone Roll, Bevel Weight, Align, Grease Pencil Opacity)

I forced a branch update which promotes more localized changes. The changes are: - Add "allow_navigation" property to the following transform operators: - `transform.translate`, - `transform.rotate`, - `transform.resize`, - `transform.edge_slide`, - `transform.vert_slide`, - `transform.transform` When this property is not enabled, it prevents the user from navigating during transform operations even if there is no keymap conflict. Another change is: - Remove keymap changes for the "Industry Compatible" configuration. This configuration does not implement shortcuts for the main transform operators, leaving only those that come from tools and menus that are activated with "release_confirm", (thus not being able to have navigation). --- With these changes, the option `Enable navigation and resolve keymap conflicts`, when disabled, does not bring any functional changes compared to current behavior without navigation. And since the only 5 chosen operators are affected, this change is unlikely to bring unpredictable results to the others (Skin Resize, To Sphere, Shear, Bend, Shrink/Fatten, Tilt, Trackball, Push/Pull, Crease, Bone Size, Bone Envelope, Bone Envelope Distance, Curve Shrink/Fatten, Grease Pencil Shrink/Fatten, Bone Roll, Bevel Weight, Align, Grease Pencil Opacity)
Brecht Van Lommel requested changes 2023-03-27 18:09:21 +02:00
Brecht Van Lommel left a comment
Owner

Not a detailed review, but mainly wondering if we can somehow call these operator through the regular operator interface and avoid some of the code complexity?

Not a detailed review, but mainly wondering if we can somehow call these operator through the regular operator interface and avoid some of the code complexity?
@ -253,0 +259,4 @@
"'Increase Max AutoIK Chain Length' will change from 'Wheel Down' to 'Alt Wheel Down' (as it conflicts with 'VIEW3D_OT_zoom').\n"
"'Decrease Max AutoIK Chain Length' will change from 'Wheel Up' to 'Alt Wheel Up' (as it conflicts with 'VIEW3D_OT_zoom').\n"
"'Automatic Constraint' will change from 'Middle Mouse' to 'Alt Middle Mouse' (as it conflicts with 'VIEW3D_OT_rotate').\n"
"'Automatic Constraint Plane' will change from 'Shift Middle Mouse' to 'Shift Alt Middle Mouse' (as it conflicts with 'VIEW3D_OT_move')"),

This doesn't need to be so detailed, I suggest:

use_transform_navigation
Transform Navigation
Enable view navigation while using transform operators. Change proportional influence, automatic constraints and auto IK chaing length shortcuts will require holding Alt key
This doesn't need to be so detailed, I suggest: ``` use_transform_navigation Transform Navigation Enable view navigation while using transform operators. Change proportional influence, automatic constraints and auto IK chaing length shortcuts will require holding Alt key ```
mano-wii marked this conversation as resolved
@ -58,6 +59,24 @@ class Prefs(bpy.types.KeyConfigPreferences):
update=update_fn,
)
use_alt_in_transform_modals: BoolProperty(

I would not bother adding this to the 2.7 keymap at all.

I would not bother adding this to the 2.7 keymap at all.
mano-wii marked this conversation as resolved
@ -1659,0 +1753,4 @@
}
size_t kmi_array_size = sizeof(struct wmKeyMapItem *) * kmi_len;
ViewOpsData *vod = MEM_callocN(sizeof(ViewOpsData) + kmi_array_size, __func__);

I'd rather not do this kind of allocation trick, and zero sized arrays don't work in C++ which we are gradually porting files to.

I don't think we need to precompute this at all. Just iterating over the keymap on every event should not have a real performance impact, we do that continuously in the WM anyway.

I'd rather not do this kind of allocation trick, and zero sized arrays don't work in C++ which we are gradually porting files to. I don't think we need to precompute this at all. Just iterating over the keymap on every event should not have a real performance impact, we do that continuously in the WM anyway.
@ -1659,0 +1866,4 @@
return viewmove_invoke_impl(vod, event);
case V3D_VIEW_PAN:
data = RNA_enum_get(ptr, "type");
return viewpan_invoke_impl(vod, data);

Can we avoid having these _impl functions by allocating a temporary wmOperator? And then calling ot->poll, ot->invoke, ot->modal?

Can we avoid having these `_impl` functions by allocating a temporary `wmOperator`? And then calling `ot->poll`, `ot->invoke`, `ot->modal`?
Germano Cavalcante reviewed 2023-03-27 21:23:32 +02:00
Germano Cavalcante left a comment
Author
Member

The proposed changes to the utility make using PASS_THROUGH more feasible.
Using ot->poll, ot->invoke, ot->modal makes it very difficult to use a ViewOpsData common to all operations and also makes it difficult to control the handles that are added to the window.

The proposed changes to the utility make using `PASS_THROUGH` more feasible. Using `ot->poll`, `ot->invoke`, `ot->modal` makes it very difficult to use a `ViewOpsData` common to all operations and also makes it difficult to control the handles that are added to the window.
@ -1659,0 +1866,4 @@
return viewmove_invoke_impl(vod, event);
case V3D_VIEW_PAN:
data = RNA_enum_get(ptr, "type");
return viewpan_invoke_impl(vod, data);
Author
Member

The idea of having a navigation utility had as one of the goals not to depend on the navigation operators.

The advantages would be:

  • Greater control of what is performed (only navigation);
  • Simplified indication of when the 3D View has changed (no need to compare matrices);
  • Possibility to run the operator together with navigation;

However, the proposal seeks to go back to the PASS_THROUGH idea. So no changes in navigation operators are required.

This was the initial idea and it was implemented at https://archive.blender.org/developer/D2624

It is not possible to use ot->invoke and capture handles created for modals without hacks and other complexities in the code.

I would prefer use the utility, but I can let this idea for another occasion and create another PR proposing to use PASS_THROUGH (if preferred).

The idea of having a navigation utility had as one of the goals not to depend on the navigation operators. The advantages would be: - Greater control of what is performed (only navigation); - Simplified indication of when the 3D View has changed (no need to compare matrices); - Possibility to run the operator together with navigation; However, the proposal seeks to go back to the `PASS_THROUGH` idea. So no changes in navigation operators are required. This was the initial idea and it was implemented at https://archive.blender.org/developer/D2624 It is not possible to use `ot->invoke` and capture handles created for modals without hacks and other complexities in the code. I would prefer use the utility, but I can let this idea for another occasion and create another PR proposing to use `PASS_THROUGH` (if preferred).
Germano Cavalcante force-pushed transform_navigate from 3645623271 to d8073a6eff 2023-03-28 19:34:30 +02:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Germano Cavalcante force-pushed transform_navigate from d276604da3 to ca70305618 2023-03-29 04:21:52 +02:00 Compare
Germano Cavalcante changed title from Transform: support navigation to WIP: Transform: support navigation 2023-03-29 21:36:32 +02:00
Germano Cavalcante force-pushed transform_navigate from ca70305618 to 8ee1162d16 2023-04-21 21:20:38 +02:00 Compare
Author
Member

I removed the entire navigation utility in favor of a solution similar to Knife Tool (with OPERATOR_PASS_THROUGH).

But in fact this solution is impractical, as it can miss custom navigation events and can also miss, during navigation, modal transform events. This can cause, for example, that the Snap remains enabled even with the Ctrl released.

This indicates that a navigation utility is really needed. But related pull requests (!106279) are already out of date. (And creating a utility like that is not my expertise).

I feel like maybe someone more experienced is needed to work on this, otherwise this feature may be something that will never get implemented.

I removed the entire navigation utility in favor of a solution similar to Knife Tool (with `OPERATOR_PASS_THROUGH`). But in fact this solution is impractical, as it can miss custom navigation events and can also miss, during navigation, modal transform events. This can cause, for example, that the Snap remains enabled even with the `Ctrl` released. This indicates that a navigation utility is really needed. But related pull requests (!106279) are already out of date. (And creating a utility like that is not my expertise). I feel like maybe someone more experienced is needed to work on this, otherwise this feature may be something that will never get implemented.

I thought the direction this was going is fine. Maybe try poking @ideasman42 in chat? And if he does not have time, maybe he can agree to have someone else review it?

I agree this is an important feature to solve.

I thought the direction this was going is fine. Maybe try poking @ideasman42 in chat? And if he does not have time, maybe he can agree to have someone else review it? I agree this is an important feature to solve.

I'm not so keen to have to support this functionality as it introduces key-conflicts and behavior which isn't well defined. Although if users really find this handy - each of these issues can be handled/resolved.

Conflicts:

  • MMB to constrain conflicts with orbit.
  • Mouse wheel conflicts with proportional edit size.

Odd behavior:

  • Currently transform "jumps" on the first cursor motion after view navigation completes. While it's understandable some mouse pointer recalculation is needed, ideally this would happen immediately instead of being delayed until the user moves the cursor.

  • View locked cameras has some potential for strange behavior. You can for example, transform a camera that's locked to the view... from some quick tests it's not failing badly, so we could leave this as-is. Although having undo steps added by an action during transform seems a bit odd. It might be simplest to disable view locking while transforming if the view-locked object it's self is being transformed.

Arbitrary operator execution:

This change allows any operator to run, passing through view operators is only ensured by convention however there is nothing stopping someone from setting up a key-map that performs destructive edits. I'd assume we would not consider these cases bugs (if reported), as it seems like the user is going to some effort to shoot themselves in the foot.

Having said that - there may be cases add-ons use middle mouse input for other editors (sequencer for e.g.) that cause sequence strips to be added/removed.

Hard coded MMB:

Ideally this patch wouldn't hard code the key used for navigation.

Further work:

Would this be supported for editors besides the 3D viewport?

I'm not so keen to have to support this functionality as it introduces key-conflicts and behavior which isn't well defined. Although if users really find this handy - each of these issues can be handled/resolved. ### Conflicts: - MMB to constrain conflicts with orbit. - Mouse wheel conflicts with proportional edit size. ### Odd behavior: - Currently transform "jumps" on the first cursor motion after view navigation completes. While it's understandable some mouse pointer recalculation is needed, ideally this would happen immediately instead of being delayed until the user moves the cursor. - View locked cameras has some potential for strange behavior. You can for example, transform a camera that's locked to the view... from some quick tests it's not failing badly, so we could leave this as-is. Although having undo steps added by an action during transform seems a bit odd. It might be simplest to disable view locking while transforming if the view-locked object it's self is being transformed. ### Arbitrary operator execution: This change allows any operator to run, passing through view operators is only ensured by convention however there is nothing stopping someone from setting up a key-map that performs destructive edits. I'd assume we would not consider these cases bugs (if reported), as it seems like the user is going to some effort to shoot themselves in the foot. Having said that - there may be cases add-ons use middle mouse input for other editors (sequencer for e.g.) that cause sequence strips to be added/removed. ### Hard coded MMB: Ideally this patch wouldn't hard code the key used for navigation. ### Further work: Would this be supported for editors besides the 3D viewport?

@ideasman42 my understanding is that this is essential for basepoint snapping to be usable, which in turn is an important feature that's been stuck for a while.

Conflicts:

  • MMB to constrain conflicts with orbit.
  • Mouse wheel conflicts with proportional edit size.

The design task #106279 has the complete list and proposed remapping.

Arbitrary operator execution:

This change allows any operator to run, passing through view operators is only ensured by convention however there is nothing stopping someone from setting up a key-map that performs destructive edits.

The previous version of this patch did not allow arbitrary operator execution, and I think that was better. It was meant to work based on the refactoring in #106279.

@mano-wii, I'm not sure why you changed it since you comment it's impractical. Maybe in the hope of having a simpler change that is easier to get approved, but it's confusing this way.

Odd behavior:

  • Currently transform "jumps" on the first cursor motion after view navigation completes. While it's understandable some mouse pointer recalculation is needed, ideally this would happen immediately instead of being delayed until the user moves the cursor.

This seems solvable, especially when not using operator pass through it should be straightforward to ensure refreshes happen.

  • View locked cameras has some potential for strange behavior. You can for example, transform a camera that's locked to the view... from some quick tests it's not failing badly, so we could leave this as-is. Although having undo steps added by an action during transform seems a bit odd. It might be simplest to disable view locking while transforming if the view-locked object it's self is being transformed.

In this case navigation can just be disabled.

@ideasman42 my understanding is that this is essential for basepoint snapping to be usable, which in turn is an important feature that's been stuck for a while. > ### Conflicts: > > - MMB to constrain conflicts with orbit. > - Mouse wheel conflicts with proportional edit size. The design task #106279 has the complete list and proposed remapping. > ### Arbitrary operator execution: > > This change allows any operator to run, passing through view operators is only ensured by convention however there is nothing stopping someone from setting up a key-map that performs destructive edits. The previous version of this patch did not allow arbitrary operator execution, and I think that was better. It was meant to work based on the refactoring in #106279. @mano-wii, I'm not sure why you changed it since you comment it's impractical. Maybe in the hope of having a simpler change that is easier to get approved, but it's confusing this way. > ### Odd behavior: > > - Currently transform "jumps" on the first cursor motion after view navigation completes. While it's understandable some mouse pointer recalculation is needed, ideally this would happen immediately instead of being delayed until the user moves the cursor. This seems solvable, especially when not using operator pass through it should be straightforward to ensure refreshes happen. > - View locked cameras has some potential for strange behavior. You can for example, transform a camera that's locked to the view... from some quick tests it's not failing badly, so we could leave this as-is. Although having undo steps added by an action during transform seems a bit odd. It might be simplest to disable view locking while transforming if the view-locked object it's self is being transformed. In this case navigation can just be disabled.
Germano Cavalcante force-pushed transform_navigate from 8ee1162d16 to 8b873dca71 2023-04-28 17:10:00 +02:00 Compare
Germano Cavalcante changed title from WIP: Transform: support navigation to Transform: support navigation 2023-04-28 17:17:03 +02:00
Author
Member

I reverted and updated the solution of the ED_view3d_navigation_do utility. Its implementation can be seen in this PR by commit a2971c8ca00e69318c7691e018c37ae3a2224735

Compared to Knife Tool navigation (which uses PASS_THROUGH) the ED_view3d_navigation_do utility does not yet support NDOF events. I plan to work on that later.

But unlike the Knife Tool this utility is adaptable to custom navigation keymaps.

Another thing that hasn't been implemented yet are the navigation modal keymaps. (For comparison, this doesn't work for the Knife either).

I also edited the descriptions of this feature in the UI.

I reverted and updated the solution of the `ED_view3d_navigation_do` utility. Its implementation can be seen in this PR by commit a2971c8ca00e69318c7691e018c37ae3a2224735 Compared to Knife Tool navigation (which uses PASS_THROUGH) the `ED_view3d_navigation_do` utility does not yet support NDOF events. I plan to work on that later. But unlike the Knife Tool this utility is adaptable to custom navigation keymaps. Another thing that hasn't been implemented yet are the navigation modal keymaps. (For comparison, this doesn't work for the Knife either). I also edited the descriptions of this feature in the UI.
Germano Cavalcante force-pushed transform_navigate from 8b873dca71 to f85a60c943 2023-04-28 21:24:05 +02:00 Compare
Brecht Van Lommel requested changes 2023-05-19 13:54:32 +02:00
@ -377,0 +392,4 @@
# Also update the "allow navigation" property in the transform operators called through menus.
bpy.types.VIEW3D_MT_transform.allow_navigation = kc_prefs.use_transform_navigation
bpy.types.VIEW3D_MT_transform_object.allow_navigation = kc_prefs.use_transform_navigation
bpy.types.VIEW3D_MT_transform_armature.allow_navigation = kc_prefs.use_transform_navigation

This is a problem since this will not be properly reset when switching to another keymap. Instead the menu code could get the value like this:

getattr(bpy.context.window_manager.keyconfigs.active.preferences, "use_transform_navigation", False)
This is a problem since this will not be properly reset when switching to another keymap. Instead the menu code could get the value like this: ``` getattr(bpy.context.window_manager.keyconfigs.active.preferences, "use_transform_navigation", False) ```
mano-wii marked this conversation as resolved
@ -1939,0 +1995,4 @@
return NULL;
}
struct wmKeyMapItem *km_items[80];

Since this is C++ code, can just use a Vector and no use such hardcoded numbers.

Since this is C++ code, can just use a Vector and no use such hardcoded numbers.
mano-wii marked this conversation as resolved
@ -1939,0 +1997,4 @@
struct wmKeyMapItem *km_items[80];
int kmi_len;
ed_view3d_kmi_navigation_fill_array(C, ARRAY_SIZE(km_items), km_items, &kmi_len);

There is no need to cache this and doing these memory allocation tricks. Computing it every time in ED_view3d_navigation_do should not have a measurable performance impact.

There is no need to cache this and doing these memory allocation tricks. Computing it every time in `ED_view3d_navigation_do` should not have a measurable performance impact.
mano-wii marked this conversation as resolved
@ -1939,0 +2093,4 @@
}
if (op_return != OPERATOR_CANCELLED) {
/* XXX: Although this doubles the work, operators need to update some values using the updated

Not sure what the meaning of XXX is. Does it mean you expect this to be fixed still. If not, can XXX be left out?

Not sure what the meaning of XXX is. Does it mean you expect this to be fixed still. If not, can XXX be left out?
mano-wii marked this conversation as resolved

For the UI, instead of a Transform section I suggest to add a Navigate during Transform boolean under 3D view, since it currently does not affect 2D views.

For the UI, instead of a Transform section I suggest to add a `Navigate during Transform` boolean under 3D view, since it currently does not affect 2D views.
Germano Cavalcante force-pushed transform_navigate from f85a60c943 to 483707ce3b 2023-05-19 20:25:14 +02:00 Compare
Author
Member

Updated as per suggestions.
Changes can be checked in 588394f506 (and 0ef1b22cee due a rebase error)
To know:

  • Move Navigate during Transform boolean under 3D view in keymap settings
  • Use getattr(context.window_manager.keyconfigs.active.preferences, "use_transform_navigation", False) in Transform menus
  • Do not cache keymap items (transverse kmi in ED_view3d_navigation_do)
  • Improve XXX comment (removing XXX)
Updated as per suggestions. Changes can be checked in 588394f506 (and 0ef1b22cee due a rebase error) To know: - Move `Navigate during Transform` boolean under `3D view` in keymap settings - Use `getattr(context.window_manager.keyconfigs.active.preferences, "use_transform_navigation", False)` in Transform menus - Do not cache keymap items (transverse kmi in `ED_view3d_navigation_do`) - Improve `XXX` comment (removing XXX)
Brecht Van Lommel approved these changes 2023-05-19 22:24:18 +02:00
Germano Cavalcante force-pushed transform_navigate from 0ef1b22cee to e937df1a7c 2023-05-19 23:24:06 +02:00 Compare
Germano Cavalcante merged commit 33c13ae6e3 into main 2023-05-19 23:45:32 +02:00
Germano Cavalcante deleted branch transform_navigate 2023-05-19 23:45:33 +02:00
First-time contributor

Wow, well done, it works well with snap base branch too.

Wow, well done, it works well with **snap base** branch too.
Howard Trickey referenced this issue from a commit 2023-05-29 02:51:38 +02: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#105764
No description provided.