Details

Reviewers

Commits

rB288e7d0af0c2: Tool Icons: Support curve and objects with modifiers + color space fix

Summary

This allows for the new tool icons to use geometry nodes.

In order to do this I also had to use the new API for accessing the attributes (instead of vertex colors). Which in turn requires a few changes to use linear color space.

I went ahead and updated the entire code to use the linear space everywhere. I will update the icon files manually to make sure the final result is similar to what we have now.

Note: We now use round instead of int. That plus the changes regarding the color space will lead to some icons to change slightly (no perceived visual change).

Diff Detail

Repository: rB Blender

Event Timeline

Dalai Felinto (dfelinto) requested review of this revision.May 19 2022, 7:36 PM

Dalai Felinto (dfelinto) created this revision.

Harbormaster completed remote builds in B22166: Diff 51630.May 19 2022, 7:36 PM

Dalai Felinto (dfelinto) retitled this revision from Tool Icons: Support curve and obejcts with modifiers to WIP: Tool Icons: Support curve and objects with modifiers.May 19 2022, 9:52 PM

Dalai Felinto (dfelinto) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) requested changes to this revision.May 20 2022, 1:09 PM

Campbell Barton (campbellbarton) added inline comments.

release/datafiles/blender_icons_geom.py
75	In `main()` all non-mesh objects are skipped. If other types are to be supported an set of object-types could be declared globally and compared against.
77	Wouldn't it be better get the evaluated objects early on, where `objects_source` is assigned, then there is no need to draw a distinction in nested functions.

This revision now requires changes to proceed.May 20 2022, 1:09 PM

Campbell Barton (campbellbarton) added inline comments.May 20 2022, 1:10 PM

release/datafiles/blender_icons_geom.py
124	`ob.data.vertex_colors` should be changed in `main()` too.

From review: Use all objects evaluated, and use attributes everywhere
Use the new color conversion API

Harbormaster completed remote builds in B22194: Diff 51689.May 20 2022, 4:51 PM

Dalai Felinto (dfelinto) retitled this revision from WIP: Tool Icons: Support curve and objects with modifiers to Tool Icons: Support curve and objects with modifiers.May 20 2022, 4:55 PM

Dalai Felinto (dfelinto) edited the summary of this revision. (Show Details)

Dalai Felinto (dfelinto) added inline comments.

release/datafiles/blender_icons_geom.py
75	I know, this was kind of by design since we stick to have a mesh for all the "parent" objects. But you are right, there is no reason to not support curves there as well.
169–170	We could use `round` instead of `int` here.

Dalai Felinto (dfelinto) removed a reviewer: Hans Goudey (HooglyBoogly).May 20 2022, 4:56 PM

Rebase with master and use the landed color management API

Harbormaster completed remote builds in B22224: Diff 51757.May 23 2022, 4:27 PM

Generally this patch seems fine, I noticed the material color (which I assume is in linear space) is being mixed with the sRGB color without being converted to sRGB as well.

We should mix the colors in the same color-space before converting them to sRGB.

release/datafiles/blender_icons_geom.py
75	`{'CURVE', 'MESH'}` can be made into a variable as it's used twice `OBJECT_TYPES_MESH_COMPATIBLE` for e.g.
81–82	Call `ob.to_mesh_clear()` before the function exits.
169–170	In that case better do now, as doing so will rewrite icons. I don't see any issues with making the change as part of this patch.
169–170	`base_color` is currently linear, but should be really be converted to `sRGB` too (or multiply the colors in linear space, then convert the result to sRGB).
184

From review: multiple changes

Note: I still haven't changed base color to be converted to sRGB as well

Harbormaster completed remote builds in B22245: Diff 51790.May 24 2022, 2:39 PM

Addressed all but:

"base_color is currently linear, but should be really be converted to sRGB too (or multiply the colors in linear space, then convert the result to sRGB)."

I need to think a bit further about that.

Refactor: Mix colors in linear space

Harbormaster completed remote builds in B22253: Diff 51819.May 25 2022, 10:16 AM

Patch is final, while waiting for a final review I will try to find a wait to adjust the icons file to adjust the colors so they yield similar results. Right now the change to treat base color as linear space is visually introducing too many changes.

Accepting although requesting some minor changes (no need for an additional review pass).

release/datafiles/blender_icons_geom.py
170–175	`# RGBA color.` comment cant be kept.
181	Name is overly vague: `color_multiply_and_from_linear_to_srgb` (although no strong opinion on the name).
191	This returns a generator which would cause problems if accessed more than once, better use a tuple here.

This revision is now accepted and ready to land.May 25 2022, 2:01 PM

Dalai Felinto (dfelinto) edited the summary of this revision. (Show Details)May 25 2022, 2:18 PM

Dalai Felinto (dfelinto) retitled this revision from Tool Icons: Support curve and objects with modifiers to Tool Icons: Support curve and objects with modifiers + color space fix.

Dalai Felinto (dfelinto) edited the summary of this revision. (Show Details)