UI: Add custom attribute edge groups for bevel modifier #117366
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#117366
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "Mike93/blender:bevel-edge-attributes"
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?
As of 4.0, bevel edge weight data is stored as an "attribute" of type
float
, domainedge
. The bevel modifier is currently hard-coded to use thebevel_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 domainedge
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.
Add custom attribute edge groups for bevel modifierto UI: Add custom attribute edge groups for bevel modifierThis 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.
@ -954,1 +954,4 @@
static void rna_BevelModifier_custom_weight_name_set(PointerRNA *ptr, const char *value)
{
Object *owner = (Object *)ptr->owner_id;
Remove this line; you never used this result.
0f9c53e677
to176dde6caf
Removed the unused reference to
owner
Not sure what happened... did not mean to close, just meant to comment. Trying to re-open now.
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
ofweight
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.
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.
176dde6caf
to9e4942ce0a
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/
Okay thanks for the links. I will probably have some time this weekend or early next week to take a look at it.
blender/blender-developer-docs#14
Will look into the testing later this week
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:
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
: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.
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 referenceversion_vertex_weight_edit_preserve_threshold_exclusivity
does something similar).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.
Any news? Seems really useful! and Bevel v2 is still very far away.
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 :)
A couple of updates here. I haven't squashed/updated my commit for this PR yet because I want to wait and see if I can fix the remaining issues.
@ideasman42 as per your second bullet point about fallback, that's working now, I was just missing an
#include "BLI_string.h"
at the top forSTRNCPY
to work. So when you create a new .blend file, the default values arevertex_weight_edge
andbevel_weight_edge
(depending on if Vertices/Edge are selected at the top) when you selectWeight
as the Limit Method. In my opinion this functions nicely as it maintains existing behaviour by default.As per your first point - whether the modifier should get an updated attribute name if the name is updated. I would say no, because when you use Vertex Groups, and you change the vertex group name after assigning it to the modifier, it does not update. It does, however, give you a red indication that the "link" to the vertex group is broken:
![image](/blender/blender/attachments/a7654888-c0f5-492f-9d97-5aed79995d61)
In this case I assigned a group called
Group
and then renamed it in the vertex group panel.I am still investigating implementing the dropdown. One hang up I am having, is that it seems that vertex group data is a part of the
Object
structure whereas attributes are assigned to theMesh
. This is odd, in my opinion, as I don't know what the conceptual difference would be - vertex groups are assigned to "groups" of vertices from a mesh, and attributes are just more general vertex groups in a sense (Vertex, Edge, Face, Face Center).If anyone knows of an "easier" example to pick on for the dropdown functionality, that would be a great help. I've been searching through the code for another couple hours here and still getting nowhere. There's a function called
modifier_vgroup_ui
inMOD_bevel.cc
that seems to be where the "magic" happens for the dropdown, but I am struggling to see how it could be implemented with effectively a "filter" for the right type of attribute group (vertex or edge as appropriate).I do believe it makes sense for this "weight" group attribute to function the same as the vertex groups - i.e., show the appropriate attributes in the dropdown so they can be easily selected, and highlight them red if the link is broken due to a rename.
I think it's done. To get the drop down to function properly, filtering on Vertex or Edge attributes, we need quite a bit of extra scaffolding for the custom search function. Here's the dropdown for Edge bevels by weight:
And for Vertex bevels:
![image](/blender/blender/attachments/3a28550c-9bfc-46db-81c0-63e862c62c0e)
You'll notice the "position" attribute is in the list - that seems to be a default vertex attribute that gets added to any mesh data. I also filtered out any attributes that started with a period, because it seemed that those were internal attributes.
After renaming the new edge group, "test_edge_weights" to "test_edge_weights2", we see the same behaviour as the vertex group dropdown:
![image](/blender/blender/attachments/a33802b5-f320-437c-a7c0-be7f1ed1f1ff)
So all in all - this will work, but I don't think it's coded in the prettiest way.
Here are the functions I had to add:
uiItemPointerR_att_prop
same asuiItemPointerR_prop
, but with an additionalint domain_filter
argumentui_but_add_search_att
same asui_but_add_search
but with an additionalint domain_filter
argument-
ui_rna_collection_search_att_point/edge_update_fn
, exactly same asui_rna_collection_search_update_fn
, except has an if statement which checks the domain type and filters out the attributes starting with "."This is a real architectural issue that I don't know how to get around and will need input on. Obviously copy/pasting this entire function for each attribute domain type is inefficient.
We can't add "domain_filter" as an integer parameter to this function, as the search button update callback depends on having a particular method signature, and presumably is used in many other places throughout Blender. So arbitrarily adding an argument to this new
search_update_fn
would likely break the search button update functions elsewhere.I will try to update my PR soon, but in the mean time, any feedback on these issues would be helpful.
9e4942ce0a
tob5e84a9c75
Squashed everything into the latest commit. So now you can see all the functions I added. I would gladly take any comments on how to make this code prettier/more efficient.
Edit: maybe a macro might be nice in place of duplicating the function each time? Not sure Blender's coding standards on macros. We could preserve the signature of the function and still keep the code compact ish:
Something like:
And then usage:
Definitely, it would be an amazing addition to Blender. For some complex models, the Bevel Modifier with the Weights option could be a big limitation when making the final bevels of a model or some specific ones. Using the Bevel tool is a destructive way in native Blender, and in the case of using addons, you'll need to select all the loops to modify, something that becomes much simpler with the ability to select and modify "Edge Groups" in one or two clicks. It would be much better than Vertex Groups for beveling, because in many cases they bevel edges you don't want to.
So, to summarize, a new system of Edge Groups implemented in the Bevel Modifier would: create a much more efficient workflow, allow for complete non-destructive management of bevels, and provide an easier way for 3ds Max users to adapt to Blender.
I'm sure there would be many more benefits to implementing this feature in Blender than the ones I mentioned. That's why I hope the Blender Team implements this great feature. Cheers!
Checkout
From your project repository, check out a new branch and test the changes.