Add panels to Principled BSDF node #112314

Also, my time zone is roughly 12 hours out from Amsterdam. So if there's a small adjustment you want to make, feel free to make the change and just commit it.

I'm not sure if you want to wait for a fix for #112233 to be committed first before committing any UI changes for built in nodes.

Here's a look at how the layout compares.

Old layout	New layout (Fully Expanded)	New Layout (Collapsed)

Feel free to add any other reviewers that you think should be involved (Pablo Vazquez? Dalai Felinto? Members of the EEVEE or UI team?). Also, my time zone is roughly 12 hours out from Amsterdam. So if there's a small adjustment you want to make, feel free to make the change and just commit it. --- I'm not sure if you want to wait for a fix for #112233 to be committed first before committing any UI changes for built in nodes. --- Here's a look at how the layout compares. | Old layout | New layout (Fully Expanded) | New Layout (Collapsed) | | - | - | - | | ![Old.png](/attachments/65a6e410-d8eb-4e42-9d7f-45e6744b129f) | ![New - Expanded.png](/attachments/0571ac6e-0363-4d16-ae6f-6184edc7173c) | ![New - Collapsed.png](/attachments/da3e932e-5aa9-45f6-b583-440169268adc) |

Old.png

191 KiB

New - Expanded.png

204 KiB

New - Collapsed.png

337 KiB

Alaska requested review from Brecht Van Lommel 2023-09-13 09:06:38 +02:00

Alaska requested review from Lukas Stockner 2023-09-13 09:06:55 +02:00

Alaska requested review from Weizhen Huang 2023-09-13 09:07:07 +02:00

Alaska added 1 commit 2023-09-13 09:17:55 +02:00

Remove code comments that was causing crashing 72186f949a

Iliya Katushenock added

and removed

labels 2023-09-13 12:08:07 +02:00

Lukas Stockner approved these changes 2023-09-13 13:47:45 +02:00

Lukas Stockner left a comment

LGTM!

Brecht Van Lommel commented

2023-09-13 14:24:09 +02:00

I think Roughness and IOR should be in the Specular panel, with that panel first and expanded by default.

Metallic and Transmission probably as well. When there are more transmission settings Transmission can move to its own panel.

I think Roughness and IOR should be in the Specular panel, with that panel first and expanded by default. Metallic and Transmission probably as well. When there are more transmission settings Transmission can move to its own panel.

Lukas Stockner commented

2023-09-13 14:27:52 +02:00

I think Roughness and IOR should be in the Specular panel, with that panel first and expanded by default.

Metallic and Transmission probably as well. When there are more transmission settings Transmission can move to its own panel.

My logic behind the placement was that the default-visible options are the ones that users will typically want to adjust.

Things like Anisotropy etc. are fairly specific, so most users won't need to touch them. If we place e.g. Roughness in the Specular panel, everyone will have to open the it, which then also shows lots of "niche" options like Anisotropy.

Also, keep in mind that Roughness and IOR affect Specular, Transmission and now technically also Subsurface.

> I think Roughness and IOR should be in the Specular panel, with that panel first and expanded by default. > > Metallic and Transmission probably as well. When there are more transmission settings Transmission can move to its own panel. My logic behind the placement was that the default-visible options are the ones that users will typically want to adjust. Things like Anisotropy etc. are fairly specific, so most users won't need to touch them. If we place e.g. Roughness in the Specular panel, everyone will have to open the it, which then also shows lots of "niche" options like Anisotropy. Also, keep in mind that Roughness and IOR affect Specular, Transmission and now technically also Subsurface.

Brecht Van Lommel approved these changes 2023-09-13 14:33:10 +02:00

Brecht Van Lommel left a comment

Ok, fair enough, that logic makes sense.

Alaska commented

2023-09-13 15:01:54 +02:00

Does anyone have any opinions on the order of panels.
At the moment it's Subsurface -> Specular -> Sheen -> Coat -> Emission but the order could be changed based on how people view things.

For example, the order could be based on the order of material layering in the Principled BSDF?
Subsurface -> Specular -> Emission -> Coat -> Sheen

Or the order could be based on the frequency of setting use? For example many people will continue to use the Specular slider to control the reflectivity of an object, even though IOR is now the main way of doing that. So maybe the order should be:
Specular -> Subsurface -> Coat -> Sheen -> Emission or
Specular -> Subsurface -> Sheen -> Coat -> Emission

We can change the order after committing. But I just wanted to quickly bring this up in case anyone had strong opinions about it.

Does anyone have any opinions on the order of panels. At the moment it's `Subsurface -> Specular -> Sheen -> Coat -> Emission` but the order could be changed based on how people view things. For example, the order could be based on the order of material layering in the Principled BSDF? `Subsurface -> Specular -> Emission -> Coat -> Sheen` Or the order could be based on the frequency of setting use? For example many people will continue to use the `Specular` slider to control the reflectivity of an object, even though IOR is now the main way of doing that. So maybe the order should be: `Specular -> Subsurface -> Coat -> Sheen -> Emission` or `Specular -> Subsurface -> Sheen -> Coat -> Emission` We can change the order after committing. But I just wanted to quickly bring this up in case anyone had strong opinions about it.

Lukas Stockner commented

2023-09-13 16:28:32 +02:00

I like the idea of ordering it based on layering, just feels intuitive to me. But I don't really have a strong opinion.

Brecht Van Lommel commented

2023-09-13 17:10:21 +02:00

I agree the order is fine.

Alaska commented

2023-09-14 01:30:26 +02:00

I've shifted Coat before Sheen to match the layering inside the Principled BSDF.
I haven't shifted emission. For me, the panel ordering feels off from a visual stand point when doing that.

I have updated the images in the comment above to reflect the changes.

Also, a fix for the hidden links is being worked on: #112326

I've shifted Coat before Sheen to match the layering inside the Principled BSDF. I haven't shifted emission. For me, the panel ordering feels off from a visual stand point when doing that. I have updated the images in the comment above to reflect the changes. ---- Also, a fix for the hidden links is being worked on: #112326

Alaska added 1 commit 2023-09-14 01:30:30 +02:00

Place coat before sheen to match layering inside the Principled BSDF shader 3985c8a6e7

Alaska changed title from ~~Add panels to the Principled BSDF~~ to Add panels to Principled BSDF node

2023-09-14 09:55:04 +02:00

Weizhen Huang commented

2023-09-14 14:39:19 +02:00

Now that we have panel, I think the sockets should be renamed because otherwise it would be too verbose. For example all the coat-related stuff can have the coat prefix removed, and coat and be rename as Intensity (Scale? Factor? Strength?). Emission should be rename to Color instead.

We can use name/identifier for this purpose, for example coat.add_input<decl::Color>("Tint", "Coat Tint")

Now that we have panel, I think the sockets should be renamed because otherwise it would be too verbose. For example all the coat-related stuff can have the coat prefix removed, and coat and be rename as Intensity (Scale? Factor? Strength?). Emission should be rename to Color instead. ![image](/attachments/69487fcb-9a4d-4403-bc33-be3cdc2a0177) We can use `name`/`identifier` for this purpose, for example ` coat.add_input<decl::Color>("Tint", "Coat Tint")`

image.png

19 KiB

👍 1

Weizhen Huang commented

2023-09-14 14:55:58 +02:00

Instead of renaming Coat to Intensity, I'm thinking maybe we can have a representative socket for the panel so that you see the value even when it's collapsed, no additional Intensity bar needed. For Emission it could show the color instead. It should be technically possible. @LukasTonne @dfelinto

~~Weizhen Huang referenced this pull request 2023-09-14 15:11:26 +02:00~~

Cycles/EEVEE: Change specular tint in Principled BSDF from float to color #112192

Lukas Tönne commented

2023-09-14 15:25:34 +02:00

maybe we can have a representative socket for the panel so that you see the value even when it's collapsed,

The easiest way to support this would be a flag in PanelDeclaration to show the first contained socket as the header instead of just the name and toggle button. The drawing code would take care of drawing the socket in the panel header location, without changes to the underlying data. It would make some data redundant like the panel name and description, but that seems acceptable.

> maybe we can have a representative socket for the panel so that you see the value even when it's collapsed, The easiest way to support this would be a flag in `PanelDeclaration` to show the first contained socket as the header instead of just the name and toggle button. The drawing code would take care of drawing the socket in the panel header location, without changes to the underlying data. It would make some data redundant like the panel name and description, but that seems acceptable.

👍 1

Brecht Van Lommel commented

2023-09-14 17:27:41 +02:00

Now that we have panel, I think the sockets should be renamed because otherwise it would be too verbose. For example all the coat-related stuff can have the coat prefix removed, and coat and be rename as Intensity (Scale? Factor? Strength?). Emission should be rename to Color instead.

Agreed. Also moving enums into panels would be good.

We can use name/identifier for this purpose, for example coat.add_input<decl::Color>("Tint", "Coat Tint")

Unfortunately this would not work for the Python API, as for some legacy reason it's actually the name that is used in node.inputs["Coat Tint"]. That's something we need to resolve at some point.

But even if we do, perhaps it would still be better to keep the full name, and have a mechanism to shorten it just for display inside a panel. In general for properties we tend to give them the full name without assumptions about how they might be shortened in the UI. That way the names still make sense when you look at them in isolation for keyframing, API docs, exposing the individual socket as a node group input, etc.

The simplest trick would just be to strip the panel name from the socket names automatically. It seems the most convenient, also for user defined node groups to do this automatically.

The easiest way to support this would be a flag in PanelDeclaration to show the first contained socket as the header instead of just the name and toggle button. The drawing code would take care of drawing the socket in the panel header location, without changes to the underlying data. It would make some data redundant like the panel name and description, but that seems acceptable.

Even for this case it could be automatic and just check if name of the panel and first socket are equal.

But if it turns out control over this is needed, there could be flags for stripping prefixes and putting the first socket in the header.

> Now that we have panel, I think the sockets should be renamed because otherwise it would be too verbose. For example all the coat-related stuff can have the coat prefix removed, and coat and be rename as Intensity (Scale? Factor? Strength?). Emission should be rename to Color instead. Agreed. Also moving enums into panels would be good. > We can use `name`/`identifier` for this purpose, for example ` coat.add_input<decl::Color>("Tint", "Coat Tint")` Unfortunately this would not work for the Python API, as for some legacy reason it's actually the name that is used in `node.inputs["Coat Tint"]`. That's something we need to resolve at some point. But even if we do, perhaps it would still be better to keep the full name, and have a mechanism to shorten it just for display inside a panel. In general for properties we tend to give them the full name without assumptions about how they might be shortened in the UI. That way the names still make sense when you look at them in isolation for keyframing, API docs, exposing the individual socket as a node group input, etc. The simplest trick would just be to strip the panel name from the socket names automatically. It seems the most convenient, also for user defined node groups to do this automatically. > The easiest way to support this would be a flag in `PanelDeclaration` to show the first contained socket as the header instead of just the name and toggle button. The drawing code would take care of drawing the socket in the panel header location, without changes to the underlying data. It would make some data redundant like the panel name and description, but that seems acceptable. Even for this case it could be automatic and just check if name of the panel and first socket are equal. But if it turns out control over this is needed, there could be flags for stripping prefixes and putting the first socket in the header.

👍 2

Hans Goudey commented

2023-09-14 17:35:04 +02:00

I'm thinking maybe we can have a representative socket for the panel so that you see the value even when it's collapsed

I think it would be better to stick with existing UI conventions and only do this for boolean sockets.
Showing arbitrary buttons in the header raises a lot of questions and adds noise.

>I'm thinking maybe we can have a representative socket for the panel so that you see the value even when it's collapsed I think it would be better to stick with existing UI conventions and only do this for boolean sockets. Showing arbitrary buttons in the header raises a lot of questions and adds noise. ![image](/attachments/919f7db1-2e21-4345-ba0c-479499a311fc)

image.png

5.2 KiB

Brecht Van Lommel commented

2023-09-14 17:47:15 +02:00

I think it would be better to stick with existing UI conventions and only do this for boolean sockets.
Showing arbitrary buttons in the header raises a lot of questions, and adds noise.

Hm yes, I was only thinking about how this would work in the node editor. But in panels in the properties editor it would look rather out of place, and we better keep them consistent.

> I think it would be better to stick with existing UI conventions and only do this for boolean sockets. > Showing arbitrary buttons in the header raises a lot of questions, and adds noise. Hm yes, I was only thinking about how this would work in the node editor. But in panels in the properties editor it would look rather out of place, and we better keep them consistent.

Alaska commented

2023-09-14 23:20:43 +02:00

Just double checking, should I shorten the names of parameters (E.G. Rename Coat IOR to IOR), or is this still a design decision being discussed?

Just double checking, should I shorten the names of parameters (E.G. Rename `Coat IOR` to `IOR`), or is this still a design decision being discussed?

Alaska commented

2023-09-15 03:11:47 +02:00

Now that we have panel, I think the sockets should be renamed because otherwise it would be too verbose.

We can use name/identifier for this purpose, for example coat.add_input<decl::Color>("Tint", "Coat Tint")

There is also a issue with this. The properties editor does not display panels (@LukasTonne not sure if this was an oversight, purposeful design, or a known limitation). So if we simplify all the names, we'll end up with a confusing Principled BSDF in the properties editor like the one shown below (E.G. There's multiple Intensity, Roughness, and IOR sliders with no indication of what they control).

Just as a side note. The Principled BSDF in the properties editor kind of looks messy with the pull request in it's current state. Mainly due to the Normal, Tangent, and Coat Normal parameters being mixed in with all the other parameters.

Main	Current Pull Request	Short names

> Now that we have panel, I think the sockets should be renamed because otherwise it would be too verbose. > ![image](/attachments/69487fcb-9a4d-4403-bc33-be3cdc2a0177) > We can use `name`/`identifier` for this purpose, for example ` coat.add_input<decl::Color>("Tint", "Coat Tint")` There is also a issue with this. The properties editor does not display panels (@LukasTonne not sure if this was an oversight, purposeful design, or a known limitation). So if we simplify all the names, we'll end up with a confusing Principled BSDF in the properties editor like the one shown below (E.G. There's multiple `Intensity`, `Roughness`, and `IOR` sliders with no indication of what they control). Just as a side note. The Principled BSDF in the properties editor kind of looks messy with the pull request in it's current state. Mainly due to the `Normal`, `Tangent`, and `Coat Normal` parameters being mixed in with all the other parameters. | Main | Current Pull Request | Short names | | - | - | - | | ![Main](/attachments/11c4664a-085a-4318-8b9a-385386ebf506) | ![Full names](/attachments/6f3a3121-b752-4074-b281-ce9de3294883) | ![Short names](/attachments/1b1932cb-2294-4e06-93f2-78e7cdfdac81) |

Full Name Principled BSDF.png

199 KiB

Main Principled BSDF.png

522 KiB

Short Name Principled BSDF.png

180 KiB

Brecht Van Lommel commented

2023-09-15 17:56:52 +02:00

Just double checking, should I shorten the names of parameters (E.G. Rename Coat IOR to IOR), or is this still a design decision being discussed?

We do want to shorten them, but for reasons I mentioned earlier and other reasons you found, we can't really do it by just shortening the name.

I'll look into a better shortening mechanism and improvements to the material properties editor next week.

> Just double checking, should I shorten the names of parameters (E.G. Rename `Coat IOR` to `IOR`), or is this still a design decision being discussed? We do want to shorten them, but for reasons I mentioned earlier and other reasons you found, we can't really do it by just shortening the name. I'll look into a better shortening mechanism and improvements to the material properties editor next week.

👍 1

Lukas Stockner commented

2023-09-16 22:02:38 +02:00

The fix for links not showing up is merged now - I think we can just merge this as-is and handle the short-name topic (and panels in the property editor) as a follow-up?

Alaska added 1 commit 2023-09-17 02:46:09 +02:00

Merge branch 'main' into principled-panels 47604557af

Brecht Van Lommel commented

2023-09-17 02:58:21 +02:00

Yes, we could merge this and refine the UI in main.

Lukas Stockner referenced this issue from a commit

2023-09-17 15:42:22 +02:00

Add panels to Principled BSDF node

Lukas Stockner merged commit 88ad79c2d1 into main

2023-09-17 15:42:24 +02:00

msalv commented

2023-09-18 18:27:47 +02:00

First-time contributor

Should the SSS type menu (Random Walk, etc) be left outside the subsurface panel? does it affect anything except subsurface?

Weizhen Huang commented

2023-09-18 18:36:41 +02:00

Should the SSS type menu (Random Walk, etc) be left outside the subsurface panel? does it affect anything except subsurface?

No, it would be good to put those options (or enums as Brecht mentioned) in the panel too. But currently they are managed separately from the sockets and always appear at the top of the node.

> Should the SSS type menu (Random Walk, etc) be left outside the subsurface panel? does it affect anything except subsurface? No, it would be good to put those options (or enums as Brecht mentioned) in the panel too. But currently they are managed separately from the sockets and always appear at the top of the node.

Brecht Van Lommel referenced this pull request

2023-09-19 19:27:44 +02:00

Principled v2 BSDF #99447

Brecht Van Lommel commented

2023-09-20 18:57:17 +02:00

See #112591 for various panel UI improvements discussed here.

Sign in to join this conversation.

No reviewers