Page MenuHome

UI: Add grid-related theme options
ClosedPublic

Authored by RedMser (RedMser) on Mon, Aug 24, 10:34 PM.

Details

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

Event Timeline

RedMser (RedMser) requested review of this revision.Mon, Aug 24, 10:34 PM
RedMser (RedMser) created this revision.
RedMser (RedMser) edited the summary of this revision. (Show Details)Mon, Aug 24, 10:39 PM
RedMser (RedMser) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) requested changes to this revision.Tue, Aug 25, 5:34 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
228

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
3667–3668

If there is a theme color specifically for the grid, it shouldn't be shaded.

This revision now requires changes to proceed.Tue, Aug 25, 5:34 PM
RedMser (RedMser) added a comment.EditedTue, Aug 25, 6:25 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
228

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
3667–3668

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?

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.

RedMser (RedMser) edited the summary of this revision. (Show Details)
  • 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

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–1346

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
3667–3668

Try to follow the comment style guide, they should start with a capital and end with a period.

3667–3668

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:

RedMser (RedMser) planned changes to this revision.Tue, Sep 1, 10:56 PM
RedMser (RedMser) marked 2 inline comments as done.

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–1346

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.

RedMser (RedMser) marked 2 inline comments as done.
RedMser (RedMser) edited the summary of this revision. (Show Details)
  • 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

RedMser (RedMser) marked an inline comment as done.Fri, Sep 11, 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"
RedMser (RedMser) planned changes to this revision.EditedSat, Sep 12, 10:29 AM

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.

RedMser (RedMser) edited the summary of this revision. (Show Details)
  • 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)
RedMser (RedMser) updated this revision to Diff 28793.EditedSat, Sep 12, 11:28 AM
  • Re-add versioning for node grid (I accidentally dropped it from an earlier refactor)

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.Mon, Sep 14, 8:11 PM
RedMser (RedMser) edited the summary of this revision. (Show Details)Mon, Sep 14, 8:46 PM
This revision was automatically updated to reflect the committed changes.