blender
/
blender
129
398
Fork 572
Code Issues 6.2k Pull Requests 745 Projects 19 Wiki Activity

Sculpt: Add Transform, Trim, and Mesh Filter operators to Sculpt menu #104718

Merged
Joseph Eagar merged 19 commits from Tarek-Yasser/blender:sculpt_mode_add_menu_operators into main 2023-03-08 01:18:34 +01:00
Conversation 25 Commits 19 Files Changed 9 +234 -49
Tarek-Yasser commented 2023-02-13 21:16:52 +01:00
Contributor
Copy Link

Hello, this is a small PR to check that my understanding of #102427 is correct before moving on to the rest of the issue.
This PR contains the updated UI of the Sculpt menu only. Other menus will be submitted for review later.

Currently exposed operators:

  • Move
  • Rotate
  • Scale
  • Box Trim (Trim Mode ="Difference")
  • Lasso Trim (Trim Mode ="Difference")
  • Box Add (Trim Mode ="Join")
  • Lasso Add (Trim Mode ="Join")
  • Line Project
  • Smooth
  • Surface Smooth
  • Scale (Could be left out?)
  • Inflate
  • Random
  • Relax Topology
  • Relax Face Set Boundaries
  • Sharpen
  • Enhance Details
  • Erase Displacement

The original issue specifies Relax Face Set Boundaries and Erase Displacement. I'm not quite sure if this is done in the UI code or somewhere else.

Hello, this is a small PR to check that my understanding of #102427 is correct before moving on to the rest of the issue. This PR contains the updated UI of the `Sculpt` menu only. Other menus will be submitted for review later. Currently exposed operators: * Move * Rotate * Scale * Box Trim (Trim Mode ="Difference") * Lasso Trim (Trim Mode ="Difference") * Box Add (Trim Mode ="Join") * Lasso Add (Trim Mode ="Join") * Line Project * Smooth * Surface Smooth * ~~Scale (Could be left out?)~~ * Inflate * Random * Relax Topology * Relax Face Set Boundaries * Sharpen * Enhance Details * Erase Displacement The original issue specifies `Relax Face Set Boundaries` and `Erase Displacement`. I'm not quite sure if this is done in the UI code or somewhere else.
Tarek-Yasser added 4 commits 2023-02-13 21:16:55 +01:00
7ddb21152a UI: Add 8 more operators to the sculpt menu. 
Added "Toggle Visibility", "Hide Active Face Set", "Invert Visible",
"Box Trim", "Lasso Trim", "Line Project", "Fair Positions", "Fair
Tangency".
256489d011 UI: Add the 8 variants of sculpt mesh filter to the sculpt menu. 
The 8 variants are: 'SMOOTH', 'SURFACE_SMOOTH', 'INFLATE', 'RELAX',
'RELAX_FACE_SETS' 'SHARPEN', 'ENHANCE_DETAILS' 'ERASE_DISCPLACEMENT',
'RANDOM'.
Tarek-Yasser requested review from Joseph Eagar 2023-02-13 21:19:18 +01:00
Tarek-Yasser requested review from Julien Kaspar 2023-02-13 21:19:19 +01:00
Julien Kaspar requested changes 2023-02-14 11:00:16 +01:00
Julien Kaspar left a comment
Member
Copy Link

Some notes:

  • 'Toggle Visibility' and 'Hide Active Face Set' still need the OPTYPE_DEPENDS_ON_CURSOR flag to be useable.

  • 'Lasso Add' needs to be set to Join. Right now it uses Difference by default.

The mesh filters work surprisingly well! But they need some additions in the UI. These menu operators should be treated like other modal operators and need visual indications such as:

  • A visual change in the cursor to indicate that left-right movement is important
    image
    This is the same mouse cursor used in the Breakdowner operators

  • Keymap info in the status bar at the bottom to indicate to use Esc/RC to exit and Enter/LC to confirm
    image

If this is too much for now then these can be left out for now.
Otherwise I can see if one of the devs can give a helping hand.

Some notes: - 'Toggle Visibility' and 'Hide Active Face Set' still need the `OPTYPE_DEPENDS_ON_CURSOR` flag to be useable. - 'Lasso Add' needs to be set to `Join`. Right now it uses `Difference` by default. The mesh filters work surprisingly well! But they need some additions in the UI. These menu operators should be treated like other modal operators and need visual indications such as: - A visual change in the cursor to indicate that left-right movement is important ![image](/attachments/daf9b557-26a6-496f-92ae-7bb3d18bfda7) This is the same mouse cursor used in the Breakdowner operators - Keymap info in the status bar at the bottom to indicate to use `Esc`/`RC` to exit and `Enter`/`LC` to confirm ![image](/attachments/3c875dac-9884-43a9-8a35-bdae4525ccb4) If this is too much for now then these can be left out for now. Otherwise I can see if one of the devs can give a helping hand.
image.png
3.1 KiB
image.png
1.4 KiB
Julien Kaspar commented 2023-02-14 12:26:39 +01:00
Member
Copy Link

There are also issues where the filter is only partially applied to the mesh, and clicking does not confirm the action.
I can not consistently reproduce this.
But anyway, those operations don't seem to work that well out of the box.

image

There are also issues where the filter is only partially applied to the mesh, and clicking does not confirm the action. I can not consistently reproduce this. But anyway, those operations don't seem to work that well out of the box. ![image](/attachments/cf512dce-5852-400a-9337-53ade19551da)
image.png
427 KiB
Tarek-Yasser commented 2023-02-14 16:49:45 +01:00
Author
Contributor
Copy Link
  • 'Toggle Visibility' and 'Hide Active Face Set' still need the OPTYPE_DEPENDS_ON_CURSOR flag to be useable.

Sorry, I forgot about that. Looking at sculpt_face_set.cc, I assume I have to modify the flags in sculpt_face_sets_change_visibility_exec so that this works on 'Toggle Visibility' and 'Hide Active Face Set' only. Right?

Update: tried it, only adding to SCULPT_OT_face_sets_change_visibility seems to make things work logically, i.e. You choose the option, you get a cursor, you click the face set you want to apply on.

  • 'Lasso Add' needs to be set to Join. Right now it uses Difference by default.

This is weird, I just looked at the last commit and it clearly shows trim_mode = 'JOIN'. I'll look if there's something I forgot to set.

Update: Regarding this, I just tried playing with it for a bit. When building using make lite, it seems like DIFFERENCE acts as JOIN for some reason (performance reasons I assume).

With make developer things work as expected. But Lasso Add/Trim seem to use the last trim mode set by the tool bar.
Steps to reproduce:

  1. Use Lasso Trim/Add. Both should effect the model as if their mode was DIFFERENCE. Order doesn't matter.
  2. Open the Lasso Trim tool from the tool bar, change the mode to Union.
  3. Draw a curve.
  4. Undo the last action.
  5. Use Lasso Trim/Add
  6. The result should be the same as step 3.

Not quite sure if I'm missing something or this is an actual bug...

The mesh filters work surprisingly well! But they need some additions in the UI. These menu operators should be treated like other modal operators and need visual indications such as:

  • A visual change in the cursor to indicate that left-right movement is important
    image
    This is the same mouse cursor used in the Breakdowner operators

  • Keymap info in the status bar at the bottom to indicate to use Esc/RC to exit and Enter/LC to confirm
    image

If this is too much for now then these can be left out for now.
Otherwise I can see if one of the devs can give a helping hand.

I'll try looking for how to do this on my own for a bit. If I can't find anything, I'll ask on blender.chat.

There are also issues where the filter is only partially applied to the mesh, and clicking does not confirm the action.
I can not consistently reproduce this.
But anyway, those operations don't seem to work that well out of the box.

Is there anything I can do on the Python side? I'm not sure if this is an issue with the settings I set or the implementation itself.

> - 'Toggle Visibility' and 'Hide Active Face Set' still need the `OPTYPE_DEPENDS_ON_CURSOR` flag to be useable. Sorry, I forgot about that. ~~Looking at `sculpt_face_set.cc`, I assume I have to modify the flags in `sculpt_face_sets_change_visibility_exec` so that this works on 'Toggle Visibility' and 'Hide Active Face Set' only. Right?~~ Update: tried it, only adding to `SCULPT_OT_face_sets_change_visibility` seems to make things work logically, i.e. You choose the option, you get a cursor, you click the face set you want to apply on. --- > - 'Lasso Add' needs to be set to `Join`. Right now it uses `Difference` by default. This is weird, I just looked at the last commit and it clearly shows `trim_mode = 'JOIN'`. I'll look if there's something I forgot to set. Update: Regarding this, I just tried playing with it for a bit. When building using `make lite`, it seems like `DIFFERENCE` acts as `JOIN` for some reason (performance reasons I assume). With `make developer` things work as expected. But `Lasso Add/Trim` seem to use the last trim mode set by the tool bar. Steps to reproduce: 1. Use Lasso Trim/Add. Both should effect the model as if their mode was `DIFFERENCE`. Order doesn't matter. 2. Open the Lasso Trim tool from the tool bar, change the mode to Union. 3. Draw a curve. 4. Undo the last action. 5. Use Lasso Trim/Add 6. The result should be the same as step 3. Not quite sure if I'm missing something or this is an actual bug... --- > The mesh filters work surprisingly well! But they need some additions in the UI. These menu operators should be treated like other modal operators and need visual indications such as: > - A visual change in the cursor to indicate that left-right movement is important > ![image](/attachments/daf9b557-26a6-496f-92ae-7bb3d18bfda7) > This is the same mouse cursor used in the Breakdowner operators > > - Keymap info in the status bar at the bottom to indicate to use `Esc`/`RC` to exit and `Enter`/`LC` to confirm > ![image](/attachments/3c875dac-9884-43a9-8a35-bdae4525ccb4) > > If this is too much for now then these can be left out for now. > Otherwise I can see if one of the devs can give a helping hand. I'll try looking for how to do this on my own for a bit. If I can't find anything, I'll ask on blender.chat. --- > There are also issues where the filter is only partially applied to the mesh, and clicking does not confirm the action. > I can not consistently reproduce this. > But anyway, those operations don't seem to work that well out of the box. Is there anything I can do on the Python side? I'm not sure if this is an issue with the settings I set or the implementation itself.
Tarek-Yasser added 2 commits 2023-02-15 23:45:42 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
d6a0fccf28 UI: Mesh filters now change the cursor and update the statusbar. 
Mesh filters now use the `WM_CURSOR_EW_SCROLL` and have "LMB: Confirm"
as their statusbar message.
Tarek-Yasser commented 2023-02-16 00:03:19 +01:00
Author
Contributor
Copy Link

So I got the cursor icon displaying and got some text in the status bar. (With the help of PhilippOeser on blender chat).

To get cursor icons in the status bar it seems that I have to call WM_modalkeymap_operator_items_to_string_buf or its macro sibling WM_MODALKEY(_id).

_id is usually an enum member (Look for KNF_MODAL_CONFIRM for an example), which is supposedly hooked up to some keymap somewhere. I tried imitating knifetool_modal_keymap by doing the following:

  1. In spacetypes.c, I call ED_keymap_sculpt inside ED_spacetypes_keymap.
  2. In ED_sculpt.h, I added a new function declaration called ED_keymap_sculpt
  3. In sculpt_ops.cc, I added the body of ED_keymap_sculpt which calls filter_mesh_modal_keymap.
  4. In sculpt_intern.h, I added a declaration for filter_mesh_modal_keymap.
  5. In sculpt_filter_mesh.cc:
    • I added a new enum with FILTER_MESH_MODAL_CANCEL and FILTER_MESH_MODAL_CONFIRM as its elements.
    • I added a the defininition filter_mesh_modal_keymap which does the same thing as knifetool_modal_keymap but with the enum members.
    • In sculpt_mesh_filter_modal, I call BLI_snprintf similarly to how knife_update_header.

I've changed my changes to a branch filter_mesh_modalkeymap in case I'm missing some simple things.

Minor thing: while looking at the status bar thingy, I noticed that WM_MODALKEY is defined in three places with the exact same definition. Perhaps we could pull this out?

Currently, the modal doesn't have cancelling, not sure if I should dive into that or if this is something for another issue? I'd be more than happy to do it. I did try to follow the logic and read The transformation engine's principles page, but I quite quickly lost track of what I'm supposed to be reverting and where its stored.

As for the mesh filter applying only to part of the mesh, I'll try playing around with the filters to figure out what's causing this.

So I got the cursor icon displaying and got some text in the status bar. (With the help of PhilippOeser on blender chat). To get cursor icons in the status bar it seems that I have to call `WM_modalkeymap_operator_items_to_string_buf` or its macro sibling `WM_MODALKEY(_id)`. `_id` is usually an enum member (Look for `KNF_MODAL_CONFIRM` for an example), which is supposedly hooked up to some keymap somewhere. I tried imitating `knifetool_modal_keymap` by doing the following: 1. In `spacetypes.c`, I call `ED_keymap_sculpt` inside `ED_spacetypes_keymap`. 2. In `ED_sculpt.h`, I added a new function declaration called `ED_keymap_sculpt` 3. In `sculpt_ops.cc`, I added the body of `ED_keymap_sculpt` which calls `filter_mesh_modal_keymap`. 4. In `sculpt_intern.h`, I added a declaration for `filter_mesh_modal_keymap`. 5. In `sculpt_filter_mesh.cc`: - I added a new enum with `FILTER_MESH_MODAL_CANCEL` and `FILTER_MESH_MODAL_CONFIRM` as its elements. - I added a the defininition `filter_mesh_modal_keymap` which does the same thing as `knifetool_modal_keymap` but with the enum members. - In `sculpt_mesh_filter_modal`, I call `BLI_snprintf` similarly to how `knife_update_header`. I've changed my changes to a branch `filter_mesh_modalkeymap` in case I'm missing some simple things. Minor thing: while looking at the status bar thingy, I noticed that `WM_MODALKEY` is defined in three places with the exact same definition. Perhaps we could pull this out? Currently, the modal doesn't have cancelling, not sure if I should dive into that or if this is something for another issue? I'd be more than happy to do it. I did try to follow the logic and read The transformation engine's [principles page](https://wiki.blender.org/wiki/Source/Architecture/Transform), but I quite quickly lost track of what I'm supposed to be reverting and where its stored. As for the mesh filter applying only to part of the mesh, I'll try playing around with the filters to figure out what's causing this.
Tarek-Yasser requested review from Julien Kaspar 2023-02-16 00:03:29 +01:00
Julien Kaspar commented 2023-02-16 11:52:57 +01:00
Member
Copy Link

The SCULPT_OT_face_sets_change_visibility menu operators work perfect.

And yes the Trim/Add operators behave very strange. The lasso entries don't have Join or Difference set so it's hard to add them to quick favorites or as shortcuts.
I can also confirm that box trim/add sometimes gets confused and does the opposite.

For the status bar and modal keymap, can you look into the transform operators like Move for reference? They directly use the modal keymap entries for the status bar.

Using RC and Esc for canceling is definitely important.

OPTYPE_DEPENDS_ON_CURSOR should also be used for the mesh filter menu operators as well actually. They also use auto-masking, so the cursor position is important to determine which mesh island/face set is used.

I suggest that if the mesh filters take a lot more time and effort, that you can also split the PR into two, so the others can be merged as soon as they are ready.

Let's see if @JosephEagar can give some help. Otherwise keep asking in blender-coders like before.

The `SCULPT_OT_face_sets_change_visibility` menu operators work perfect. And yes the Trim/Add operators behave very strange. The lasso entries don't have `Join` or `Difference` set so it's hard to add them to quick favorites or as shortcuts. I can also confirm that box trim/add sometimes gets confused and does the opposite. For the status bar and modal keymap, can you look into the transform operators like Move for reference? They directly use the modal keymap entries for the status bar. Using `RC` and `Esc` for canceling is definitely important. `OPTYPE_DEPENDS_ON_CURSOR` should also be used for the mesh filter menu operators as well actually. They also use auto-masking, so the cursor position is important to determine which mesh island/face set is used. I suggest that if the mesh filters take a lot more time and effort, that you can also split the PR into two, so the others can be merged as soon as they are ready. Let's see if @JosephEagar can give some help. Otherwise keep asking in blender-coders like before.
Tarek-Yasser commented 2023-02-17 02:30:51 +01:00
Author
Contributor
Copy Link

And yes the Trim/Add operators behave very strange. The lasso entries don't have Join or Difference set so it's hard to add them to quick favorites or as shortcuts.

Can't quite figure out what you mean by "don't have 'Join' or 'Difference' set". Can you please elaborate?

I can also confirm that box trim/add sometimes gets confused and does the opposite.

Hmm, I didn't quite remember if I tried to break Box Trim/Add. Would this weird behavior warrant a bug report/issue? Would be quite helpful to first know if this is something related to the code I added (mostly UI stuff) or the core operator though.

For the status bar and modal keymap, can you look into the transform operators like Move for reference? They directly use the modal keymap entries for the status bar.

I'll try diving back into this rabbit hole soon.

Using RC and Esc for canceling is definitely important.

Seems like it's not quite trivial to revert changes for mesh filters. I recall being blocked by some parallel task thingy that made me lose track of where's the data. Hopefully once I check it out with a clear mind I'll be able to figure things out.

OPTYPE_DEPENDS_ON_CURSOR should also be used for the mesh filter menu operators as well actually. They also use auto-masking, so the cursor position is important to determine which mesh island/face set is used.

Will do!

I suggest that if the mesh filters take a lot more time and effort, that you can also split the PR into two, so the others can be merged as soon as they are ready.

Well, most of the time is me roaming around in the code or documentation trying to make sense of what goes where. I'll give the mesh filters one more session before splitting them out if I can't get them done.

> And yes the Trim/Add operators behave very strange. The lasso entries don't have `Join` or `Difference` set so it's hard to add them to quick favorites or as shortcuts. Can't quite figure out what you mean by "don't have 'Join' or 'Difference' set". Can you please elaborate? > I can also confirm that box trim/add sometimes gets confused and does the opposite. Hmm, I didn't quite remember if I tried to break Box Trim/Add. Would this weird behavior warrant a bug report/issue? Would be quite helpful to first know if this is something related to the code I added (mostly UI stuff) or the core operator though. > For the status bar and modal keymap, can you look into the transform operators like Move for reference? They directly use the modal keymap entries for the status bar. I'll try diving back into this rabbit hole soon. > Using `RC` and `Esc` for canceling is definitely important. Seems like it's not quite trivial to revert changes for mesh filters. I recall being blocked by some parallel task thingy that made me lose track of where's the data. Hopefully once I check it out with a clear mind I'll be able to figure things out. > `OPTYPE_DEPENDS_ON_CURSOR` should also be used for the mesh filter menu operators as well actually. They also use auto-masking, so the cursor position is important to determine which mesh island/face set is used. Will do! > I suggest that if the mesh filters take a lot more time and effort, that you can also split the PR into two, so the others can be merged as soon as they are ready. Well, most of the time is me roaming around in the code or documentation trying to make sense of what goes where. I'll give the mesh filters one more session before splitting them out if I can't get them done.
Joseph Eagar commented 2023-02-17 06:04:12 +01:00
Member
Copy Link

I've not done a lot with modal tools. The last one was actually the original knife tool 10 years ago.

I've not done a lot with modal tools. The last one was actually the original knife tool 10 years ago.
Joseph Eagar commented 2023-02-17 06:21:14 +01:00
Member
Copy Link

I cleaned up the mesh filter code a bit. I rebased your patch; unfortunately it seems you can't make pull requests of personal repo forks, but here's the branch: https://projects.blender.org/JosephEagar/blender/src/branch/rebase-104718

I cleaned up the mesh filter code a bit. I rebased your patch; unfortunately it seems you can't make pull requests of personal repo forks, but here's the branch: https://projects.blender.org/JosephEagar/blender/src/branch/rebase-104718
Joseph Eagar commented 2023-02-17 06:28:09 +01:00
Member
Copy Link

Don't forget about OPTYPE_DEPENDS_ON_CURSOR. It should fix mesh filter.

Don't forget about OPTYPE_DEPENDS_ON_CURSOR. It should fix mesh filter.
Tarek-Yasser added 3 commits 2023-02-18 13:12:32 +01:00
bfa51d9bd6 UI: Add modal keymap for Mesh Filter + depends on cursor flag. 
Modify `SCULPT_OT_mesh_filter` to use the `OPTYPE_DEPENDS_ON_CURSOR`
flag.
Move filter specific initialization into its own function for
readability.
Tarek-Yasser commented 2023-02-18 13:55:48 +01:00
Author
Contributor
Copy Link

I might've gotten a bit sidetracked :)

So after spending a few hours yesterday, I (think) I got the statusbar almost working?
The current state is that the modal keymap is defined on the python side and hooked up on the C side.
The current status bar for the mesh filter works fine, but for some reason it shows text instead of the icons.

image

I asked around a bit on blender-coders, but no one seemed to have an explanation.
Also, Preferences/Keymap now contains an entry for Mesh Filter Modal Map.

image

The next few sections detail how I implemented the modal keymap, using it in the status bar, setting the mouse cursor, and some bugs? I encountered. I mainly documented these because I couldn't really find this information by searching the dev wiki or the code. Hopefully they're good enough to help others.

For the time being, I'll be moving on to the rest of the original issue unless there's something I can do. I'd also appreciate some feedback if possible.

Adding a modal keymap

expand me for details

Keymaps are used to associate input events (mouse/keyboard/etc) with values in the code that can be used to determine which action was taken.

For example, most operators support Esc/RMB to cancel them. Keymaps help tell Blender that for a certain operator Esc and RMB both map to the action MY_OP_CANCEL.

The code for the operator can then properly handle all actions without having to handle Esc or RMB explicitly. This also gives us the ability to remap MY_OP_CANCEL to whatver key if we so wish without having to modify the operator's code.

The operator only cares about the actions it defined, the mapping between buttons and actions is defined somewhere else.

Here's a high level overview of what you need to add a keymap:

  1. Go to release/scripts/presets/keyconfig/keymap_data/blender_default.py.

  2. Scroll down to a similar keymap to the one you want to use, for example km_sculpt_mesh_filter_modal_map. It should look like this:

    def km_sculpt_mesh_filter_modal_map(_params):
    items = []
    keymap = (
        "Mesh Filter Modal Map",
        {"space_type": 'EMPTY', "region_type": 'WINDOW', "modal": True},
        {"items": items},
    )

    items.extend([
        ("CONFIRM", {"type": 'LEFTMOUSE', "value": 'PRESS', "any": True}, None),
        ("CONFIRM", {"type": 'LEFTMOUSE', "value": 'RELEASE', "any": True}, None),
        ("CONFIRM", {"type": 'RET', "value": 'RELEASE', "any": True}, None),
        ("CONFIRM", {"type": 'NUMPAD_ENTER', "value": 'RELEASE', "any": True}, None),

        ("CANCEL", {"type": 'ESC', "value": 'PRESS', "any": True}, None),
        ("CANCEL", {"type": 'RIGHTMOUSE', "value": 'PRESS', "any": True}, None),
    ])
    return keymap

    The most important things here are the keymap's name Mesh Filter Modal Map and the items.extend() call.
    Try to keep the keymap name consistent in the Python and C code so it's easy to reach either side.
    items.extend() takes the actions you want and registers them as actions for the keymap specified above.

  3. Don't forget to call your function in generate_keymaps_impl!

  4. For your keymaps to show in Preferences/Keymap, you need to add it to keymap_hierarchy.py

    _km_hierarchy = [
   # Existing keymaps
   ('Mesh Filter Modal Map', 'EMPTY', 'WINDOW', []),
]

  5. Go to the operator you want to add a keymap for. You have a function similar to this. If it doesn't exist, you need to implement it.

    wmKeyMap *filter_mesh_modal_keymap(wmKeyConfig *keyconf)
{
  static const EnumPropertyItem modal_items[] = {
      {FILTER_MESH_MODAL_CANCEL, "CANCEL", 0, "Cancel", ""},
      {FILTER_MESH_MODAL_CONFIRM, "CONFIRM", 0, "Confirm", ""},
      {0, NULL, 0, NULL, NULL},
  };

  wmKeyMap *keymap = WM_modalkeymap_find(keyconf, "Mesh Filter Modal Map");

  /* This function is called for each space-type, only needs to add map once. */
  if (keymap && keymap->modal_items) {
    return NULL;
  }

  keymap = WM_modalkeymap_ensure(keyconf, "Mesh Filter Modal Map", modal_items);

  WM_modalkeymap_assign(keymap, "SCULPT_OT_mesh_filter");

  return keymap;
}

    Notice how "CONFIRM" and "CANCEL" on the Python and C side seem to link an action to its keys.
    For example:

    • On the python side, you have a "CONFIRM" action, which maps to the left mouse button.
    • On the C side, you have a FILTER_MESH_MODAL_CONFIRM action, which maps to a "CONFIRM" action.

    So whenever you left click while using the modal filter mesh, whatever code handles FILTER_MESH_MODAL_CONFIRM will run.

    Note that you'll have to define an enum for the actions so you can handle them in C code later on:

    enum {
  FILTER_MESH_MODAL_CANCEL = 1,
  FILTER_MESH_MODAL_CONFIRM,
};

  6. Add the modal keymap function to the appropriate header file.
    For filter_mesh_modal_keymap, it can be added in sculpt_intern.hh, just under the declaration for its operator function:

    void SCULPT_OT_mesh_filter(wmOperatorType *ot);
struct wmKeyMap *filter_mesh_modal_keymap(struct wmKeyConfig *keyconf);

  7. The C modal map function doesn't get magically called, you need to call it somewhere.

    For example filter_mesh_modal_keymap gets called in ED_keymap_sculpt.
    Similar functions are usually in a file called x_ops.cc. For sculpting operators, you can find this function in sculpt_ops.cc. If it doesn't exist, you can create it as follows:

    void ED_keymap_sculpt(wmKeyConfig *keyconf)
{
	filter_mesh_modal_keymap(keyconf);
}

    You'll probably have to add the function's declartion in a header file somehere. For the previous function, you can add its declaration in ED_sculpt.h

  8. ED_keymap_sculpt doesn't also get magically called, it's itself called in ED_spacetypes_keymap

Once you have your op_modal_keymap or op_keymap, you should be able to see your keymap in Preferences/Keymap and use it in status bar messages!

Needs further clarification:

keymap = (
    "Mesh Filter Modal Map",
    {"space_type": 'EMPTY', "region_type": 'WINDOW', "modal": True},
    {"items": items},
)

What does "space_type" : 'EMPTY' mean here? What about "region_type"? I couldn't find much information about what this dict's fields mean and what their options are.

("CANCEL", {"type": 'ESC', "value": 'PRESS', "any": True}, None),
  • "CANCEL" is what links Python's definitions to C actions?
  • The second dict determines the key to press?, what action (press / release)? No idea what "any" is for.
  • What's the None for?
    {FILTER_MESH_MODAL_CANCEL, "CANCEL", 0, "Cancel", ""},
  • FILTER_MESH_MODAL_CANCEL is the action that might mapped to multiple keys
  • "CANCEL" is some string that's shared between Python and C
  • Whats the 0 for?
  • The other "Cancel" is for GUI?
  • "" What's this for?

How to use modal keymaps in status bar updates?

Here's also some info I gathered about how to use a keymap for statusbar upates:

expand me for details For modal keymaps, you just need to call a function like this one inside your modal callback. 
static void my_operator_update_status_bar(bContext* C, wmOperator* op) {
	char header[UI_MAX_DRAW_STR];
	char buf[UI_MAX_DRAW_STR];
	int available_len = sizeof(buf);

	char* p = buf;
#define WM_MODALKEY(_id) \
	WM_modalkeymap_operator_items_to_string_buf( \
			op->type, (_id), true, UI_MAX_SHORTCUT_STR, &available_len, &p)

	BLI_snprintf(
			header,
			sizeof(header),
			TIP_("%s: Confirm, %s: Cancel"),
			WM_MODALKEY(MY_OPERATOR_MODAL_CONFIRM),
			WM_MODALKEY(MY_OPERATOR_MODAL_CANCEL)
	);

#undef WM_MODALKEY
	ED_workspace_status_text(C, TIP_(header));
}

Don't forget to clear the statusbar by calling ED_workspace_status_text(C, NULL) once your operator is done.

Changing the mouse cursor for modals

Just call WM_cursor_modal_set(CTX_wm_window(C), WM_CURSOR_EW_SCROLL) for example in your modal callback.
Don't forget to call WM_cursor_modal_restore(CTX_wm_window(C)) once the operator finishes!

You can find the different types of cursors you can use in wm_cursors.h

Cancelling mesh filters

Regarding cancelling mesh filters, I spent a fair bit of time trying to understand how the transform tool cancells its operators.
Here's what I understood:

  • Most of the undoing logic is actually in restoreTransObjects (transform.c lines 925-937 iirc)
    • Which is called inside transformEvent.
    • How did I reach this conclusion? I commented the parts I suspected until cancelling a transformation didn't work.
    • From what I understand, the original transform of the object is saved, so restoreTransObjects copies that data back.
  • the cancel callback (transform_cancel) doesn't do anything special? I think it mostly does cleanups?

So, to apply this to mesh filters, one would have to:

  1. Figure out exactly what data is changed during the operators operation,
  2. Store that data somewhere (op->customdata?),
  3. On cancellation, copy that data back.

I'm not 100% sure that this is completely applicable to mesh filters yet, but it might be a good starting point.
At this point, I think the cancel functionality should be split into its own issue, unless a more experienced developer deems it easy enough to be bundled into this issue.

Bugs?

  • After using a mesh filter, upon trying to use it again this assert fails:

    BLI_assert failed: source/blender/blenkernel/intern/undo_system.cc:451, BKE_undosys_step_push_init_with_type(), at 'ustack->step_init == nullptr'

    I'm not quite sure what's the cause of this. I read the usage guide at the top of sculpt_undo.cc and confirmed that SCULPT_undo_push_begin is only called in invoke and SCULPT_undo_push_end is only called once at the end of the operator (confrimation or cancellation)
    I did try to only use in when the operator is confirmed or cancelled only in case I understood the usage guide wrong, but it seems to crash still.

  • There seems to be a memory leak related to cancelling mesh filter operators:
    What's weird here is that this only occurs when the operator is cancelled. Confirming the operator doesn't seem to leak memory.
    What's even weirder is that when confirming, no deallocation logic is specific to confirming or cancelling, both do the same cleanup logic.
    What's the most weird is that the only following filter types have specific initializations that allocate memory. The others don't:

    • MESH_FILTER_SURFACE_SMOOTH
    • MESH_FILTER_SHARPEN
    • MESH_FILTER_ENHANCE_DETAILS
    • MESH_FILTER_ERASE_DISPLACEMENT

    Here are the steps to reproduce:

    1. Build a debug version of blender,
    2. Launch blender, go to the sculpt tab,
    3. Select any mesh filter operator,
    4. Apply the filter, then quit. Here you shouldn't see any errors of leaking memory.
    5. Redo steps 2, 3, and 4. But cancel the operator instead of confirming. You should see an error mentioning un-freed blocks.
I might've gotten a bit sidetracked :) So after spending a few hours yesterday, I (think) I got the statusbar almost working? The current state is that the modal keymap is defined on the python side and hooked up on the C side. The current status bar for the mesh filter works fine, but for some reason it shows text instead of the icons. ![image](/attachments/6c2b27c1-314d-44c9-aa4a-dfa8f08f87d4) I asked around a bit on `blender-coders`, but no one seemed to have an explanation. Also, `Preferences/Keymap` now contains an entry for `Mesh Filter Modal Map`. ![image](/attachments/87442db2-9a3d-48a5-a170-59b39c1f409a) The next few sections detail how I implemented the modal keymap, using it in the status bar, setting the mouse cursor, and some bugs? I encountered. I mainly documented these because I couldn't really find this information by searching the dev wiki or the code. Hopefully they're good enough to help others. For the time being, I'll be moving on to the rest of the original issue unless there's something I can do. I'd also appreciate some feedback if possible. # Adding a modal keymap <details> <summary> expand me for details </summary> Keymaps are used to associate input events (mouse/keyboard/etc) with values in the code that can be used to determine which action was taken. For example, most operators support `Esc/RMB` to cancel them. Keymaps help tell Blender that for a certain operator `Esc` and `RMB` both map to the action `MY_OP_CANCEL`. The code for the operator can then properly handle all actions without having to handle `Esc` or `RMB` explicitly. This also gives us the ability to remap `MY_OP_CANCEL` to whatver key if we so wish without having to modify the operator's code. The operator only cares about the actions it defined, the mapping between buttons and actions is defined somewhere else. Here's a high level overview of what you need to add a keymap: 1. Go to `release/scripts/presets/keyconfig/keymap_data/blender_default.py`. 2. Scroll down to a similar keymap to the one you want to use, for example `km_sculpt_mesh_filter_modal_map`. It should look like this: ```python def km_sculpt_mesh_filter_modal_map(_params): items = [] keymap = ( "Mesh Filter Modal Map", {"space_type": 'EMPTY', "region_type": 'WINDOW', "modal": True}, {"items": items}, ) items.extend([ ("CONFIRM", {"type": 'LEFTMOUSE', "value": 'PRESS', "any": True}, None), ("CONFIRM", {"type": 'LEFTMOUSE', "value": 'RELEASE', "any": True}, None), ("CONFIRM", {"type": 'RET', "value": 'RELEASE', "any": True}, None), ("CONFIRM", {"type": 'NUMPAD_ENTER', "value": 'RELEASE', "any": True}, None), ("CANCEL", {"type": 'ESC', "value": 'PRESS', "any": True}, None), ("CANCEL", {"type": 'RIGHTMOUSE', "value": 'PRESS', "any": True}, None), ]) return keymap ``` The most important things here are the keymap's name `Mesh Filter Modal Map` and the `items.extend()` call. Try to keep the keymap name consistent in the Python and C code so it's easy to reach either side. `items.extend()` takes the actions you want and registers them as actions for the keymap specified above. 3. Don't forget to call your function in `generate_keymaps_impl`! 4. For your keymaps to show in `Preferences/Keymap`, you need to add it to `keymap_hierarchy.py` ```python _km_hierarchy = [ # Existing keymaps ('Mesh Filter Modal Map', 'EMPTY', 'WINDOW', []), ] ``` 5. Go to the operator you want to add a keymap for. You have a function similar to this. If it doesn't exist, you need to implement it. ```C wmKeyMap *filter_mesh_modal_keymap(wmKeyConfig *keyconf) { static const EnumPropertyItem modal_items[] = { {FILTER_MESH_MODAL_CANCEL, "CANCEL", 0, "Cancel", ""}, {FILTER_MESH_MODAL_CONFIRM, "CONFIRM", 0, "Confirm", ""}, {0, NULL, 0, NULL, NULL}, }; wmKeyMap *keymap = WM_modalkeymap_find(keyconf, "Mesh Filter Modal Map"); /* This function is called for each space-type, only needs to add map once. */ if (keymap && keymap->modal_items) { return NULL; } keymap = WM_modalkeymap_ensure(keyconf, "Mesh Filter Modal Map", modal_items); WM_modalkeymap_assign(keymap, "SCULPT_OT_mesh_filter"); return keymap; } ``` Notice how "CONFIRM" and "CANCEL" on the Python and C side seem to link an action to its keys. For example: - On the python side, you have a "CONFIRM" action, which maps to the left mouse button. - On the C side, you have a `FILTER_MESH_MODAL_CONFIRM` action, which maps to a "CONFIRM" action. So whenever you left click while using the modal filter mesh, whatever code handles `FILTER_MESH_MODAL_CONFIRM` will run. Note that you'll have to define an enum for the actions so you can handle them in C code later on: ```C enum { FILTER_MESH_MODAL_CANCEL = 1, FILTER_MESH_MODAL_CONFIRM, }; ``` 6. Add the modal keymap function to the appropriate header file. For `filter_mesh_modal_keymap`, it can be added in `sculpt_intern.hh`, just under the declaration for its operator function: ```C void SCULPT_OT_mesh_filter(wmOperatorType *ot); struct wmKeyMap *filter_mesh_modal_keymap(struct wmKeyConfig *keyconf); ``` 7. The C modal map function doesn't get magically called, you need to call it somewhere. For example `filter_mesh_modal_keymap` gets called in `ED_keymap_sculpt`. Similar functions are usually in a file called `x_ops.cc`. For sculpting operators, you can find this function in `sculpt_ops.cc`. If it doesn't exist, you can create it as follows: ```C void ED_keymap_sculpt(wmKeyConfig *keyconf) { filter_mesh_modal_keymap(keyconf); } ``` You'll probably have to add the function's declartion in a header file somehere. For the previous function, you can add its declaration in `ED_sculpt.h` 8. `ED_keymap_sculpt` doesn't also get magically called, it's itself called in `ED_spacetypes_keymap` Once you have your `op_modal_keymap` or `op_keymap`, you should be able to see your keymap in `Preferences/Keymap` and use it in status bar messages! ## Needs further clarification: ```python keymap = ( "Mesh Filter Modal Map", {"space_type": 'EMPTY', "region_type": 'WINDOW', "modal": True}, {"items": items}, ) ``` What does `"space_type" : 'EMPTY'` mean here? What about "region_type"? I couldn't find much information about what this dict's fields mean and what their options are. --- ```python ("CANCEL", {"type": 'ESC', "value": 'PRESS', "any": True}, None), ``` - "CANCEL" is what links Python's definitions to C actions? - The second dict determines the key to press?, what action (press / release)? No idea what "any" is for. - What's the `None` for? --- ```C {FILTER_MESH_MODAL_CANCEL, "CANCEL", 0, "Cancel", ""}, ``` - `FILTER_MESH_MODAL_CANCEL` is the action that might mapped to multiple keys - `"CANCEL"` is some string that's shared between Python and C - Whats the `0` for? - The other "Cancel" is for GUI? - `""` What's this for? </details> # How to use modal keymaps in status bar updates? Here's also some info I gathered about how to use a keymap for statusbar upates: <details> <summary> expand me for details </summary> For modal keymaps, you just need to call a function like this one inside your modal callback. ```C static void my_operator_update_status_bar(bContext* C, wmOperator* op) { char header[UI_MAX_DRAW_STR]; char buf[UI_MAX_DRAW_STR]; int available_len = sizeof(buf); char* p = buf; #define WM_MODALKEY(_id) \ WM_modalkeymap_operator_items_to_string_buf( \ op->type, (_id), true, UI_MAX_SHORTCUT_STR, &available_len, &p) BLI_snprintf( header, sizeof(header), TIP_("%s: Confirm, %s: Cancel"), WM_MODALKEY(MY_OPERATOR_MODAL_CONFIRM), WM_MODALKEY(MY_OPERATOR_MODAL_CANCEL) ); #undef WM_MODALKEY ED_workspace_status_text(C, TIP_(header)); } ``` Don't forget to clear the statusbar by calling `ED_workspace_status_text(C, NULL)` once your operator is done. </details> # Changing the mouse cursor for modals Just call `WM_cursor_modal_set(CTX_wm_window(C), WM_CURSOR_EW_SCROLL)` for example in your modal callback. Don't forget to call `WM_cursor_modal_restore(CTX_wm_window(C))` once the operator finishes! You can find the different types of cursors you can use in `wm_cursors.h` # Cancelling mesh filters Regarding cancelling mesh filters, I spent a fair bit of time trying to understand how the transform tool cancells its operators. Here's what I understood: - Most of the undoing logic is actually in `restoreTransObjects` (`transform.c` lines 925-937 iirc) - Which is called inside `transformEvent`. - How did I reach this conclusion? I commented the parts I suspected until cancelling a transformation didn't work. - From what I understand, the original transform of the object is saved, so `restoreTransObjects` copies that data back. - the cancel callback (`transform_cancel`) doesn't do anything special? I think it mostly does cleanups? So, to apply this to mesh filters, one would have to: 1. Figure out exactly what data is changed during the operators operation, 2. Store that data somewhere (op->customdata?), 3. On cancellation, copy that data back. I'm not 100% sure that this is completely applicable to mesh filters yet, but it might be a good starting point. At this point, I think the cancel functionality should be split into its own issue, unless a more experienced developer deems it easy enough to be bundled into this issue. # Bugs? - After using a mesh filter, upon trying to use it again this assert fails: ``` BLI_assert failed: source/blender/blenkernel/intern/undo_system.cc:451, BKE_undosys_step_push_init_with_type(), at 'ustack->step_init == nullptr' ``` I'm not quite sure what's the cause of this. I read the usage guide at the top of `sculpt_undo.cc` and confirmed that `SCULPT_undo_push_begin` is only called in `invoke` and `SCULPT_undo_push_end` is only called once at the end of the operator (confrimation or cancellation) I did try to only use in when the operator is confirmed or cancelled only in case I understood the usage guide wrong, but it seems to crash still. - There seems to be a memory leak related to cancelling mesh filter operators: What's weird here is that this only occurs when the operator is cancelled. Confirming the operator doesn't seem to leak memory. What's even weirder is that when confirming, no deallocation logic is specific to confirming or cancelling, both do the same cleanup logic. What's the most weird is that the only following filter types have specific initializations that allocate memory. The others don't: * MESH_FILTER_SURFACE_SMOOTH * MESH_FILTER_SHARPEN * MESH_FILTER_ENHANCE_DETAILS * MESH_FILTER_ERASE_DISPLACEMENT Here are the steps to reproduce: 1. Build a debug version of blender, 2. Launch blender, go to the sculpt tab, 3. Select any mesh filter operator, 4. Apply the filter, then quit. Here you shouldn't see any errors of leaking memory. 5. Redo steps 2, 3, and 4. But cancel the operator instead of confirming. You should see an error mentioning un-freed blocks.
image.png
23 KiB
image.png
3.2 KiB
Joseph Eagar commented 2023-02-21 03:23:23 +01:00
Member
Copy Link

I implemented a cancel function for mesh filter in master, sculpt_mesh_filter_cancel (it's not used yet since mesh filter's modal map currently lacks cancel). Note that you still have to call sculpt_mesh_filter_end.

I implemented a cancel function for mesh filter in master, sculpt_mesh_filter_cancel (it's not used yet since mesh filter's modal map currently lacks cancel). Note that you still have to call sculpt_mesh_filter_end.
Joseph Eagar commented 2023-02-21 03:23:47 +01:00
Member
Copy Link

I implemented a cancel function for mesh filter in master, sculpt_mesh_filter_cancel (it's not used yet since mesh filter's modal map currently lacks cancel). Note that you still have to call sculpt_mesh_filter_end.

> I implemented a cancel function for mesh filter in master, sculpt_mesh_filter_cancel (it's not used yet since mesh filter's modal map currently lacks cancel). Note that you still have to call sculpt_mesh_filter_end. > >
Joseph Eagar closed this pull request 2023-02-21 03:23:48 +01:00
Joseph Eagar reopened this pull request 2023-02-21 03:23:55 +01:00
Joseph Eagar commented 2023-02-21 03:24:16 +01:00
Member
Copy Link

Okay that "close" button looks awfully like a "cancel" button for comments.

Okay that "close" button looks awfully like a "cancel" button for comments.
Tarek-Yasser commented 2023-02-21 09:14:57 +01:00
Author
Contributor
Copy Link

I implemented a cancel function for mesh filter in master, sculpt_mesh_filter_cancel (it's not used yet since mesh filter's modal map currently lacks cancel). Note that you still have to call sculpt_mesh_filter_end.

Thanks a bunch! I'll integrate that as soon as possible.

> I implemented a cancel function for mesh filter in master, sculpt_mesh_filter_cancel (it's not used yet since mesh filter's modal map currently lacks cancel). Note that you still have to call sculpt_mesh_filter_end. Thanks a bunch! I'll integrate that as soon as possible.
Erik Abrahamsson commented 2023-02-21 12:00:33 +01:00
Member
Copy Link

I implemented a modal keymap for linking nodes a few weeks ago, see dc79281223

I implemented a modal keymap for linking nodes a few weeks ago, see https://projects.blender.org/blender/blender/commit/dc79281223cdc92d45f34133b3601cf1a20163d3
Tarek-Yasser commented 2023-02-21 22:05:04 +01:00
Author
Contributor
Copy Link

I implemented a modal keymap for linking nodes a few weeks ago, see dc79281223

Thanks for the suggestion! I checked out your commit and I think I implemented something similar in bfa51d9bd6. I think the main issue here is that there's no documentation for this, or it's not quite acessible. Perhaps the big comment I posted a few days ago can be added to the dev wiki or something (after being iterated upon, of course.)?

> I implemented a modal keymap for linking nodes a few weeks ago, see https://projects.blender.org/blender/blender/commit/dc79281223cdc92d45f34133b3601cf1a20163d3 Thanks for the suggestion! I checked out your commit and I think I implemented something similar in bfa51d9bd6. I think the main issue here is that there's no documentation for this, or it's not quite acessible. Perhaps the big comment I posted a few days ago can be added to the dev wiki or something (after being iterated upon, of course.)?
Tarek-Yasser added 2 commits 2023-02-22 02:11:19 +01:00
Tarek-Yasser commented 2023-02-22 02:17:04 +01:00
Author
Contributor
Copy Link

@JulienKaspar
I think the only thing left for this PR is the aforementioned bugs:

  • Lasso/Box Add/Trim sometimes get confused and don't work properly (trim adds or vice versa).
  • Partial application of mesh filters, clicking doesn't seem to confirm.
  • Memory leaks when using mesh filters (not sure about other stuff), not sure if they already existed or are introduced by me. I'll try checking out an older commit and seeing if memory leaks there. I haven't dove super deep so hopefully it's nothing serious.
  • Exposing Sphere
  • Greying out Erase Displacement if no Multires (Can't quite understand what's Multires, but hopefully I can pick up on it once I look around).
    • Someone suggested that greying out can be done by modifying the return of poll().
@JulienKaspar I think the only thing left for this PR is the aforementioned bugs: - Lasso/Box Add/Trim sometimes get confused and don't work properly (trim adds or vice versa). - Partial application of mesh filters, clicking doesn't seem to confirm. - Memory leaks when using mesh filters (not sure about other stuff), not sure if they already existed or are introduced by me. I'll try checking out an older commit and seeing if memory leaks there. I haven't dove super deep so hopefully it's nothing serious. - Exposing `Sphere` - Greying out `Erase Displacement` if no Multires (Can't quite understand what's `Multires`, but hopefully I can pick up on it once I look around). - Someone suggested that greying out can be done by modifying the return of `poll()`.
Tarek-Yasser added 1 commit 2023-02-22 02:23:06 +01:00
Julien Kaspar commented 2023-02-22 12:45:03 +01:00
Member
Copy Link

@Tarek-Yasser Good to hear!
Sorry for being unclear on Erase Displacement. It's used when the object has a Multiresolution modiifer with subdivision levels.
So in the case that no Multires modifier is used for sculpt mode on the active object, the operator could just as well be grayed out, since it's not doing anything.

@Tarek-Yasser Good to hear! Sorry for being unclear on `Erase Displacement`. It's used when the object has a Multiresolution modiifer with subdivision levels. So in the case that no Multires modifier is used for sculpt mode on the active object, the operator could just as well be grayed out, since it's not doing anything.
Tarek-Yasser commented 2023-02-25 20:28:36 +01:00
Author
Contributor
Copy Link

So in the case that no Multires modifier is used for sculpt mode on the active object, the operator could just as well be grayed out, since it's not doing anything.

Just spent a few hours looking into this. I toyed a bit with poll() and it indeed does disable the operator's entry in the menu. The main issue I'm facing is knowing which entry I'm in.
For example, when I use a static variable to alternate enabling and disabling entries, I get this pattern:
image

Using this snippet, I was able to enable or disable the hole Filter Mesh section when a multires modifier was added with a sculpt subdiv > 0:

Object *ob = CTX_data_active_object(C);
bool multires_with_subdivision = false;
MultiresModifierData * md = (MultiresModifierData*)BKE_modifiers_findby_type(ob, eModifierType_Multires);

if (md && md->sculptlvl > 0) {
multires_with_subdivision = true; 
}

return SCULPT_mode_poll(C) && !multires_with_subdivision;

Here's how it affects the menu:
image

I'm pretty much out of ideas now. The other operators seem to not be grouped by like Filter Mesh is so I'm not sure if there's a way to do this without splitting the operator up.

Also, I'd like confirmation whether or not checking sculptlvl only is correct, there are other "lvl"s but I'm not sure if this menu cares about them.

@JosephEagar thoughts?

> So in the case that no Multires modifier is used for sculpt mode on the active object, the operator could just as well be grayed out, since it's not doing anything. Just spent a few hours looking into this. I toyed a bit with `poll()` and it indeed does disable the operator's entry in the menu. The main issue I'm facing is knowing which entry I'm in. For example, when I use a static variable to alternate enabling and disabling entries, I get this pattern: ![image](/attachments/1581a8b7-f147-4207-ba83-c8f00cee4aa0) Using this snippet, I was able to enable or disable the hole Filter Mesh section when a multires modifier was added with a sculpt subdiv > 0: ```C Object *ob = CTX_data_active_object(C); bool multires_with_subdivision = false; MultiresModifierData * md = (MultiresModifierData*)BKE_modifiers_findby_type(ob, eModifierType_Multires); if (md && md->sculptlvl > 0) { multires_with_subdivision = true; } return SCULPT_mode_poll(C) && !multires_with_subdivision; ``` Here's how it affects the menu: ![image](/attachments/b9ad1616-c104-4982-91ab-9a6bf2968f69) I'm pretty much out of ideas now. The other operators seem to not be grouped by like Filter Mesh is so I'm not sure if there's a way to do this without splitting the operator up. Also, I'd like confirmation whether or not checking `sculptlvl` only is correct, there are other "lvl"s but I'm not sure if this menu cares about them. @JosephEagar thoughts?
image.png
11 KiB
image.png
208 KiB
Tarek-Yasser added 3 commits 2023-02-26 22:49:17 +01:00
115ec325f2 UI: Expose Sphere operator 
Also setup `poll()` for greying out `Erase Multires Displacement`.
Tarek-Yasser commented 2023-02-26 22:49:51 +01:00
Author
Contributor
Copy Link

Ok, I think I'm done with this PR, It's getting a teeny tiny too big.

I've spent the last couple of days investigating the memory leak and the greying out of Erase Displacement to no avail so I suggest that the remaining stuff may be spun into their own issues. I'll continue working on the main issue.

Here are the things that I think deserve an issue that are related to this PR:

  • Lasso/Box Add/Trim getting confused.
    Possbily a bug in the implementation? The Python code exposing the operator in the UI just sets the operation type so I'm not sure why it's bugged.
  • Partial application of mesh filters
    Not easily replicable, but might be worth an issue just to figure out how to reproduce it.
  • Memory Leaks when cancelling mesh filters
    Tried compiling with the current main branch and the parent branch of this PR's branch, both don't seem to have this issue which points me towards the operator cancelling code. compiled a dev debug build and got some address sanitizer output attached below. Hope it helps.
  • Greying out Erase Displacement if no Multires
    Checking in poll() works fine, but I didn't find a way to check which operator we're polling for given only the context. I'll revert the code but my trial can be found at this commit 115ec325f2

@JulienKaspar @JosephEagar Please review and tell me if there's anything else I can do.

Ok, I think I'm done with this PR, It's getting a teeny tiny too big. I've spent the last couple of days investigating the memory leak and the greying out of Erase Displacement to no avail so I suggest that the remaining stuff may be spun into their own issues. I'll continue working on the main issue. Here are the things that I think deserve an issue that are related to this PR: - Lasso/Box Add/Trim getting confused. Possbily a bug in the implementation? The Python code exposing the operator in the UI just sets the operation type so I'm not sure why it's bugged. - Partial application of mesh filters Not easily replicable, but might be worth an issue just to figure out how to reproduce it. - Memory Leaks when cancelling mesh filters Tried compiling with the current main branch and the parent branch of this PR's branch, both don't seem to have this issue which points me towards the operator cancelling code. compiled a dev debug build and got some address sanitizer output attached below. Hope it helps. - Greying out Erase Displacement if no Multires Checking in `poll()` works fine, but I didn't find a way to check which operator we're polling for given only the context. I'll revert the code but my trial can be found at this commit 115ec325f2b7a3991a2ea171d2eafe392a2fc924 @JulienKaspar @JosephEagar Please review and tell me if there's anything else I can do.
address-sanitizer-output.txt
24 KiB
Joseph Eagar commented 2023-02-27 12:07:24 +01:00
Member
Copy Link

The nodes out parameters from BKE_pbvh_search_gather needs to be freed, MEM_SAFE_FREE(nodes), that was probably my fault. Not sure what you mean by partial application of mesh filters, I'll take a look.

Let's ignore multires for now; I need to figure out how to best expose the sculpt data backing type to python (multires is the PBVH_GRIDS type).

The CONFIRM modal item needs a RELEASE entry for LEFTMOUSE.

I've marked a few nitpicks (C++ files should use nullptr instead of NULL, and // comments are not allowed).

The `nodes` out parameters from `BKE_pbvh_search_gather` needs to be freed, `MEM_SAFE_FREE(nodes)`, that was probably my fault. Not sure what you mean by partial application of mesh filters, I'll take a look. Let's ignore multires for now; I need to figure out how to best expose the sculpt data backing type to python (multires is the PBVH_GRIDS type). The CONFIRM modal item needs a RELEASE entry for LEFTMOUSE. I've marked a few nitpicks (C++ files should use `nullptr` instead of `NULL`, and `//` comments are not allowed).
👍 2
Tarek-Yasser added 1 commit 2023-02-28 19:36:06 +01:00
76a7387276 Cleanup: Fix memory leaking when cancelling Filter Mesh operators. 
Seems like the memory leak was related to `SCULPT_undo_push_end` not
being recursive. Using `SCULPT_undo_push_end_ex` and passing
`use_nested_undo` as true fixes the leak.

Replace line comments (//) with block comments (/**/).
Add a `RELEASE` entry for `LEFTMOUSE` for the filter mesh keymap.
Replace `NULL` with `nullptr`.
Tarek-Yasser added 1 commit 2023-02-28 20:17:25 +01:00
a685beabb8 Fix: Undo not working properly with Mesh Filter 
The previous behaviour was that if you applied a mesh filter and
modified its strength for example through the redo last operator, it
would additively add the new update on top of the existing one. It would
also add an undo step.

This was due to calling `SCULPT_undo_push_end_ex(ob, true)` on both
operator confirmation and cancellation, where it was only needed on
cancellation.
Tarek-Yasser commented 2023-02-28 20:23:00 +01:00
Author
Contributor
Copy Link

@JosephEagar
The notes you mentioned should (hopefully) be addressed now.

For the memory leak, I did try freeing nodes, but the unfreed blocks message still showed up.

I inspected the address sanitizer output a bit more and noticed that the biggest chunks that leaked were related to the undo stack. I did some debugging and got the idea to check where we push/pop and noticed that SCULPT_undo_push_end used SCULPT_undo_push_end_ex with use_nested_undo set to false. Changing it to true seems to have fixed the leak. I did my usual memory leak test by running Randomize and cancelling it, and no leak was reported! I also tried a couple of other Filter Mesh options and they also didn't leak.

Speaking of the undo system, this change introduced a small bug related to undo that I explained and fixed in this commit

@JosephEagar The notes you mentioned should (hopefully) be addressed now. For the memory leak, I did try freeing `nodes`, but the unfreed blocks message still showed up. I inspected the address sanitizer output a bit more and noticed that the biggest chunks that leaked were related to the undo stack. I did some debugging and got the idea to check where we push/pop and noticed that `SCULPT_undo_push_end` used `SCULPT_undo_push_end_ex` with `use_nested_undo` set to false. Changing it to true seems to have fixed the leak. I did my usual memory leak test by running `Randomize` and cancelling it, and no leak was reported! I also tried a couple of other Filter Mesh options and they also didn't leak. Speaking of the undo system, this change introduced a small bug related to undo that I explained and fixed in [this commit](https://projects.blender.org/Tarek-Yasser/blender/commit/a685beabb8947bc2816777b23b34f22af13dca18)
Hans Goudey changed title from WIP: #102427 Sculpt Mode Add exisiting Tools as Menu Operators to WIP: Sculpt:Add exisiting tools as menu operators 2023-02-28 21:10:48 +01:00
Hans Goudey changed title from WIP: Sculpt:Add exisiting tools as menu operators to WIP: Sculpt: Add exisiting tools as menu operators 2023-02-28 21:10:55 +01:00
Julien Kaspar added the
Module
Sculpt, Paint & Texture
 label 2023-03-01 10:59:25 +01:00
Tarek-Yasser added 1 commit 2023-03-03 17:21:09 +01:00
c72cd77ebe Cleanup: Replace some `NULL`s with `nullptr`. 
Also, use `OPERATOR_FINISHED` as a default for modal events instead of
`FILTER_MESH_MODAL_CONFIRM`.
Tarek-Yasser changed title from WIP: Sculpt: Add exisiting tools as menu operators to Sculpt: Add Transform, Trim, and Mesh Filter operators to Sculpt menu 2023-03-03 17:44:23 +01:00
Tarek-Yasser added 1 commit 2023-03-03 17:56:49 +01:00
Joseph Eagar approved these changes 2023-03-08 01:16:09 +01:00
source/blender/editors/sculpt_paint/sculpt_filter_mesh.cc
@ -704,0 +717,4 @@
static const EnumPropertyItem modal_items[] = {
{FILTER_MESH_MODAL_CANCEL, "CANCEL", 0, "Cancel", ""},
{FILTER_MESH_MODAL_CONFIRM, "CONFIRM", 0, "Confirm", ""},
{0, NULL, 0, NULL, NULL},
Joseph Eagar commented 2023-02-27 11:51:53 +01:00
Member
Copy Link

NULL -> nullptr

NULL -> nullptr
source/blender/editors/sculpt_paint/sculpt_filter_mesh.cc
@ -860,2 +873,3 @@
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO | OPTYPE_GRAB_CURSOR_X | OPTYPE_BLOCKING;
Joseph Eagar commented 2023-02-17 06:28:39 +01:00
Member
Copy Link

Add OPTYPE_DEPENDS_ON_CURSOR too.

Add OPTYPE_DEPENDS_ON_CURSOR too.
source/blender/editors/sculpt_paint/sculpt_filter_mesh.cc
@ -1084,3 +1172,3 @@
ot->ui = sculpt_mesh_ui_exec;
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
// Doesn't seem to actually be called?
Joseph Eagar commented 2023-02-27 11:51:08 +01:00
Member
Copy Link

Don't use // comments.

Don't use // comments.
Joseph Eagar merged commit da65b21e2e into main 2023-03-08 01:18:33 +01:00
Carlos commented 2023-03-08 08:25:02 +01:00
First-time contributor
Copy Link

Hi
Lasso trim and lasso add are the same, both are trimming, are the same operator and mode.
I didn't know if I should put it here or sould I open an issue.

Hi Lasso trim and lasso add are the same, both are trimming, are the same operator and mode. I didn't know if I should put it here or sould I open an issue.
2023-03-08 01-12-26.mp4
2.7 MiB
Tarek-Yasser commented 2023-03-08 17:06:15 +01:00
Author
Contributor
Copy Link

Hi
Lasso trim and lasso add are the same, both are trimming, are the same operator and mode.
I didn't know if I should put it here or sould I open an issue.

Hello, I think I already reported this in an earlier comment, also #105557 seems to mention this. Sorry for the inconvinience and hopefully it'll be fixed soon.

> Hi > Lasso trim and lasso add are the same, both are trimming, are the same operator and mode. > I didn't know if I should put it here or sould I open an issue. Hello, I think I already reported this in an [earlier comment](https://projects.blender.org/blender/blender/pulls/104718#issuecomment-883218), also #105557 seems to mention this. Sorry for the inconvinience and hopefully it'll be fixed soon.
❤️ 1
Julien Kaspar commented 2023-03-15 11:40:58 +01:00
Member
Copy Link

I'm unavailable for this month, but I'll see if I can review this towards the end of the week.

I'm unavailable for this month, but I'll see if I can review this towards the end of the week.
Julien Kaspar commented 2023-03-17 19:54:53 +01:00
Member
Copy Link

Ah I see this was already committed :/
I tested it a bit more in main. There are still a couple of bugs that I noticed but overall it works great!
Thanks @Tarek-Yasser @JosephEagar for your work!

Ah I see this was already committed :/ I tested it a bit more in main. There are still a couple of bugs that I noticed but overall it works great! Thanks @Tarek-Yasser @JosephEagar for your work!
Sign in to join this conversation.
Reviewers
No reviewers
Julien Kaspar
Joseph Eagar
Labels
Clear labels   
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
This issue affects/is about backward or forward 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
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
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
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
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 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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
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#104718
No description provided.
Delete Branch "Tarek-Yasser/blender:sculpt_mode_add_menu_operators"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?