Details

Reviewers

Hans Goudey (HooglyBoogly)
Campbell Barton (campbellbarton)
Julian Eisel (Severin)

Group Reviewers

User Interface

Commits

rBded9484925ed: UI: Viewport Navigate Gizmo Refactor

Summary

Simplification of the code used for the Navigate Gizmo. In a nutshell it goes from 623 lines down to 362, so about a 42% decrease. Unused code removed, more defines, improved comments, etc.

While there I changed the behavior substantially.

There is no longer any flashing when balls pass from behind to in front. Instead there is a smooth transition, from brighter when close to duller and darker when further away. They are also a bit bigger when closer and smaller in the back. And the negative balls become transparent when far away. Even the axis lines themselves smoothly transition along their lengths.

I've changed highlighting so it primarily affects the text color. And now it shows the negative axis names on hover.

While aligned it is showing all axis lines so it looks a bit more like the axis lines in the viewport. The middle ball always shows axis name, even if negative. The other negative axis will show on hover though.

Note that this changes the mouse cursor used here. Currently the mouse changes to a "+" (WM_CURSOR_EDIT) when hovering over this gizmo. But that type of cursor obscures the text (negative axis names) when hovering. So obscuring the very thing that is revealed. So this patch uses the normal "pointer" cursor when hovering as that is less obscuring because of its offset design.

You can also choose to make the Navigate Gizmo a little larger (or smaller) if you want, just as you can with the Simple Axis version:

Diff Detail

Repository: rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@Hans Goudey (HooglyBoogly) - I'm not sure about the change to the inactive circles. It makes the whole widget feel a bit more complicated than before. This behavior in particular distracting...

Could you elaborate on what part was distracting. The current code (and identically in this patch) has a change of coloring when an axis ball changes from behind to in front. And so so does look dumb right where they change. So I'm not sure if you are saying that my changes made that look worse or that you just don't like that behavior at all and it could change to something better.

The basic problem there is that we want to show the parts behind as being dimmer or diminished in some way to contrast with those in front. If those colors changed smoothly from back to front then you would not get that annoying sudden change at half-way. But that also means that aligned axes would be in an equal half-way color state. Maybe a solution to that is to just treat axis alignment separately. Will play with that idea.

This patch is mostly an excuse to get it into a shape where these kinds of changes are easier to make. I could have this refactor make no visible changes but that's not any fun.

Harley Acheson (harley) retitled this revision from UI: Viewport Navigate Gizmo Refactor to WIP: Viewport Navigate Gizmo Refactor.Dec 4 2020, 7:24 PM

Harley Acheson (harley) edited the summary of this revision. (Show Details)

Using a smaller font size, but in bold, for the axis balls. And also now properly scaling the entire gizmo based on a centrally-defined size. So changing that one define can now increase the size and have everything (including font and relationship to the nav buttons below) all scale properly.

In D9744#242298, @Harley Acheson (harley) wrote:

Could you elaborate on what part was distracting. The current code (and identically in this patch) has a change of coloring when an axis ball changes from behind to in front. And so so does look dumb right where they change. So I'm not sure if you are saying that my changes made that look worse or that you just don't like that behavior at all and it could change to something better.

The basic problem there is that we want to show the parts behind as being dimmer or diminished in some way to contrast with those in front. If those colors changed smoothly from back to front then you would not get that annoying sudden change at half-way. But that also means that aligned axes would be in an equal half-way color state. Maybe a solution to that is to just treat axis alignment separately. Will play with that idea.

This patch is mostly an excuse to get it into a shape where these kinds of changes are easier to make. I could have this refactor make no visible changes but that's not any fun.

I think it's mostly that the change between outline only and filled circle is a bit larger than the visual change we already get in master, which just makes those sudden changes a bit more visible. The feedback was a bit picky indeed and I don't feel strongly about it. It's the sort of thing that is much less noticeable when you're not laser focused on it for code review. Some sort of smooth transition could be nice if you figured out some way to do it.

This revision is now accepted and ready to land.Dec 4 2020, 8:41 PM

@Hans Goudey (HooglyBoogly) - I think it's mostly that the change between outline only and filled circle is a bit larger than the visual change we already get in master...feedback was a bit picky...

I did not think it was picky at all. Picky is always good anyway. I was just mostly wanting to understand what you saw. I could definitely decrease that contrast, but that whole thing will change if I can make that front-to-back transition smoother. It is the use of the rings that I think will allow me to finally address that. The only wrinkle is that those negative balls cannot have transparent middles when in front.

Will do some playing over the weekend. If I can't come up with something nice then I can always backtrack and make a patch that doesn't make any behavior changes.

BC (ChinoD) awarded a token.Dec 4 2020, 11:02 PM

BC (ChinoD) added a subscriber: BC (ChinoD).

BC (ChinoD) added a comment.Dec 4 2020, 11:07 PM

This comment was removed by BC (ChinoD).

@BC (ChinoD) - Maybe try having the transitions fade in and out instead of popping in and out.

Yes, will try. Transitioning nicely seems obvious, but there are some odd considerations about how it should look half-way between. But I'm hoping I can address this by treating axis alignment specially. Will see...

Now smoothly transitioning from front to back, changes to highlighting, etc. Will update the patch description above.

Harley Acheson (harley) retitled this revision from WIP: Viewport Navigate Gizmo Refactor to UI: Viewport Navigate Gizmo Refactor.Dec 5 2020, 3:13 AM

Harley Acheson (harley) edited the summary of this revision. (Show Details)

Harley Acheson (harley) edited the summary of this revision. (Show Details)Dec 5 2020, 3:15 AM

Harley Acheson (harley) edited the summary of this revision. (Show Details)Dec 5 2020, 4:40 AM

Raimund Klink (Raimund58) awarded a token.Dec 5 2020, 9:34 AM

Raimund Klink (Raimund58) added a subscriber: Raimund Klink (Raimund58).Dec 5 2020, 9:37 AM

Hans Goudey (HooglyBoogly) awarded a token.Dec 5 2020, 3:35 PM

Axis lines meet nicer at center. Better calculations of line and ring widths to allow scaling of entire gizmo, if ever desired.

Improved connection of axis ball color to connected line. Super subtle; nobody would notice but me.

Small changes to keep the sizes and visual weight about the same as we have currently. Changes to how all the navigate gizmos are scaled so that the Navigation (Axis) one can change independently properly.

Harley Acheson (harley) edited the summary of this revision. (Show Details)Dec 6 2020, 2:13 AM

Harley Acheson (harley) edited the summary of this revision. (Show Details)Dec 6 2020, 2:51 AM

Harley Acheson (harley) edited the summary of this revision. (Show Details)Dec 6 2020, 2:54 AM

In general this seems OK, I'm also not sure on inactive circles, will let others in the UI team decide though.

With this patch applied font's look a bit blurry, the minus symbol for -Y doesn't read very well:

[ 43%: -282] Building C object source/blender/editors/space_view3d/CMakeFiles/bf_editor_space_view3d.dir/view3d_gizmo_navigate.c.o
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_navigate.c: In function ‘WIDGETGROUP_navigate_draw_prepare’:
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_navigate.c:269:15: warning: unused variable ‘icon_size’ [-Wunused-variable]
  269 |   const float icon_size = GIZMO_SIZE;
      |               ^~~~~~~~~
[ 43%: -281] Building C object source/blender/editors/space_view3d/CMakeFiles/bf_editor_space_view3d.dir/view3d_gizmo_navigate_type.c.o
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_navigate_type.c: In function ‘gizmo_axis_cursor_get’:
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_navigate_type.c:346:43: warning: unused parameter ‘gz’ [-Wunused-parameter]
  346 | static int gizmo_axis_cursor_get(wmGizmo *gz)
      |                                  ~~~~~~~~~^~

source/blender/editors/space_view3d/view3d_gizmo_navigate_type.c
31	This should be added below low level headers like BLI (after GPU for e.g.).
77	Prefer `axis_opposite`, since it reads like it might be a boolean.
606–609	This isn't related to drawing. Although I don't have a strong opinion against it.

This revision now requires changes to proceed.Dec 7 2020, 4:19 AM

With this patch applied font's look a bit blurry, the minus symbol for -Y doesn't read very well.

I can't say that the font became blurry, it remained the same.
We can try using a 'minus' instead of a 'hyphen', it should be longer and centered.

The handles look too big for me, I'd prefer 0.85f - 1.0f.

Updated to address issues @Campbell Barton (campbellbarton).

I'm also not sure on inactive circles, will let others in the UI team decide though.

Just the use of the outline for negative axis? That part is important for this solution. The only two things I was really trying to address is the current use of dimming to mean both negative and farther away, and changes in size to indicate negative irrespective of distance. Using this circle for negative allows us to use both shade and size in ways that help orient in 3D.

With this patch applied font's look a bit blurry, the minus symbol for -Y doesn't read very well:

Although I haven't noticed the text looking blurrier, I do find this less than ideal: negative axis in the middle while aligned. There is less change between positive and negative. And we definitely want that clear in this orientation. That needs some thought.

Also less than ideal is my use of a roundbox_ex. That seems okay to have (and would also removal of redundancy in interface_draw.c), but for this patch I would have prefereer using shader GPU_SHADER_2D_POINT_UNIFORM_SIZE_UNIFORM_COLOR_OUTLINE_AA but couldn't figure out to use that without crashing.

This isn't related to drawing. Although I don't have a strong opinion against it.

I should have mentioned that. This became a requirement as soon as hover shows negative axis labels. The "+" cursor, with its content all around the hotspot would obscure the text that was exposed. The regular pointer doesn't do this because of how its shape is off-center to the hotspot.

@Yevgeny Makarov (jenkm) - The handles look too big for me, I'd prefer 0.85f - 1.0f.

The handles look too big in relation to the overall size of the widget? The handles look too large in proportion to the lines? Or is the change in size, from when they are smallest (further away) and when at their largest (in front) too great? or too small?

I'm guessing that you want both. For it to be a bit smaller and to vary less?

We can try using a 'minus' instead of a 'hyphen', it should be longer and centered.

Interesting idea.

Easier to differentiate negative from positive axis ball in the center when aligned.

Harley Acheson (harley) edited the summary of this revision. (Show Details)Dec 8 2020, 2:32 AM

Most issues are resolved now.

Minor notes.

Clang format needs to be run.
Since this is a very visible change, it'd be good to get sign-off from @Julian Eisel (Severin) before being committed.

source/blender/editors/space_view3d/view3d_gizmo_navigate_type.c
233	This is assigned and used after it goes out of scope, it should be declared in the same scope it's used.

This revision now requires changes to proceed.Dec 8 2020, 11:27 AM

Gilberto Rodrigues (gilberto_rodrigues) awarded a token.Dec 8 2020, 3:43 PM

Harley Acheson (harley) edited the summary of this revision. (Show Details)Dec 8 2020, 5:48 PM

Updated to incorporate changes requested by @Campbell Barton (campbellbarton)

Rang Clang Format
A variable scope issue

Since this is a very visible change, it'd be good to get sign-off from @Julian Eisel (Severin) before being committed.

Yes, for sure.

Thanks!!

Looked at this again, I'm not sure about the size of handles, maybe it's just looks unusual, but the text is definitely too transparent.

/* current */

float scale = ((depth + 1) * 0.075f) + 0.85f;

text_color[3] = is_active ? 1.0f : 0.9f;

Slight size changes, as per suggestions by @Yevgeny Makarov (jenkm)

Axis balls slightly smaller while text remaining the same size
Size varying by depth reduced a bit
Text keeps stronger color when inactive (when not hovering anywhere near)

Harley Acheson (harley) edited the summary of this revision. (Show Details)Dec 9 2020, 2:41 AM

Adding ability to set the size of the navigate gizmo in Preferences.

Harley Acheson (harley) edited the summary of this revision. (Show Details)Jan 1 2021, 3:00 AM

xan2622 (xan2622) added a subscriber: xan2622 (xan2622).Jan 1 2021, 3:00 PM

xan2622 (xan2622) awarded a token.Jan 2 2021, 11:48 AM

slwk1d (Slowwkidd) awarded a token.Jan 2 2021, 1:40 PM

Rahul Chaudhary (RC12) awarded a token.Jan 2 2021, 2:06 PM

Harley Acheson (harley) added a project: BF Blender (2.93).Jan 4 2021, 4:20 AM

Maged afra (Maged_afra) awarded a token.Jan 4 2021, 4:35 AM

slwk1d (Slowwkidd) added a subscriber: slwk1d (Slowwkidd).Jan 9 2021, 3:03 PM

This comment was removed by Campbell Barton (campbellbarton).

Silas Opel (Schamph) awarded a token.Jan 9 2021, 3:48 PM

TheRedWaxPolice (TheRedWaxPolice) added a subscriber: TheRedWaxPolice (TheRedWaxPolice).Jan 9 2021, 4:40 PM

This comment was removed by Campbell Barton (campbellbarton).

Yevgeny Makarov (jenkm) added a comment.Jan 9 2021, 4:43 PM

This comment was removed by Campbell Barton (campbellbarton).

slwk1d (Slowwkidd) added a comment.Jan 10 2021, 1:19 PM

This comment was removed by Campbell Barton (campbellbarton).

Reminder not to have off topic discussion on patches.

Minor name change request, otherwise looks good.

source/blender/makesdna/DNA_userdef_types.h
777	We might have other gizmos sizes in the future, prefer made the name on the existing `gizmo_size` name, eg: `gizmo_size_navigate_v3d`. Also, this can be a char.