Anim: Per bone wire width for custom shapes #120176

Merged
Christoph Lendenfeld merged 48 commits from ChrisLend/blender:bone_wire_width into main 2024-05-30 15:51:42 +02:00

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
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!
Christoph Lendenfeld added 2 commits 2024-05-02 12:12:03 +02:00
Christoph Lendenfeld added 1 commit 2024-05-02 12:16:22 +02:00
Author
Member

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

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

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.

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.
Christoph Lendenfeld added 1 commit 2024-05-24 16:43:35 +02:00

@ChrisLend here's the diff attached that makes custom bone shape wire widths work on Metal (for me at least!).

@ChrisLend here's the diff attached that makes custom bone shape wire widths work on Metal (for me at least!).
Christoph Lendenfeld added 2 commits 2024-05-28 14:00:47 +02:00
apply aras' diff
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
422b30e893
Author
Member

@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

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

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR120176) when ready.

In one of your comments you say Verch vertex positions. Is verch a typo or just a word I don't know

What, you don't know the world-famous word "verch"?!
...it should have been "fetch" :)

> In one of your comments you say `Verch vertex positions`. Is verch a typo or just a word I don't know What, you don't know the world-famous word "verch"?! ...it should have been "fetch" :)
Christoph Lendenfeld added 1 commit 2024-05-28 15:07:46 +02:00
Member

I will test the patch on Thursday on Intel/AMD/Apple GPUs and do a review on the GPU part of this patch.

I will test the patch on Thursday on Intel/AMD/Apple GPUs and do a review on the GPU part of this patch.
Jeroen Bakker requested changes 2024-05-30 08:44:01 +02:00
Dismissed
Jeroen Bakker left a comment
Member

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.
  • User preferences resolution scale is ignored and should be added. In shaders pixels should be multiplied with the pixel size. Edges should take into account UI_GetThemeValuef(TH_EDGE_WIDTH) There is a convenience sizeEdge in the global block that applies both already.

Non HiDPI screen
image
HiDPI screen
image

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. - User preferences resolution scale is ignored and should be added. In shaders pixels should be multiplied with the pixel size. Edges should take into account `UI_GetThemeValuef(TH_EDGE_WIDTH)` There is a convenience `sizeEdge` in the global block that applies both already. Non HiDPI screen ![image](/attachments/59b8c92d-54d2-4c47-8b4d-d6195b55122d) HiDPI screen ![image](/attachments/e7d54201-8094-482b-94bd-e21f40dcfb0d)
1.8 MiB
2.1 MiB
@ -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);
Member

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

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

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 in DNA_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

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 in `DNA_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
Jeroen-Bakker marked this conversation as resolved
Christoph Lendenfeld added 3 commits 2024-05-30 10:45:54 +02:00
Author
Member

@Jeroen-Bakker I've added the sizeEdgewithin the shader.
As a result the wire width was halved in the case of default settings.

Is that expected?

@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?
Christoph Lendenfeld requested review from Jeroen Bakker 2024-05-30 10:47:46 +02:00
Member

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.

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

Yes the sizeEdge is the distance from the center of the edge to the outer part.

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)

For the rendering of the caps we should use a different solution as proposed previously.

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

> Yes the sizeEdge is the distance from the center of the edge to the outer part. ~~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)` > For the rendering of the caps we should use a different solution as proposed previously. 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](https://projects.blender.org/blender/blender/pulls/122481)
Member

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.

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.
Christoph Lendenfeld added 2 commits 2024-05-30 12:28:17 +02:00
Christoph Lendenfeld added 1 commit 2024-05-30 12:58:58 +02:00
Author
Member

@Jeroen-Bakker I had to use (sizeEdge * 2) in the end because the wire_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.

@Jeroen-Bakker I had to use `(sizeEdge * 2)` in the end because the `wire_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.
Jeroen Bakker approved these changes 2024-05-30 13:42:44 +02:00
Christoph Lendenfeld added 1 commit 2024-05-30 14:46:06 +02:00
ensure that wires even with AA wires can be 1px wide
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
4468f41e98
Author
Member

@blender-bot build

@blender-bot build
Christoph Lendenfeld added 1 commit 2024-05-30 15:04:42 +02:00
Christoph Lendenfeld merged commit f9ea64b0ba into main 2024-05-30 15:51:42 +02:00
Christoph Lendenfeld deleted branch bone_wire_width 2024-05-30 15:51:45 +02:00
Member

Hey, 3 things about this patch

  1. The tittle should only be Wire Width. since it's already under the shapekey panel, that part of the name is redundant

image

  1. When duplicating a bone, the custom width is not carried over, but all other custom shape settings are

  2. The hard max limit seems arbitrary at 16? Limits are usually rounded at 10 or 100 or 1000
    image

Hey, 3 things about this patch 1. The tittle should only be Wire Width. since it's already under the shapekey panel, that part of the name is redundant ![image](/attachments/836ad05a-d3a1-4821-989b-22ae09079666) 2. When duplicating a bone, the custom width is not carried over, but all other custom shape settings are 3. The hard max limit seems arbitrary at 16? Limits are usually rounded at 10 or 100 or 1000 ![image](/attachments/19a4c10b-4e46-4e1e-9d0d-c207b5df2f1c)
1.7 KiB
4.4 KiB
Member

@ChrisLend @linen @dr.sybren Hi, I don't know if the above issues have reached you

@ChrisLend @linen @dr.sybren Hi, I don't know if the above issues have reached you

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.

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

@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

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

Fair enough, thank you for addressing the feedback

Fair enough, thank you for addressing the feedback
Sign in to join this conversation.
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
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.