Anim: Per bone wire width for custom shapes #120176
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
9 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#120176
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ChrisLend/blender:bone_wire_width"
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?
The setting adds the "Custom Shape Wire Width" option to the "Viewport Display/Custom Shape" section of a pose bone.
As the setting says, this controls how thick the wire is drawn in the viewport.
This is done by adding a geometry shader that makes two triangles out of a line.
Location in menu.
![image](/blender/blender/attachments/250e6e05-f982-4307-9b68-45e1dda395e3)
The Anti-Aliasing is controlled by the setting Viewport->Quality->Smooth Wires->Overlay in the user preferences.
Example.
![image](/blender/blender/attachments/04bab050-5ddd-43c9-a918-9bfdfc285139)
Artifacts
When increasing the line width, you can get the following issue.
![image](/blender/blender/attachments/9e5849b6-dda9-4994-8827-54a738c591cb)
This comes from extruding each edge along the normal of its direction. This could be solved by adding round caps. IMO that is out of scope for this PR though since the same issue also occurs with thick edges in edit mode
For the review
Test File Attached
no_geom
shader. It is there though, so Mac shouldn't crash.Future improvements
Just a note. Apple doesn't support geometry shaders and a work-around needs to be added for the metal backend. See the "no geom" shaders.
@Jeroen-Bakker thanks I was wondering about those. I will ask in the blender chat for mac people to review/test this once it's ready.
See #119146 for implementation for no geom shaders.
Sorry wrong button.
@Jeroen-Bakker I got the Anti Aliasing to work.
I had this weird artifact that with certain bone colors, the bones disappeared.
Using
RenderDoc
I was able to determine that this came from theantialiasing_ps
.No idea why, but what I needed to do was to add the
lineOutput
back to the fragment shader. Even though I just always set it tovec4(0)
this makes it work.Yes lineOutput is defined as an output and should always be set. Otherwise it can create undefined behavior.
I think the simplest solution for the visual artifacts on thick wires might be to introduce rounded tips:
![image](/blender/blender/attachments/e7903277-fd95-4ea3-8ccf-58757fc5e3c4)
@ZedDB I thought about that as well. But I think I'll leave that for a future improvement.
WIP: Anim: Per bone wire width for custom shapesto Anim: Per bone wire width for custom shapeshmmm in the Rain rig, I can't seem to find the option to set the thickness. Am I looking in the wrong place?
@dr.sybren it should be there![image](/blender/blender/attachments/30f4be77-45a9-4deb-a031-b10d2ffeb7a4)
for good measure I just merged main in case that makes a difference
@ -0,0 +1,47 @@
/* SPDX-FileCopyrightText: 2019-2023 Blender Authors
if i may, i think
2023
should be replaced by2024
Good catch. I think the currently preferred approach is to just use the year at which the file was first created, so
2019-2023
→2024
.I didn't review the GLSL, my shader-fu is not strong enough.
@ -3237,0 +3240,4 @@
continue;
}
LISTBASE_FOREACH (bPoseChannel *, pchan, &ob->pose->chanbase) {
pchan->custom_shape_wire_width = 1.0;
It might be enough to add this to
DNA_action_defaults.h
. The file doesn't exist yet; check90ef46baa1
to see how I addedDNA_anim_defaults.h
.Just tried that and without the versioning code the wire_width is at 0 for old files.
Should I still add the defaults code?
@ -882,2 +890,3 @@
OVERLAY_bone_instance_data_set_color_hint(&inst_data, outline_color);
OVERLAY_bone_instance_data_set_color(&inst_data, outline_color);
inst_data.color_a = encode_2f_to_float(outline_color[0], outline_color[1]);
/* Due to the encoding clamping the passed in floats, the wire width needs to be scaled down.
eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeuw. Not your code, but just that this is necessary. Please do add what the floats are clamped to (I'm guessing 1.0, but better to have this explicit).
I am not sure what to do here. It is clamping to 2, but I am only using the 0-1 range.
Adding a comment here means it can go out of sync with the code easily.
@ -883,1 +891,3 @@
OVERLAY_bone_instance_data_set_color(&inst_data, outline_color);
inst_data.color_a = encode_2f_to_float(outline_color[0], outline_color[1]);
/* Due to the encoding clamping the passed in floats, the wire width needs to be scaled down.
* Keep in sync with the max value of RNA. */
Define this maximum as a
constexpr
in a header somewhere, then use it in the various places, instead of having16.0
that needs to be synced up.moved it to
DNA_action_types.h
just above wherebPoseChannel
is definedThe artifacts mentioned in the PR description are more pronounced with curved shapes. The image below shows this well, but is at thickness = 10, which I think will be used quite rarely in production files. With thickness = 4 it still looks good.
@ChrisLend do you know if round caps are something we'll get soon, as in for Blender 4.2?
@dr.sybren I don't know. There is still more than a month to go until beta, but I have never done such a thing so I can't give an estimate.
I think it should be doable by just emitting more vertices in the geometry shader....
@Jeroen-Bakker could you share some of your wisdom with us?
@ -0,0 +10,4 @@
geometry_out.finalColor = color;
geometry_noperspective_out.edgeCoord = coord;
gl_Position = pos;
/* Multiply offset by 2 because gl_Position range is [-1..1]. */
The comment seems a bit incomplete. It's nice that that range is
[-1…1]
, but without knowing what the range ofoffset
is, this doesn't help much.About this and the comment below. I copied that comment from the _geom.glsl file I used as a reference to construct this.
I see how that isn't clear from the PR but I am still uncertain how I should handle that.
Change the comment only in this file or make a refactor that changes it in all
@ -0,0 +12,4 @@
gl_Position = pos;
/* Multiply offset by 2 because gl_Position range is [-1..1]. */
gl_Position.xy += offset * 2.0 * pos.w;
/* Correct but fails due to an AMD compiler bug, see: #62792.
Move the comment underneath the
#if 0
so that it's clearer that it's scoped to that.Also issue #62792 has been closed as 'resolved' years ago, so not sure why this is referenced here.
@ -20,0 +17,4 @@
geometry_in.finalColor.a = 1.0;
/* Due to packing, the wire width is passed in compressed. If the RNA range is increased, this
* needs to change as well. */
geometry_in.wire_width = bone_color.a * 16.0;
AFAIK it should be possible to
#include
a file in both GLSL and C++, so that this constant can be shared from a single definition. If that's not possible, at least mentionWIRE_WIDTH_COMPRESSION
here so that it's clear where this value comes from.@ -0,0 +15,4 @@
/* Due to packing, the wire width is passed in compressed. If the RNA range is increased, this
* needs to change as well. */
const float wire_width = bone_color.a * 16.0;
Same as above, should be using the constant, or as a fallback reference it.
@ -246,1 +246,4 @@
/* Used in the armature drawing code. Due to the encoding clamping the passed in floats, the wire
* width needs to be scaled down. */
static constexpr float WIRE_WIDTH_COMPRESSION = 16.0f;
Also mention that this is used in some GLSL files. If those explicitly name the constant, it shouldn't be necessary to list the filenames here (as those can get outdated).
+This should be protected by
#ifdef __cplusplus
@mod_moder can you explain why?
This is C header, but
constexpr
is CPP thing.@ -1165,0 +1167,4 @@
RNA_def_property_ui_text(
prop, "Custom Shape Wire Width", "Adjust the line thickness of custom shapes");
/* When increasing the property range,
* also modify the geometry shader and overlay_armature.cc. */
This comment is now outdated.
LGTM!
@Jeroen-Bakker I have a local branch where I made rounded caps by emitting 10 verts instead of 4. That will be a separate PR once this one landed.
For this PR, the most important thing to answer is how I can develop the
no_geom
shader without owning a mac.I quickly looked at it, but there isn't a doc or compile directive that would help.
I will plan some time next week to look at your patch and how will document what needs to be done. Perhaps I will provide a patch along the side.
@ChrisLend here's the diff attached that makes custom bone shape wire widths work on Metal (for me at least!).
@aras_p I applied your diff, thanks again for that.
In one of your comments you say
Verch vertex positions
. Is verch a typo or just a word I don't know@blender-bot package
Package build started. Download here when ready.
What, you don't know the world-famous word "verch"?!
...it should have been "fetch" :)
I will test the patch on Thursday on Intel/AMD/Apple GPUs and do a review on the GPU part of this patch.
I tested and works on mac as well.
Some changes I would like to see:
WIRE_WIDTH_COMPRESSION
is really confusing and might not work as expected as it is stored in pixels and ignores HIDPI and resolution scaling.UI_GetThemeValuef(TH_EDGE_WIDTH)
There is a conveniencesizeEdge
in the global block that applies both already.Non HiDPI screen
![image](/blender/blender/attachments/59b8c92d-54d2-4c47-8b4d-d6195b55122d)
![image](/blender/blender/attachments/e7d54201-8094-482b-94bd-e21f40dcfb0d)
HiDPI screen
@ -459,2 +464,4 @@
DRW_shgroup_uniform_block(grp, "globalsBlock", G_draw.block_ubo);
DRW_shgroup_uniform_float_copy(grp, "alpha", 1.0f);
DRW_shgroup_uniform_bool_copy(grp, "do_smooth_wire", do_smooth_wire);
DRW_shgroup_uniform_float_copy(grp, "width_compression", WIRE_WIDTH_COMPRESSION);
Not sure what compression means or does by reading the name.
Currently the line width doesn't take into account the user preference resolution scale. When this will be added this 'constant' might become a variable. In this case using a uniform is good. If it is still a constant this uniform should be replaced with a define in
overlay_armature_info.hh
Because the value is passed in packed next to the color it has to be in a range of 0-2 (Though I am only using 0-1 here)
See
overlay_armature.cc/892
.To achieve this range I am dividing by the max wire width allowed by RNA and then multiplying by that same number in the shader, essentially packing and unpacking. The precision loss doesn't matter in this case.
Given that this is heavily linked to the RNA range, @dr.sybren suggested using a
constexpr
which can be found inDNA_action_types.h/274
Insofar it could be a define, if I can use
WIRE_WIDTH_COMPRESSION
converted to a string passed into the shader.There is already a comment in the shader when the value is unpacked, I've added one for the no_geom version as well
sizeEdge
22ab17d70b@Jeroen-Bakker I've added the
sizeEdge
within the shader.As a result the wire width was halved in the case of default settings.
Is that expected?
Yes the sizeEdge is the distance from the center of the edge to the outer part.
For the rendering of the caps we should use a different solution as proposed previously. In stead of calculating the caps, we should just draw the vertices on top of the lines using the same width.
Christoph Lendenfeld referenced this pull request2024-05-30 12:03:18 +02:00
Ah in that case(sizeEdge * 2)
is what I need.Ignore me, it was actually too thick before. Now it is correct.
Edit: Ok still not quite. To match the look before this PR, the wire width needs to be 0.5
To get the correct pixel width of the line I need
(sizeEdge * 2)
Not sure what you mean exactly, but I'd like to keep that as a separate PR if possible.
I started work on this on a branch, just now put up the PR for visibility
#122481: WIP: Anim: Rounded caps for thick bone wires
If you use sizeEdge * 2, doesn't this mean that if the line width is set to 10 it will extend to both size the equivalent of 10 pixels, making the width of the edge twice what the user actually wants.
Note see a diff to remove the uniform float for width compression. The only side effect is that in the RNA this value needs to be duplicated. IMO this is not an issue as this is typically done for other UI ranges as well.
@Jeroen-Bakker I had to use
(sizeEdge * 2)
in the end because thewire_width
is expected to be the total width of the wire.The value gets halved again later in the code to calculate the offsets. Confirmed by pixel counting.
Bone wires of size 1 DO look a bit thicker after this patch though. That is because of AntiAliasing adding 1px of width.
I briefly tried reducing the lower range of the rna property to 0.5 which makes the wire look the same as before this PR.
However that breaks as soon as AA is turned off.
I will see if I can get around this by not expanding the geometry, but rather adapt the code in the fragment shader.
@blender-bot build
Hey, 3 things about this patch
When duplicating a bone, the custom width is not carried over, but all other custom shape settings are
The hard max limit seems arbitrary at 16? Limits are usually rounded at 10 or 100 or 1000
![image](/blender/blender/attachments/19a4c10b-4e46-4e1e-9d0d-c207b5df2f1c)
@ChrisLend @linen @dr.sybren Hi, I don't know if the above issues have reached you
It has, but Christoph was on holiday, so hence it was sitting still for a little bit.
@zanqdo 1 and 2 I agree with, will send fixes
for the hard limit, I just set it to something so thick I thought it unlikely anyone will want to use it. Hence the soft limit at 10
If you want to go higher than 16 let me know. We can expand that limit
Fair enough, thank you for addressing the feedback