Page MenuHome

Viewport normal drawing with constant length
ClosedPublic

Authored by Piotr Makal (pmakal) on Jun 3 2021, 6:57 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Piotr Makal (pmakal) requested review of this revision.Jun 3 2021, 6:57 PM
Piotr Makal (pmakal) created this revision.

One question I have is about UI representation of toggle button to turn on/off this behavior. In this patch I implemented checkbox with text, but there is also possibility of less cluttered icon toggle button to the right of the normals size slider.

Clément Foucault (fclem) requested changes to this revision.Jun 3 2021, 7:29 PM

One question I have is about UI representation of toggle button to turn on/off this behavior. In this patch I implemented checkbox with text, but there is also possibility of less cluttered icon toggle button to the right of the normals size slider.

I like the clutter less approach. But screen space is a bit too technical even for a tooltip. I would suggest Constant Screen Size or Constant Screen Length. I'm quite bad at naming myself. Maybe @Campbell Barton (campbellbarton) (because it touches the modeling) , @Brecht Van Lommel (brecht) or @Pablo Vazquez (pablovazquez) have a better idea.

source/blender/draw/engines/overlay/shaders/edit_mesh_normal_vert.glsl
59

Instead of this, I would create a new value for the screen space scale and make it a PROP_PIXEL value in DNA.

This way toggling between both does not become anoying.

This revision now requires changes to proceed.Jun 3 2021, 7:29 PM

"Constant Screen Size" seems reasonable. Would not use "length" since it's already named "size" and that's the term we use for other viewport preferences.

I think an icon is preferable, but maybe the UI team can help find the best icon to use or add a new one.

Piotr Makal (pmakal) updated this revision to Diff 38111.EditedJun 10 2021, 6:53 AM
Piotr Makal (pmakal) edited the summary of this revision. (Show Details)
  • Improved naming for code and UI
  • Changed UI to be less cluttered (icon used in this patch is not a final solution though)
Piotr Makal (pmakal) marked an inline comment as done.EditedJun 10 2021, 7:19 AM

Regarding the icon
I created my version of an icon to be used in this patch, unfortunately I didn't manged to include it as whenever I saved the blender_icons.svg file my Inkscape version was changing a lot of stuff all over the place in the SVG/XML text file and I think this was affecting the blender_icons_update.py script as it wasn't adding a new *.dat file for the icon but also was changing some other random ones.

First things first - UI/UX
@Julian Eisel (Severin), can somebody from the UI team can give "go/no go" for this UI/UX approach presented in this patch (video showing it is in description of this diff)? If this solution will have the blessing from the UI team then I would propose to submit the icon in the separate diff (before this one). Of course as discussed with @Julian Eisel (Severin) this new icon would need to be accepted by @Andrzej Ambroz (jendrzych), but we can talk about the details in private. My current version looks like this (in the center):

Regarding the design: Personally I too prefer having it as an icon, simply to make it take less space. Although it's certainly useful, it's not a vital option, so having it nicely compact makes sense.

@Piotr Makal (pmakal) yeah, unfortunately Inkscape always does change a lot of stuff in the SVG. This stuff can be manually removed from the patch (e.g. by only adding relevant bits to the commit using git add -p), but it's a hassle. And it's not always clear what's related and what not... (usually it's harmless though, it's just a bit of noise in the patch.)
Also unfortunately, there seem to be version differences in Inkscape that generate different .dat files. Just use git restore path/to/a/file.dat for the unrelated ones.

Hey - this button cries for more generic and universal "fixed size" icon, that can be easily reused enywhere else in the GUI.

(just ignore the right one - it's way to similar to an "X"=delete)

Clément Foucault (fclem) requested changes to this revision.Jun 11 2021, 2:45 PM

The patch is good feature wise. I just have a few style comment.

source/blender/draw/engines/overlay/shaders/edit_mesh_normal_vert.glsl
58–59

Do not put comment this ways.
Either put it in the matching branch or do the following (widely used in the codebase):

bool is_persp = (ProjectionMatrix[3][3] == 0.0);
if (is_persp) {
source/blender/makesdna/DNA_view3d_types.h
559

Rename to V3D_OVERLAY_EDIT_CONSTANT_SCREEN_SIZE_NORMALS.
Even if it's longer, it is better than using acronyms that are not widely used throughout the codebase.

source/blender/makesrna/intern/rna_space.c
4369

Rename to normals_constant_screen_size. This way it is easier to find with auto complete.

This revision now requires changes to proceed.Jun 11 2021, 2:45 PM
Piotr Makal (pmakal) edited the summary of this revision. (Show Details)
  • Improved naming convention
  • Added proper icon

This revision is now accepted and ready to land.Jun 13 2021, 11:48 PM
  • Merged to latest master
  • Cleanup: Renamed normals_screen_length to normals_constant_screen_size.
  • Add versioning code for normals_constant_screen_size.