Viewport: draw light icons using the light colors #105236

Weizhen Huang · 2023-02-27T11:27:10+01:00

Weizhen Huang commented

2023-02-27 11:27:10 +01:00

Indicating light colors by coloring the light icons. Ref: #104280

Added a checkbox under overlay extra to enable/disable light coloring. Disabled by default.
The outer ring and the sun rays are colored.
Only the base color is shown, colors in nodes are not considered.

Indicating light colors by coloring the light icons. Ref: #104280 * Added a checkbox under overlay extra to enable/disable light coloring. Disabled by default. * The outer ring and the sun rays are colored. * Only the base color is shown, colors in nodes are not considered. ![image](/attachments/7d4592cf-c52e-4762-87aa-a029249ee30f)

image.png

244 KiB

🎉 4 🚀 2

Weizhen Huang added the

Module

EEVEE & Viewport

label 2023-02-27 11:27:10 +01:00

Weizhen Huang added this to the EEVEE & Viewport project 2023-02-27 11:27:33 +01:00

Weizhen Huang requested review from Andy Goralczyk 2023-02-27 11:29:18 +01:00

Jeroen Bakker requested review from Jeroen Bakker 2023-02-28 04:17:50 +01:00

Jeroen Bakker reviewed 2023-02-28 04:22:06 +01:00

source/blender/draw/engines/overlay/overlay_extra.cc

						
				@ -639,0 +645,4 @@

				  const bool show_light_colors = vedata->stl->pd->overlay.flag & V3D_OVERLAY_SHOW_LIGHT_COLORS;

				  if (show_light_colors) {

				    /* Get light color. */

				    PointerRNA lamp_ptr;

Jeroen Bakker commented

2023-02-28 04:22:06 +01:00

In drawing (where performance matters) we don't use RNA. You can just read the data directly from the la.

In drawing (where performance matters) we don't use RNA. You can just read the data directly from the `la`.

weizhen marked this conversation as resolved

Jeroen Bakker reviewed 2023-02-28 04:30:19 +01:00

source/blender/draw/engines/overlay/overlay_private.hh

						
				@ -178,6 +178,7 @@ typedef struct OVERLAY_ExtraCallBuffers {

				  DRWCallBuffer *groundline;

				  DRWCallBuffer *light_icon;

Jeroen Bakker commented

2023-02-28 04:30:19 +01:00

I know this is just a WIP ticket and you can apply these changes whenever it's appropriate.

perhaps light_center should be a better description. (also for the other related methods).

I know this is just a WIP ticket and you can apply these changes whenever it's appropriate. perhaps `light_center` should be a better description. (also for the other related methods).

Weizhen Huang commented

2023-02-28 16:36:08 +01:00

I am following the comment I deleted for drawing the part, it was /* Light Icon */. Now we draw the inner circles and the outer circle separately, it's weird to call all of them "center". If "icon" is not a good name, I could probably change them to DRW_cache_light_inner_circles_get(), DRW_cache_light_outer_circle_get()? Then I will also adjust it in the commit message.

I am following the comment I deleted for drawing the part, it was `/* Light Icon */`. Now we draw the inner circles and the outer circle separately, it's weird to call all of them "center". If "icon" is not a good name, I could probably change them to `DRW_cache_light_inner_circles_get()`, `DRW_cache_light_outer_circle_get()`? Then I will also adjust it in the commit message.

Weizhen Huang reviewed 2023-02-28 16:25:33 +01:00

source/blender/makesrna/intern/rna_space.c

						
				@ -4402,6 +4402,11 @@ static void rna_def_space_view3d_overlay(BlenderRNA *brna)

				      prop, "Extras", "Object details, including empty wire, cameras and other visual guides");

				  RNA_def_property_update(prop, NC_SPACE | ND_SPACE_VIEW3D, NULL);

				  prop = RNA_def_property(srna, "show_light_colors", PROP_BOOLEAN, PROP_NONE);

Weizhen Huang commented

2023-02-28 16:25:33 +01:00

I am not sure if coloring the light should be default. Currently it is disabled by default.

I am not sure if coloring the light should be default. Currently it is disabled by default. ![image](/attachments/adcd1e95-c342-4367-9a23-a94963a9804b)

Weizhen Huang reviewed 2023-02-28 16:28:43 +01:00

source/blender/draw/intern/draw_cache.c

						
				@ -1513,0 +1557,4 @@

				  return SHC.drw_light_point_lines;

				}

				GPUBatch *DRW_cache_light_sun_lines_get(void)

Weizhen Huang commented

2023-02-28 16:28:43 +01:00

Now this poor function only draws the light direction, not sure if we should rename it. I was considering adding a function that only draws the light direction because it is shared by other light types, but then other light types also draw clip start and end, so they are different.

Clément Foucault commented

2023-02-28 17:02:51 +01:00

Maybe DRW_cache_light_sun_rays_get is more descriptive. I think it is ok to leave it only draw the rays.

Maybe `DRW_cache_light_sun_rays_get` is more descriptive. I think it is ok to leave it only draw the rays.

Weizhen Huang commented

2023-02-28 17:28:57 +01:00

The other function DRW_cache_light_icon_sun_rays_get() draw the rays. (I could delete the word "icon" if it's inappropriate).
What's left of this DRW_cache_light_sun_lines_get() only draws the direction line.

The other function `DRW_cache_light_icon_sun_rays_get()` draw the rays. (I could delete the word "icon" if it's inappropriate). What's left of this `DRW_cache_light_sun_lines_get()` only draws the direction line.

Weizhen Huang changed title from ~~WIP: draw light icon using the light color~~ to draw light icon using the light color

2023-02-28 16:41:53 +01:00

Weizhen Huang changed title from ~~draw light icon using the light color~~ to Viewport: draw light icons using the light colors

2023-02-28 16:43:25 +01:00

Weizhen Huang force-pushed colorful_light_icon from 3ddb17c59e to e375b9714b

2023-02-28 16:45:11 +01:00

Compare

Weizhen Huang requested review from Clément Foucault 2023-02-28 16:45:45 +01:00

Weizhen Huang requested review from Julian Eisel 2023-02-28 16:46:09 +01:00

Clément Foucault reviewed 2023-02-28 17:08:15 +01:00

source/blender/draw/engines/overlay/overlay_extra.cc

						
				@ -639,0 +643,4 @@

				  float light_color[4] = {1.0f};

				  const bool show_light_colors = vedata->stl->pd->overlay.flag & V3D_OVERLAY_SHOW_LIGHT_COLORS;

				  if (show_light_colors) {

				    copy_v3_v3(light_color, &la->r);

Clément Foucault commented

2023-02-28 17:08:05 +01:00

I am unsure about using the light color as is. I think it should be normalized.

For instance, a light with color (0.0, 0.01, 0.0) at really high power will still give a powerful green.

I am unsure about using the light color as is. I think it should be normalized. For instance, a light with color `(0.0, 0.01, 0.0)` at really high power will still give a powerful green.

Weizhen Huang commented

2023-02-28 17:34:02 +01:00

Not sure about getting into such details, because different light types with the same power could have quite different perceptual strength, it also depends on the size/radius and the spread of the light, a lot of things need to be considered.. we could do this in another commit if needed.

Clément Foucault commented

2023-02-28 18:26:54 +01:00

Maybe there is a misunderstanding: I'm not asking about taking light perceptual intensity into account, I'm saying that the color should be normalized as it is the "tint" we are interested in for display. Having a nearly black light overlay makes zero sense to me.

I'm just asking to replace copy_v3_v3(light_color, &la->r); by normalize_v3_v3(light_color, &la->r);.

Maybe there is a misunderstanding: I'm not asking about taking light perceptual intensity into account, I'm saying that the color should be normalized as it is the "tint" we are interested in for display. Having a nearly black light overlay makes zero sense to me. I'm just asking to replace `copy_v3_v3(light_color, &la->r);` by `normalize_v3_v3(light_color, &la->r);`.

Andy Goralczyk commented

2023-03-07 14:48:33 +01:00

For the record: Today I discussed this with @weizhen and @fclem and we decided to go with the non-normalized display for the time being. If many users struggle to find their lights, we can still look into normalization (or other ways to make the overlay more visible).

👍 1

Clément Foucault approved these changes 2023-03-07 14:51:44 +01:00

Clément Foucault left a comment

Talked IRL to Andy about the pros and cons of each approach.
The outcome is that the current behavior (not normalized) is more intuitive and can be used as a way to replace a "custom light overlay color" without having to set a different property. It isn't quite correct but it is practical.

We might tweak the behavior after the initial implementation.

We also discussed about making the lines bigger. We could do that by doubling the lines a bit like the armatures axes points are using tons of lines for their axis end dot.

Anyway the patch is good as it is. 👍

Talked IRL to Andy about the pros and cons of each approach. The outcome is that the current behavior (not normalized) is more intuitive and can be used as a way to replace a "custom light overlay color" without having to set a different property. It isn't quite correct but it is practical. We might tweak the behavior after the initial implementation. We also discussed about making the lines bigger. We could do that by doubling the lines a bit like the armatures axes points are using tons of lines for their axis end dot. Anyway the patch is good as it is. 👍

Andy Goralczyk approved these changes 2023-03-07 14:57:05 +01:00

Weizhen Huang force-pushed colorful_light_icon from e375b9714b to 4508c6fc6e

2023-03-07 15:02:24 +01:00

Compare

Jeroen Bakker approved these changes 2023-03-07 15:06:12 +01:00

Weizhen Huang force-pushed colorful_light_icon from 4508c6fc6e to aa4201dc7f

2023-03-07 15:06:37 +01:00

Compare

Weizhen Huang referenced this issue from a commit

2023-03-07 15:18:12 +01:00

Viewport: draw light icons using the light colors

Weizhen Huang commented

2023-03-07 15:19:44 +01:00

Committed as 6fbc52bdca

Committed as 6fbc52bdca3b38872c178dbafe0abc92328927e3

Weizhen Huang closed this pull request

2023-03-07 15:19:45 +01:00

Weizhen Huang deleted branch colorful_light_icon

2023-03-07 15:19:57 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Viewport: draw light icons using the light colors #105236

Pull request closed