WIP: Cleanup: Move definition of some navigation operators to 'view3d_ops.cc' #109973

Germano Cavalcante · 2023-07-11T17:01:04+02:00

2023-07-11 17:01:04 +02:00

Since we are unifying the implementation of the navigation operators
and using them in a non-operator utility, they no longer have a strong
dependency on the operator API and therefore the definition of these
operators can be moved to view3d_ops.cc itself.

This more closely matches the design seen in the transform code.

There are still many other navigation operators that can be adapted.

Another change is that lambda has been used instead of static functions
for the operators' invoke callbacks.

Since we are unifying the implementation of the navigation operators and using them in a non-operator utility, they no longer have a strong dependency on the operator API and therefore the definition of these operators can be moved to `view3d_ops.cc` itself. This more closely matches the design seen in the transform code. There are still many other navigation operators that can be adapted. Another change is that lambda has been used instead of static functions for the operators' invoke callbacks.

Germano Cavalcante requested review from Brecht Van Lommel 2023-07-11 17:01:56 +02:00

Germano Cavalcante requested review from Campbell Barton 2023-07-11 17:01:57 +02:00

Germano Cavalcante force-pushed navigation_cleanup from 40baf8e0b2 to 8ff9b6ee9f

2023-07-11 17:10:58 +02:00

Compare

Brecht Van Lommel commented

2023-07-11 18:08:37 +02:00

I'm not sure we should be matching the transform code or start using lambdas, those are both quite different than most operator implementations. I don't really see the benefit of this.

Germano Cavalcante commented

2023-07-11 18:59:58 +02:00

Perhaps I am quite used to the solution seen in the transform code. I don't have a very strong opinion either.

But it might be worth imagining on the side of someone venturing into the code for the first time.

When looking at the implementation of some operator, the first place to look is usually the files that have _ops in the name. After identifying the operator, the next place would be to check the definition of the callbacks.

Following this route, in the space_view3d editor the files would be in the order:
view3d_ops.cc > view3d_navigate_<ops_name>.cc > view3d_navigate.cc > view3d_navigate_<ops_name>.cc

Note that view3d_navigate_<ops_name>.cc is repeated twice. This is because the operator's invoke now calls a generic function used by most operators (view3d_navigate_invoke_impl). This function is in view3d_navigate.cc and it calls the implementation of each operator in the corresponding file.

Moving the operator definition to view3d_ops.cc, the path the developer would take would now be:
view3d_ops.cc > view3d_navigate.cc > view3d_navigate_<ops_name>.cc. There would be no going back and forth.

(The use of lambda for functions that are only one line seemed to be convenient in this case, but it's not really important).

Perhaps I am quite used to the solution seen in the transform code. I don't have a very strong opinion either. But it might be worth imagining on the side of someone venturing into the code for the first time. When looking at the implementation of some operator, the first place to look is usually the files that have `_ops` in the name. After identifying the operator, the next place would be to check the definition of the callbacks. Following this route, in the `space_view3d` editor the files would be in the order: `view3d_ops.cc` > `view3d_navigate_<ops_name>.cc` > `view3d_navigate.cc` > `view3d_navigate_<ops_name>.cc` Note that `view3d_navigate_<ops_name>.cc` is repeated twice. This is because the operator's invoke now calls a generic function used by most operators (`view3d_navigate_invoke_impl`). This function is in `view3d_navigate.cc` and it calls the implementation of each operator in the corresponding file. Moving the operator definition to `view3d_ops.cc`, the path the developer would take would now be: `view3d_ops.cc` > `view3d_navigate.cc` > `view3d_navigate_<ops_name>.cc`. There would be no going back and forth. (The use of `lambda` for functions that are only one line seemed to be convenient in this case, but it's not really important).

Brecht Van Lommel commented

2023-07-12 17:46:46 +02:00

Ok, I just search by operator name and would never open view3d_ops.cc , and in that case it's simpler to have the operator implementation only in 2 files rather than 3.

But this is mostly maintained by @ideasman42, so whatever he agrees with is fine with me.

Ok, I just search by operator name and would never open `view3d_ops.cc `, and in that case it's simpler to have the operator implementation only in 2 files rather than 3. But this is mostly maintained by @ideasman42, so whatever he agrees with is fine with me.

Brecht Van Lommel commented

2023-07-17 18:12:50 +02:00

For me this is not an improvement, but if @ideasman42 is fine with it I have no objection.

Brecht Van Lommel refused to review 2023-07-17 18:12:58 +02:00

Germano Cavalcante changed title from ~~Cleanup: Move definition of some navigation operators to 'view3d_ops.cc'~~ to WIP: Cleanup: Move definition of some navigation operators to 'view3d_ops.cc'

2023-07-24 14:58:48 +02:00

This pull request has changes conflicting with the target branch.

source/blender/editors/space_view3d/view3d_navigate.cc
source/blender/editors/space_view3d/view3d_navigate.hh
source/blender/editors/space_view3d/view3d_navigate_move.cc
source/blender/editors/space_view3d/view3d_navigate_ndof.cc
source/blender/editors/space_view3d/view3d_navigate_rotate.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u navigation_cleanup:mano-wii-navigation_cleanup

git checkout mano-wii-navigation_cleanup

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Cleanup: Move definition of some navigation operators to 'view3d_ops.cc' #109973

Checkout