Switch lockgeom to interpolated and interactive #1662

lgritz · 2023-03-29T06:14:55Z

Establish two new metadata hints about shader parameters:

"interpolated" means that the parameter comes from the geometry somehow and may be bound with a different value at different points on the surfaces, or different values for different primitives that share the material. This corresponds to concepts known variously as "geometric primitive data/variables", "user data", or "vertex variables", depending on the renderer.
"interactive" means that the user desires the parameter to be allowed to change when the scene is interactively rerendered, without incurring a full recompile of the shader. (Variables not so marked might still be capable of being changed, but not necessarily without a re-JIT of the shaders.)

Both of these require that the parameter not be fully constant-folded or optimized away during runtime optimizeation or JIT, and thus supplant the old "lockgeom=0" hint. But their semantics may differ in other ways, so we thought it was worth introducing a separation, and at the same time, finally fixing the mis-design of "lockgeom" (which in retrospect is both a confusing name and also has the 0|1 sense backwards, thwarting even my own intuition every time I use it).

We introduce new variants of ShadingSystem::Parameter() that changes the optional bool lockgeom parameter for a new enum ParamHints that allows for future expansion with more hinted properties. For now, the old calls still work (lockgeom=false is the same as passing ParamHints::interpolated) and this does not introduce any break to backwards ABI compatibility at this time.

In this PR, "interpolated" is basically the replacement for the old lockgeom (though the sense is reversed). The "interactive" hint is put in place but doesn't do anything especially useful at the moment that is different from interpolated. A few more things are coming down the pike in subsequent PRs: (1) Make interactive work and fully hook it up to ReParameter(), including on GPU. (2) Make a new global SS option that says if you're building shaders for an interactive render or an offline render. (If offline, the even "interactive" hinted params should be constant-folded.)

aconty · 2023-03-30T12:05:42Z

src/include/osl_pvt.h

+    unsigned m_initialized : 1;     ///< If a param, has it been initialized?
+    unsigned m_interpolated : 1;    ///< Is the param overridden by geom?
+    unsigned m_interactive : 1;     ///< May the param change interactively?
+    unsigned m_noninteractive : 1;  ///< The param def won't modify interactively


Not sure I understand why we need interactive and noninteractive flags.

In discussion with Lee, we wanted leave room for the source code hints to be able to explicitly say [[ int interactive = 0 ]] to indicting being quite sure that this parameter is not one that lookdev artists will need to adjust interactively, whereas lacking any hint one way or the other has the possibility for a runtime control to allow everything not marked to be flipped to interactive.

It's still a half-baked idea that won't be fleshed out until we have a UI and understand more how it's used in practice, but I'm leaving as a placeholder so that the hint is really 3-state on, off, or not specified.

lgritz · 2023-03-30T17:24:39Z

By the way, if people feel like "interpolated" is the wrong name for this concept, it can be debated. We can discuss at today's TSC meeeting.

ee33 · 2023-03-30T19:57:00Z

Also curious, is there a case where you’d want both “interpolated” and “interactive” on the same attr? From: Larry Gritz ***@***.***> Date: Thursday, March 30, 2023 at 10:25 AM To: AcademySoftwareFoundation/OpenShadingLanguage ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [AcademySoftwareFoundation/OpenShadingLanguage] Switch lockgeom to interpolated and interactive (PR #1662) By the way, if people feel like "interpolated" is the wrong name for this concept, it can be debated. We can discuss at today's TSC meeeting. — Reply to this email directly, view it on GitHub<#1662 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AERHLSUFKEDTY735JESB3ULW6W6WVANCNFSM6AAAAAAWLPXG34>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

ee33 · 2023-03-30T19:58:20Z

Never mind, easier to leave them separate bits as you have them. From: Eric Enderton ***@***.***> Date: Thursday, March 30, 2023 at 12:56 PM To: AcademySoftwareFoundation/OpenShadingLanguage ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [AcademySoftwareFoundation/OpenShadingLanguage] Switch lockgeom to interpolated and interactive (PR #1662) Also curious, is there a case where you’d want both “interpolated” and “interactive” on the same attr? From: Larry Gritz ***@***.***> Date: Thursday, March 30, 2023 at 10:25 AM To: AcademySoftwareFoundation/OpenShadingLanguage ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [AcademySoftwareFoundation/OpenShadingLanguage] Switch lockgeom to interpolated and interactive (PR #1662) By the way, if people feel like "interpolated" is the wrong name for this concept, it can be debated. We can discuss at today's TSC meeeting. — Reply to this email directly, view it on GitHub<#1662 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AERHLSUFKEDTY735JESB3ULW6W6WVANCNFSM6AAAAAAWLPXG34>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

AlexMWells · 2023-04-03T16:07:02Z

src/include/OSL/oslexec.h

+    /// `interactive`: Parameter may have its value interactively modified by
+    /// subsequent ReParameter calls. Any optimization of the shader should
+    /// take this into account.
+    interactive = 2


Should these enum values be specified in hex so its more obvious they are bitflags vs. a vanilla enum?

Considering that the only values now are 1 and 2, I'm not sure it's any clearer in hex. :-)

But I take your point, and I will clarify in the comments about the class itself.

C++ has binary literals since C++14 which I think is our floor, right?

Yes, 14 is the floor. I've never used binary literals, but they make sense here. Another idiom I've used is

pepperoni = (1 << 13), mushroom = (1 << 14), pineapple = (1 << 15), ...

But when the only choices are 1 and 2, I'm not sure any of these language flourishes really improves the clarity. If and when there are enough bits to be confusing, we can choose a different notation.

AlexMWells · 2023-04-03T16:07:31Z

src/include/OSL/oslexec.h

@@ -111,6 +111,66 @@ struct SymLocationDesc {



+/// Parameter property hint bitfield values


Should bitfield read "bitflag"?

yes, good point

lgritz · 2023-04-07T00:08:17Z

Sorry for the delay in following up with this. At last week's TSC meeting, we discussed this and there was a lot of back-and-forth about the naming. To summarize from my notes: Some people liked "live" instead of "interactive." And also some people didn't like "interpolated", resulting in a variety of not-quite-better suggestions like generated, provided, queried, bound, overrideable, inconstant (ugh).

After agonizing over this for a while, none of these alternatives have really won me over as being any more clear than my original choices, so I'm inclined to claim "implementor's prerogative" and just merge this PR as-is and move on to get the next set of changes ready to show. My guess is that once it's in, it will seem fine ("what color should the bike shed be?" is a different dynamic from "I painted it green, let me know you think that's a problem after you've used it for a week"). There's nothing stopping us from changing it, up until we branch for a 1.13 release. Everything in main is understood to be subject to revision, and this will be a lot less intrusive than other changes we know are coming down the road.

fpsunflower

LGTM

Establish two new metadata hints about shader parameters: - "interpolated" means that the parameter comes from the geometry somehow and may be bound with a different value at different points on the surfaces, or different values for different primitives that share the material. This corresponds to concepts known variously as "geometric primitive data/variables", "user data", or "vertex variables", depending on the renderer. - "interactive" means that the user desires the parameter to be allowed to change when the scene is interactively rerendered, without incurring a full recompile of the shader. (Variables not so marked might still be capable of being changed, but not necessarily without a re-JIT of the shaders.) Both of these require that the parameter not be fully constant-folded or optimized away during runtime optimizeation or JIT, and thus supplant the old "lockgeom=0" hint. But their semantics may differ in other ways, so we thought it was worth introducing a separation, and at the same time, finally fixing the mis-design of "lockgeom" (which in retrospect is both a confusing name and also has the 0|1 sense backwards, thwarting even my own intuition every time I use it). We introduce new variants of `ShadingSystem::Parameter()` that changes the optional `bool lockgeom` parameter for a new enum `ParamHints` that allows for future expansion with more hinted properties. For now, the old calls still work (lockgeom=false is the same as passing ParamHints::interpolated) and this does not introduce any break to backwards ABI compatibility at this time. In this PR, "interpolated" is basically the replacement for the old lockgeom (though the sense is reversed). The "interactive" hint is put in place but doesn't do anything especially useful at the moment that is different from interpolated. A few more things are coming down the pike in subsequent PRs: (1) Make interactive work and fully hook it up to ReParameter(), including on GPU. (2) Make a new global SS option that says if you're building shaders for an interactive render or an offline render. (If offline, the even "interactive" hinted params should be constant-folded.) Signed-off-by: Larry Gritz <lg@larrygritz.com>

Signed-off-by: Larry Gritz <lg@larrygritz.com>

aconty reviewed Mar 30, 2023

View reviewed changes

aconty approved these changes Mar 30, 2023

View reviewed changes

AlexMWells reviewed Apr 3, 2023

View reviewed changes

lgritz force-pushed the lg-lockgeom branch from b265c63 to 95ae959 Compare April 6, 2023 23:56

fpsunflower approved these changes Apr 7, 2023

View reviewed changes

lgritz added 2 commits April 8, 2023 11:55

revision

7420e53

Signed-off-by: Larry Gritz <lg@larrygritz.com>

lgritz force-pushed the lg-lockgeom branch from 3ed1758 to 7420e53 Compare April 8, 2023 18:55

lgritz merged commit 0bd014c into AcademySoftwareFoundation:main Apr 8, 2023
20 checks passed

lgritz deleted the lg-lockgeom branch April 11, 2023 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch lockgeom to interpolated and interactive #1662

Switch lockgeom to interpolated and interactive #1662

lgritz commented Mar 29, 2023

aconty Mar 30, 2023

lgritz Mar 30, 2023

lgritz commented Mar 30, 2023

ee33 commented Mar 30, 2023 via email

ee33 commented Mar 30, 2023 via email

AlexMWells Apr 3, 2023

lgritz Apr 7, 2023

fpsunflower Apr 7, 2023

lgritz Apr 7, 2023

AlexMWells Apr 3, 2023

lgritz Apr 7, 2023

lgritz commented Apr 7, 2023

fpsunflower left a comment

		@@ -111,6 +111,66 @@ struct SymLocationDesc {



		/// Parameter property hint bitfield values

Switch lockgeom to interpolated and interactive #1662

Switch lockgeom to interpolated and interactive #1662

Conversation

lgritz commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgritz commented Mar 30, 2023

ee33 commented Mar 30, 2023 via email

ee33 commented Mar 30, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgritz commented Apr 7, 2023

fpsunflower left a comment

Choose a reason for hiding this comment