UI: Add custom attribute edge groups for bevel modifier #117366

Open

Mike93 wants to merge 1 commits from Mike93/blender:bevel-edge-attributes into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Mike93 commented

2024-01-20 18:35:55 +01:00

First-time contributor

As of 4.0, bevel edge weight data is stored as an "attribute" of type float, domain edge. The bevel modifier is currently hard-coded to use the bevel_weight_edge attribute as the data source for the edge weights. This small change provides a string textbox where the user can specify alternate attributes for the bevel's edge weights.

I tried to implement a search dropdown box that would filter on attributes of type float and domain edge but 1) could not figure it out, and 2) was advised that it was more complexity than was worth it. This might be a temporary upgrade to the bevel modifier until bevel geometry nodes take over.

One more note: this code change requires adding a parameter to the bmesh bevel operator - so there's a chance this could break addons that don't use that last argument. Maybe there is a way to do default arguments or something to get around this.

As of 4.0, bevel edge weight data is stored as an "attribute" of type `float`, domain `edge`. The bevel modifier is currently hard-coded to use the `bevel_weight_edge` attribute as the data source for the edge weights. This small change provides a string textbox where the user can specify alternate attributes for the bevel's edge weights. ![image](/attachments/8ea283dd-6b2f-43f7-9abc-18238dbd9431) I tried to implement a search dropdown box that would filter on attributes of type `float` and domain `edge` but 1) could not figure it out, and 2) was advised that it was more complexity than was worth it. This might be a temporary upgrade to the bevel modifier until bevel geometry nodes take over. One more note: this code change requires adding a parameter to the bmesh bevel operator - so there's a chance this could break addons that don't use that last argument. Maybe there is a way to do default arguments or something to get around this.

16 KiB

👍 3 ❤️ 11 🎉 4 🚀 4

Mike93 changed title from ~~Add custom attribute edge groups for bevel modifier~~ to UI: Add custom attribute edge groups for bevel modifier

2024-01-20 18:39:56 +01:00

Iliya Katushenock added this to the Modeling project 2024-01-20 18:44:32 +01:00

Iliya Katushenock added the

Nodes & Physics

labels 2024-01-20 18:44:41 +01:00

Howard Trickey requested changes 2024-01-21 23:09:32 +01:00

Howard Trickey left a comment

Member

This generally looks fine to me, and makes a lot of sense given how it basically just allows changing a hard-coded property string.

It would indeed by nice if you could do something like the UI for vertex groups that shows up in the bevel modifier to choose a vertex group name. I looked a bit at the code that does that and couldn't figure out where the actual guts of the panel drawing happened. I wouldn't regard the lack of this as a show-stopper for putting this in.

Normally I would insist on trying to have parity of features with the edit mode bevel. But when you are doing destructive bevels, it doesn't seem that much of a hardship to just use the default bevel edge weight property. And anyway, I'm hoping to make bigger changes to bevel next year and so getting the current version just right seems less than crucial to me.

This generally looks fine to me, and makes a lot of sense given how it basically just allows changing a hard-coded property string. It would indeed by nice if you could do something like the UI for vertex groups that shows up in the bevel modifier to choose a vertex group name. I looked a bit at the code that does that and couldn't figure out where the actual guts of the panel drawing happened. I wouldn't regard the lack of this as a show-stopper for putting this in. Normally I would insist on trying to have parity of features with the edit mode bevel. But when you are doing destructive bevels, it doesn't seem that much of a hardship to just use the default bevel edge weight property. And anyway, I'm hoping to make bigger changes to bevel next year and so getting the current version just right seems less than crucial to me.

source/blender/makesrna/intern/rna_modifier.cc Outdated

						
				@ -954,1 +954,4 @@

				static void rna_BevelModifier_custom_weight_name_set(PointerRNA *ptr, const char *value)

				{

				  Object *owner = (Object *)ptr->owner_id;

Howard Trickey commented

2024-01-21 22:30:32 +01:00

Member

Remove this line; you never used this result.

Remove this line; you never used this result.

Mike93 marked this conversation as resolved

Mike93 force-pushed bevel-edge-attributes from 0f9c53e677 to 176dde6caf

2024-01-22 00:01:56 +01:00

Compare

Mike93 commented

2024-01-22 00:03:52 +01:00

Author

First-time contributor

Removed the unused reference to owner

Removed the unused reference to `owner`

Mike93 closed this pull request

2024-01-22 00:03:54 +01:00

Mike93 commented

2024-01-22 00:04:28 +01:00

Author

First-time contributor

Not sure what happened... did not mean to close, just meant to comment. Trying to re-open now.

Not sure what happened... did not mean to close, just meant to comment. Trying to re-open now.

Mike93 reopened this pull request

2024-01-22 00:04:30 +01:00

Mike93 requested review from Howard Trickey 2024-01-22 00:05:44 +01:00

Howard Trickey commented

2024-01-25 20:27:28 +01:00

Member

How hard would it be to do the same thing for vertex bevel weights? Hans stated that for consistency, we should probably do that too. I'm not sure how often people use vertex bevel weights -- I guess that's the main question as to whether the consistency is worth the bother.

How hard would it be to do the same thing for vertex bevel weights? Hans stated that for consistency, we should probably do that too. I'm not sure how often people use vertex bevel weights -- I guess that's the main question as to whether the consistency is worth the bother.

Mike93 commented

2024-01-25 21:05:13 +01:00

Author

First-time contributor

How hard would it be to do the same thing for vertex bevel weights? Hans stated that for consistency, we should probably do that too. I'm not sure how often people use vertex bevel weights -- I guess that's the main question as to whether the consistency is worth the bother.

Took a quick stab at it, seems to work fine (I tested the default attribute which is bevel_weight_vert as well):

Had to refactor some of the RNA/DNA names and descriptions so they are more clear. I like this more anyways, actually, since it is clear that the limit method of weight clearly points to an attribute now.

As per whether people use vertex beveling... I personally have never used it. Nor have I ever seen a tutorial or video of someone else using it. I also don't know if there's a standard way of creating vertex weights, like there is with bevel edge weights (Ctrl + E > Bevel Edge Weight).

Let me know if you want me to bring in these changes.

> How hard would it be to do the same thing for vertex bevel weights? Hans stated that for consistency, we should probably do that too. I'm not sure how often people use vertex bevel weights -- I guess that's the main question as to whether the consistency is worth the bother. Took a quick stab at it, seems to work fine (I tested the default attribute which is `bevel_weight_vert` as well): ![image](/attachments/ea801534-9bfb-4480-8f7a-68e3b85e11a9) ![image](/attachments/835c7de7-8d4a-41ce-92a1-bf7d16c34a10) Had to refactor some of the RNA/DNA names and descriptions so they are more clear. I like this more anyways, actually, since it is clear that the `limit method` of `weight` clearly points to an attribute now. As per whether people use vertex beveling... I personally have never used it. Nor have I ever seen a tutorial or video of someone else using it. I also don't know if there's a standard way of creating vertex weights, like there is with bevel edge weights (`Ctrl + E > Bevel Edge Weight`). Let me know if you want me to bring in these changes.

109 KiB

123 KiB

Howard Trickey commented

2024-01-25 21:08:05 +01:00

Member

Yes, please bring in these changes. Thanks for doing it!
I'm waiting for Campbell to review this too, as he said he had some comments., but to me this is looking good to include in 4.1.

Yes, please bring in these changes. Thanks for doing it! I'm waiting for Campbell to review this too, as he said he had some comments., but to me this is looking good to include in 4.1.

Howard Trickey requested review from Campbell Barton 2024-01-25 21:08:47 +01:00

Mike93 force-pushed bevel-edge-attributes from 176dde6caf to 9e4942ce0a

2024-01-25 21:15:01 +01:00

Compare

Howard Trickey commented

2024-01-26 13:47:50 +01:00

Member

While you are waiting for the other review, one thing to think about and perhaps work on is what should go in the release notes for this change. We like to put documentation in the release notes about the same time as we commit things that change visible behavior. The note would likely go here: https://developer.blender.org/docs/release_notes/4.1/modeling/ . Also, make sure that the automated tests are not broken by your changes. See https://developer.blender.org/docs/handbook/testing/

While you are waiting for the other review, one thing to think about and perhaps work on is what should go in the release notes for this change. We like to put documentation in the release notes about the same time as we commit things that change visible behavior. The note would likely go here: https://developer.blender.org/docs/release_notes/4.1/modeling/ . Also, make sure that the automated tests are not broken by your changes. See https://developer.blender.org/docs/handbook/testing/

Mike93 commented

2024-01-27 00:11:12 +01:00

Author

First-time contributor

Okay thanks for the links. I will probably have some time this weekend or early next week to take a look at it.

Okay thanks for the links. I will probably have some time this weekend or early next week to take a look at it.

Mike93 referenced this pull request from blender/blender-developer-docs

2024-01-30 01:45:29 +01:00

Bevel modifier: vertex/edge weight attributes now exposed as string input #14

Mike93 commented

2024-01-30 01:46:51 +01:00

Author

First-time contributor

blender/blender-developer-docs#14

Will look into the testing later this week

https://projects.blender.org/blender/blender-developer-docs/pulls/14 Will look into the testing later this week

Campbell Barton referenced this pull request

2024-02-06 13:04:28 +01:00

Attribute References in Modifiers #117893

Campbell Barton referenced this pull request

2024-02-06 13:08:55 +01:00

Attribute References in Modifiers #117893

Campbell Barton commented

2024-02-06 13:17:19 +01:00

Owner

As far as I know we don't have conventions for attributes in modifiers, created #117893 to address this.

From reading over this patch there looks to be some issues:

Renaming an attribute doesn't update the attribute name in the modifier, wouldn't users expect this?
Existing files will have empty attribute names, since there doesn't look to be any versioning code, in #117893 I suggest to use empty names which fall back to the default attribute - in that case versioning isn't needed. Whatever the case, loading existing files shouldn't loose references to vertex/edge weights.

As far as I know we don't have conventions for attributes in modifiers, created #117893 to address this. From reading over this patch there looks to be some issues: - Renaming an attribute doesn't update the attribute name in the modifier, wouldn't users expect this? - Existing files will have empty attribute names, since there doesn't look to be any versioning code, in #117893 I suggest to use empty names which fall back to the default attribute - in that case versioning isn't needed. Whatever the case, loading existing files shouldn't loose references to vertex/edge weights.

Mike93 commented

2024-02-06 17:38:44 +01:00

Author

First-time contributor

I read through your other issue, it all makes sense to me. I attempted to fix the empty attribute name issue by putting this in modify_mesh:

  if (bmd->vertex_weight_name == "") {
    STRNCPY(bmd->vertex_weight_name, "bevel_weight_vert");
  }
  if (bmd->edge_weight_name == "") {
    STRNCPY(bmd->edge_weight_name, "bevel_weight_edge");
  }

But I'm probably missing something larger about the DNA/RNA philosophy. This is the logic that's used in the RNA string function which actually sets the attribute value, but it seems I can't do it this way outside of the UI.

I read through your other issue, it all makes sense to me. I attempted to fix the empty attribute name issue by putting this in `modify_mesh`: ``` if (bmd->vertex_weight_name == "") { STRNCPY(bmd->vertex_weight_name, "bevel_weight_vert"); } if (bmd->edge_weight_name == "") { STRNCPY(bmd->edge_weight_name, "bevel_weight_edge"); } ``` But I'm probably missing something larger about the DNA/RNA philosophy. This is the logic that's used in the RNA string function which actually sets the attribute value, but it seems I can't do it this way outside of the UI.

Campbell Barton requested changes 2024-02-07 09:47:15 +01:00

Campbell Barton left a comment

Owner

Based on Hans reply to #117893 it seems like the names are expected to be set (no fallback for empty names). This means existing files will need to be updated.

Check source/blender/blenloader/intern/versioning_400.cc (for reference version_vertex_weight_edit_preserve_threshold_exclusivity does something similar).

Based on Hans reply to #117893 it seems like the names are expected to be set (no fallback for empty names). This means existing files will need to be updated. Check `source/blender/blenloader/intern/versioning_400.cc` (for reference `version_vertex_weight_edit_preserve_threshold_exclusivity` does something similar).

Mike93 commented

2024-02-08 01:33:24 +01:00

Author

First-time contributor

Thanks for the references. Sorry, this is going to seem unrelated, but are we technically not accepting new features for 4.1 now? I was reading this. I'm under a mountain of work right now and I'd like to know how long I have to tinker with this.

I'd really like to see this feature get implemented, and I'd be worried that it would get lost in the shuffle.

Thanks for the references. Sorry, this is going to seem unrelated, but are we technically not accepting new features for 4.1 now? I was reading [this](https://projects.blender.org/blender/blender/milestone/18). I'm under a mountain of work right now and I'd like to know how long I have to tinker with this. I'd really like to see this feature get implemented, and I'd be worried that it would get lost in the shuffle.

Daniel Gryningstjerna commented

2024-03-12 17:40:51 +01:00

First-time contributor

Any news? Seems really useful! and Bevel v2 is still very far away.

Any news? Seems really useful! and Bevel v2 is still very far away.

Mike93 commented

2024-03-12 17:44:35 +01:00

Author

First-time contributor

I'm still keeping an eye on the proposal regarding attribute references, but unfortunately my work load has pretty much doubled in the past month or so and I have a deadline to hit at beginning of April.

If another dev wants to pick this up I would be more than happy to walk through what I've done so far. The difficult piece will be "Clicking on the name shows a drop-down list of names (as with vertex groups), this includes the default name.". I spent nearly a day trying to figure this out, and it's beyond me. I did use the attribute dropdown in geo-nodes as a reference, but still couldn't hack it.

I'm still keeping an eye on the [proposal](https://projects.blender.org/blender/blender/issues/117893) regarding attribute references, but unfortunately my work load has pretty much doubled in the past month or so and I have a deadline to hit at beginning of April. If another dev wants to pick this up I would be more than happy to walk through what I've done so far. The difficult piece will be "Clicking on the name shows a drop-down list of names (as with vertex groups), this includes the default name.". I spent nearly a day trying to figure this out, and it's beyond me. I did use the attribute dropdown in geo-nodes as a reference, but still couldn't hack it.

Daniel Gryningstjerna commented

2024-03-12 18:00:53 +01:00

First-time contributor

I'm still keeping an eye on the proposal regarding attribute references, but unfortunately my work load has pretty much doubled in the past month or so and I have a deadline to hit at beginning of April.

If another dev wants to pick this up I would be more than happy to walk through what I've done so far. The difficult piece will be "Clicking on the name shows a drop-down list of names (as with vertex groups), this includes the default name.". I spent nearly a day trying to figure this out, and it's beyond me. I did use the attribute dropdown in geo-nodes as a reference, but still couldn't hack it.

Understandable, and thanks for taking the patch so far! Hope it eventually lands some day, but dont stress about it :)

> I'm still keeping an eye on the [proposal](https://projects.blender.org/blender/blender/issues/117893) regarding attribute references, but unfortunately my work load has pretty much doubled in the past month or so and I have a deadline to hit at beginning of April. > > If another dev wants to pick this up I would be more than happy to walk through what I've done so far. The difficult piece will be "Clicking on the name shows a drop-down list of names (as with vertex groups), this includes the default name.". I spent nearly a day trying to figure this out, and it's beyond me. I did use the attribute dropdown in geo-nodes as a reference, but still couldn't hack it. Understandable, and thanks for taking the patch so far! Hope it eventually lands some day, but dont stress about it :)

Merge conflict checking is in progress. Try again in few moments.

View command line instructions.

Checkout

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

git fetch -u bevel-edge-attributes:Mike93-bevel-edge-attributes

git checkout Mike93-bevel-edge-attributes

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 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#117366

No description provided.