Details

Reviewers

Hans Goudey (HooglyBoogly)

Group Reviewers

User Interface

Commits

rB675807e2b6b2: UI: Add grid-related theme options

Summary

As part of this right-click select post, this adds some new theme colors related to grid rendering:

Add grid theme color for node editor
- Call UI_view2d_multi_grid_draw with TH_GRID instead of a shaded TH_BACK, and color-blend TH_NODE_GROUP
Movie Clip Editor's clip preview grid respects grid theme color
- Make ED_region_grid_draw use color-blended TH_GRID
Add versioning code to allow fixing existing themes (the resulting themes should visually look the same as before)

These changes did cause some inconsistencies, even after adjusting the themes accordingly:

In the movie clip editor, the alpha slider of the grid color affects the *background* and not the grid lines itself.
In the movie clip editor, the grids used by graph and dopesheet mode could already be themed in the past. Now that the clip preview's grid can also be themed, two completely different modes share the same theme color.

TODO after this diff: Apply D8874 as well, so the add-on themes don't look out-of-place

Diff Detail

Repository

rB Blender

Branch

more-theme-options (branched from master)

Build Status

Buildable 9806
Build 9806: arc lint + arc unit

Event Timeline

RedMser (RedMser) requested review of this revision.Aug 24 2020, 10:34 PM

RedMser (RedMser) created this revision.

Harbormaster completed remote builds in B9764: Diff 28109.Aug 24 2020, 10:35 PM

RedMser (RedMser) edited the summary of this revision. (Show Details)Aug 24 2020, 10:39 PM

RedMser (RedMser) added a reviewer: User Interface.

RedMser (RedMser) edited the summary of this revision. (Show Details)

Julian Eisel (Severin) added a project: User Interface.Aug 25 2020, 5:17 PM

Overall to me is seems nice to have these colors defined a bit more explicitly, and I could see the use case for when people want the grid to pop out more. (Although I don't actually know how common that is).
However, I know some UI module members are more against adding theme colors. It's pretty clear that the theme system is far from optimal, and adding more colors sort of expands the mess. Just something to keep in mind.

If you define the grid color RNA property lower down in the functions it won't appear first in the list. It probably shouldn't be at the top.

In the image/UV editor, the alpha slider of the grid color affects the *background* and not the grid lines itself. Should the grid background be a separate theme color instead, and remove the alpha channel from the grid theme color?

If the goal is making the grid theming consistent, that would probably be a good change. Again though, there are downsides to adding more theme colors. More opinions needed here I guess.

It should maybe be blended with the node group color as well?

I think that makes sense.

Should they maybe be split into two separate theme colors (e.g. "Timeline Grid" and "Clip Grid")?

IMO this isn't worth the additional theme color.

The file browser changes should probably be a separate patch, they're pretty unrelated to the rest of the changes.

source/blender/blenloader/intern/versioning_userdef.c
220	It might make more sense to just use `FROM_DEFAULT_V4_UCHAR` on all the changed colors? Certainly simpler.
source/blender/editors/screen/area.c
3573–3574	If there is a theme color specifically for the grid, it shouldn't be shaded.

This revision now requires changes to proceed.Aug 25 2020, 5:34 PM

I will work on the requested changes tomorrow.

I could see the use case for when people want the grid to pop out more. (Although I don't actually know how common that is).

My experimentation showed that, on pure black or white backgrounds, the grids in these editors don't show up at all. Allowing to theme them is the simplest way to make accessible themes that use high-contrast colors possible.

It probably shouldn't be at the top.

It's the very topmost color for all other editors that already define a grid color (such as NLA, graph, 3dview). Just didn't want to mess with the rna_def_userdef_theme_spaces_* function call order. But I can move it down further.

The file browser changes should probably be a separate patch, they're pretty unrelated to the rest of the changes.

Fair enough. ~~Do I have to bump file subversion another time then, or will that be done before committing?~~ Actually I just realized, these changes probably shouldn't bump subversion yet, so I'll move them to the block below. (I assume that's how the versioning code works? Haven't found any documentation on the subversion workflow.)

source/blender/blenloader/intern/versioning_userdef.c
220	I tried to make it so user-made themes would look the same before and after this diff. Since Blender in the past used the (shaded) window background color for the grid, carrying that theme color over to the grid provides the result closest to the original. So if the user had for example a red window background for image editor (with a bright-red grid), it wouldn't reset to the default theme color (which would be gray). But I can definitely simplify this code by using `copy_v3_v3_uchar`.
source/blender/editors/screen/area.c
3573–3574	Since this is the color for the grid background, I found it most intuitive that the grid theme color defines the color of the grid lines, and the background is adjusted accordingly (also makes theme compatibility better). But I could alternatively offset everything, so the background uses the theme color without shading, and the fine/coarse grid lines get a darker version of that color instead?

Matt (Kickflipkid687) awarded a token.Aug 25 2020, 6:33 PM

In D8699#212371, @RedMser (RedMser) wrote:

It's the very topmost color for all other editors that already define a grid color (such as NLA, graph, 3dview). Just didn't want to mess with the rna_def_userdef_theme_spaces_* function call order. But I can move it down further.

You're right, I didn't notice this! In that case stay consistent I guess.

Fair enough. ~~Do I have to bump file subversion another time then, or will that be done before committing?~~ Actually I just realized, these changes probably shouldn't bump subversion yet, so I'll move them to the block below. (I assume that's how the versioning code works? Haven't found any documentation on the subversion workflow.)

If the subversion isn't bumped, the version code runs every time Blender starts I think. (for file versioning it's whenever a file is loaded). But it's fine to leave it out of the patch, it can be done when committing it if it's necessary.

Remove file browser changes
Replace ED_region_grid_draw alpha-blending with color-blending of TH_BACK (visually the same result, just less expensive to compute)
Undo subversion bump (this should probably be done later)
Clean up versioning code a bit
Inspecting node groups now blends the grid color with the node group color, to lower the grid contrast
Fix a bug with the implementation of UI_GetThemeColorBlendShade3ubv (which was only used in one other place prior to me, probably why this bug wasn't found sooner). If there's a nicer way to fix it, let me know since I'm not sure
Move new grid RNA properties to the very top of the theme colors list, to be consistent with the existing ones

Harbormaster completed remote builds in B9806: Diff 28182.Aug 26 2020, 3:27 PM

Looking pretty good I think, a few comments though.

Personally I'm not sure about the visual change in the node editor. I see how it could be better for accessible themes, but by default I'm not sure the change is a plus.

I see why this is sort of hard to fix though, without adding yet another color.

IMO, the main thing to consider is that we only want to introduce visual changes where we really mean to.

source/blender/editors/interface/view2d.c
1344	The `Blend` seems a bit odd, and was a bit confusing at first because 3/4 of the time it's blender with itself if you look at what the `colorid` argument is set to.
source/blender/editors/screen/area.c
3573	Try to follow the comment style guide, they should start with a capital and end with a period.
3573–3574	Ah yes, I underestimated the complexity of trying to drive all of those colors from one theme color. Maybe a bit of a fraught exercise to begin with... I think the way you've done it is probably be best. Arguably the blending between the grid and the back isn't necessary, but it's sort of nice. I think the lighter color of the outer lines was lost though:

In D8699#214198, @Hans Goudey (HooglyBoogly) wrote:

Personally I'm not sure about the visual change in the node editor. I see how it could be better for accessible themes, but by default I'm not sure the change is a plus.

I will experiment with the blending factor, maybe we can find a compromise that isn't as high contrast on default theme but still allows for accessible themes, without introducing any more new settings.

Maybe introduce an alpha channel to the grid color, which replaces the current blend factor of 0.5f? (UI_view2d_multi_grid_draw would then be called with TH_BACK instead of TH_GRID)

IMO, the main thing to consider is that we only want to introduce visual changes where we really mean to.

I personally think that the node group grid in current Blender is practically invisible (especially the major grid lines), doesn't look like it's intended design to the end user:

source/blender/editors/interface/view2d.c
1344	Yeah, I personally would change the `colorid` parameter to something like `bool is_in_nodegroup` and only blend if that's set to true. Or check if `colorid == TH_GRID` and decide based on that.

Re-add bright border around image editor grid (before/after this patch now look exactly the same with the default theme)
Reduce node grid blending, to stay closer to how it was before, but still increasing contrast of the grid when in a node group compared to pre-patch

Harbormaster completed remote builds in B10143: Diff 28785.Sep 11 2020, 7:51 PM

RedMser (RedMser) marked an inline comment as done.Sep 11 2020, 7:52 PM

From the code and your descriptions this seems good. I'd like to test it one more time though.

Do you mind merging master into your branch? The patch doesn't apply cleanly for me now.

Also getting an RNA error when building:

ERROR (rna.define): rna_define.c:1301 RNA_def_property: duplicate identifier "ThemeImageEditor.grid"

In D8699#217495, @Hans Goudey (HooglyBoogly) wrote:
Do you mind merging master into your branch? The patch doesn't apply cleanly for me now.

Also getting an RNA error when building:
ERROR (rna.define): rna_define.c:1301 RNA_def_property: duplicate identifier "ThemeImageEditor.grid"

I did not see D8234 got merged into master. It changes the Image Editor's rendering completely, and it *also* introduces a Grid theme color for the Image Editor.

Rebase
Adjust according to changes caused by D8234
- Since the new draw mode is enabled by default in master, the theme color should be reset to default, since the grid color is no longer tied to TH_BACK there (and the new Image Editor looks pretty different anyway)
  - As a result of this, using the legacy Image Editor will show a brighter grid than before, but users will only see this if they change the experimental option
- The Movie Clip Editor did not get the updated draw mode, so for now its changes from this patch will stay (same for the Node Editor of course)

Harbormaster completed remote builds in B10144: Diff 28791.Sep 12 2020, 10:59 AM

Re-add versioning for node grid (I accidentally dropped it from an earlier refactor)

Harbormaster completed remote builds in B10145: Diff 28793.Sep 12 2020, 11:28 AM

RedMser (RedMser) added a child revision: D8874: UI: Update add-on themes with new grid colors.Sep 12 2020, 11:34 AM

RedMser (RedMser) edited the summary of this revision. (Show Details)

I tested this again and it works well! Before I commit, if you'd like to make sure everything in the patch description is correct that would be great.
Particularly for the first bullet point, I don't think it's so important to mention how changes affect the legacy image editor drawing since it will be removed soon.

This revision is now accepted and ready to land.Sep 14 2020, 8:11 PM

RedMser (RedMser) edited the summary of this revision. (Show Details)Sep 14 2020, 8:46 PM

Closed by commit rB675807e2b6b2: UI: Add grid-related theme options (authored by Red Mser <RedMser>, committed by Hans Goudey (HooglyBoogly)). · Explain WhySep 16 2020, 1:43 AM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB675807e2b6b2: UI: Add grid-related theme options.

UI: Add grid-related theme options
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 28182

release/datafiles/userdef/userdef_default_theme.c

release/scripts/presets/interface_theme/blender_light.xml

source/blender/blenloader/intern/versioning_userdef.c

source/blender/editors/interface/resources.c

source/blender/editors/interface/view2d.c

source/blender/editors/screen/area.c

source/blender/editors/space_image/image_draw.c

source/blender/editors/space_node/node_draw.c

source/blender/makesrna/intern/rna_userdef.c

UI: Add grid-related theme optionsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 28182

release/datafiles/userdef/userdef_default_theme.c

release/scripts/presets/interface_theme/blender_light.xml

source/blender/blenloader/intern/versioning_userdef.c

source/blender/editors/interface/resources.c

source/blender/editors/interface/view2d.c

source/blender/editors/screen/area.c

source/blender/editors/space_image/image_draw.c

source/blender/editors/space_node/node_draw.c

source/blender/makesrna/intern/rna_userdef.c

UI: Add grid-related theme options
ClosedPublic
Actions

Revision Contents
Changeset List