Anim: Per bone wire width for custom shapes #120176

Open
Christoph Lendenfeld wants to merge 33 commits from ChrisLend/blender:bone_wire_width into main

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

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

The Anti-Aliasing is controlled by the setting Viewport->Quality->Smooth Wires->Overlay in the user preferences.

Example.
image

Artifacts

When increasing the line width, you can get the following issue.
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
image

For the review

Test File Attached

  • I am packing the wire_width into the color alpha value because that logic already exists and I couldn't find another way. Anything that can be improved there?
  • It will not work on Mac. Since I don't have a mac I have no way of developing/testing the no_geom shader. It is there though, so Mac shouldn't crash.

Future improvements

  • Rounded caps to remove artifacts on the end of edges.
  • Per bone alpha value. Can be really useful on thick wires.
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](/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](/attachments/04bab050-5ddd-43c9-a918-9bfdfc285139) ## Artifacts When increasing the line width, you can get the following issue. 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 ![image](/attachments/9e5849b6-dda9-4994-8827-54a738c591cb) ## For the review **Test File Attached** * I am packing the wire_width into the color alpha value because that logic already exists and I couldn't find another way. Anything that can be improved there? * It will not work on Mac. Since I don't have a mac I have no way of developing/testing the `no_geom` shader. It is there though, so Mac shouldn't crash. ## Future improvements * Rounded caps to remove artifacts on the end of edges. * Per bone alpha value. Can be really useful on thick wires.
Christoph Lendenfeld added the
Module
Animation & Rigging
label 2024-04-02 16:06:57 +02:00
Christoph Lendenfeld added 9 commits 2024-04-02 16:07:08 +02:00
Christoph Lendenfeld added 1 commit 2024-04-02 16:27:13 +02:00
Member

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.

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.
Author
Member

@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.

@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.
Christoph Lendenfeld added 2 commits 2024-04-02 17:27:20 +02:00
Christoph Lendenfeld added 2 commits 2024-04-09 17:24:50 +02:00
Member

See #119146 for implementation for no geom shaders.

See #119146 for implementation for no geom shaders.
Jeroen Bakker closed this pull request 2024-04-11 10:24:25 +02:00
Jeroen Bakker reopened this pull request 2024-04-11 10:24:31 +02:00
Member

Sorry wrong button.

Sorry wrong button.
Christoph Lendenfeld added 1 commit 2024-04-11 14:58:37 +02:00
Christoph Lendenfeld added 1 commit 2024-04-12 15:57:25 +02:00
Christoph Lendenfeld added 2 commits 2024-04-16 10:13:36 +02:00
Christoph Lendenfeld added 1 commit 2024-04-16 11:25:50 +02:00
Author
Member

@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 the antialiasing_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 to vec4(0) this makes it work.

@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 the `antialiasing_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 to `vec4(0)` this makes it work.
Christoph Lendenfeld added 1 commit 2024-04-16 11:40:38 +02:00
Member

Yes lineOutput is defined as an output and should always be set. Otherwise it can create undefined behavior.

Yes lineOutput is defined as an output and should always be set. Otherwise it can create undefined behavior.
Christoph Lendenfeld added 1 commit 2024-04-16 17:25:13 +02:00
Christoph Lendenfeld added 1 commit 2024-04-16 17:41:30 +02:00

I think the simplest solution for the visual artifacts on thick wires might be to introduce rounded tips:
image

I think the simplest solution for the visual artifacts on thick wires might be to introduce rounded tips: ![image](/attachments/e7903277-fd95-4ea3-8ccf-58757fc5e3c4)
110 KiB
Christoph Lendenfeld added 2 commits 2024-04-18 11:13:28 +02:00
a78cb3a793 remove code of setting an index of edge_ofs to 0
now it properly creates the geometry based on the
direction of the line
Author
Member

@ZedDB I thought about that as well. But I think I'll leave that for a future improvement.

@ZedDB I thought about that as well. But I think I'll leave that for a future improvement.
Christoph Lendenfeld changed title from WIP: Anim: Per bone wire width for custom shapes to Anim: Per bone wire width for custom shapes 2024-04-18 11:25:59 +02:00
Christoph Lendenfeld added 1 commit 2024-04-18 11:31:23 +02:00
Christoph Lendenfeld added 1 commit 2024-04-18 11:38:45 +02:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-04-18 11:44:01 +02:00
Christoph Lendenfeld requested review from Jeroen Bakker 2024-04-18 11:44:02 +02:00

hmmm in the Rain rig, I can't seem to find the option to set the thickness. Am I looking in the wrong place?

image

hmmm in the Rain rig, I can't seem to find the option to set the thickness. Am I looking in the wrong place? ![image](/attachments/1d3cbda0-e9ca-40a1-9731-11828a19207e)
Author
Member

@dr.sybren it should be there image
for good measure I just merged main in case that makes a difference

@dr.sybren it should be there ![image](/attachments/30f4be77-45a9-4deb-a031-b10d2ffeb7a4) for good measure I just merged main in case that makes a difference
Christoph Lendenfeld added 2 commits 2024-04-25 15:28:30 +02:00
Gangneron reviewed 2024-04-26 15:18:09 +02:00
@ -0,0 +1,47 @@
/* SPDX-FileCopyrightText: 2019-2023 Blender Authors
First-time contributor

if i may, i think 2023should be replaced by 2024

if i may, i think `2023`should be replaced by `2024`

Good catch. I think the currently preferred approach is to just use the year at which the file was first created, so 2019-20232024.

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`.
Sybren A. Stüvel requested changes 2024-04-26 15:34:24 +02:00
Dismissed
Sybren A. Stüvel left a comment
Member

I didn't review the GLSL, my shader-fu is not strong enough.

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; check 90ef46baa1 to see how I added DNA_anim_defaults.h.

It might be enough to add this to `DNA_action_defaults.h`. The file doesn't exist yet; check 90ef46baa1bcacc1a886ac5ac4493fc46eaac7ce to see how I added `DNA_anim_defaults.h`.
Author
Member

Just tried that and without the versioning code the wire_width is at 0 for old files.
Should I still add the defaults code?

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).

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).
Author
Member

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.

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 having 16.0 that needs to be synced up.

Define this maximum as a `constexpr` in a header somewhere, then use it in the various places, instead of having `16.0` that needs to be synced up.
Author
Member

moved it to DNA_action_types.h just above where bPoseChannel is defined

moved it to `DNA_action_types.h` just above where `bPoseChannel` is defined

The 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.

image

@ChrisLend do you know if round caps are something we'll get soon, as in for Blender 4.2?

The 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. ![image](/attachments/446a92a9-0747-43b3-941e-d8d5fd709517) @ChrisLend do you know if round caps are something we'll get soon, as in for Blender 4.2?
Author
Member

@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....

@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....
Christoph Lendenfeld added 1 commit 2024-04-26 16:07:09 +02:00
Christoph Lendenfeld added 2 commits 2024-04-26 16:42:13 +02:00

@Jeroen-Bakker could you share some of your wisdom with us?

@Jeroen-Bakker could you share some of your wisdom with us?
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-04-26 16:47:03 +02:00
Sybren A. Stüvel requested changes 2024-04-29 11:23:04 +02:00
Dismissed
@ -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 of offset is, this doesn't help much.

The comment seems a bit incomplete. It's nice that that range is `[-1…1]`, but without knowing what the range of `offset` is, this doesn't help much.
Author
Member

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

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.

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 mention WIRE_WIDTH_COMPRESSION here so that it's clear where this value comes from.

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 mention `WIRE_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.

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).

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

+This should be protected by `#ifdef __cplusplus`
Author
Member

@mod_moder can you explain why?

@mod_moder can you explain why?

This is C header, but constexpr is CPP thing.

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.

This comment is now outdated.
Christoph Lendenfeld added 1 commit 2024-04-30 12:44:45 +02:00
Christoph Lendenfeld added 1 commit 2024-04-30 13:07:09 +02:00
Sybren A. Stüvel approved these changes 2024-05-02 11:40:38 +02:00
Sybren A. Stüvel left a comment
Member

LGTM!

LGTM!
This pull request has changes conflicting with the target branch.
  • source/blender/blenkernel/BKE_blender_version.h
  • source/blender/blenloader/intern/versioning_400.cc

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u bone_wire_width:ChrisLend-bone_wire_width
git checkout ChrisLend-bone_wire_width
Sign in to join this conversation.
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
No Milestone
No project
No Assignees
6 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#120176
No description provided.