AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers
BSD 3-Clause "New" or "Revised" License
2.07k stars 350 forks source link

Switch lockgeom to interpolated and interactive #1662

Closed lgritz closed 1 year ago

lgritz commented 1 year ago

Establish two new metadata hints about shader parameters:

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.)

lgritz commented 1 year ago

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 commented 1 year ago

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 GitHubhttps://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1662#issuecomment-1490665217, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AERHLSUFKEDTY735JESB3ULW6W6WVANCNFSM6AAAAAAWLPXG34. You are receiving this because you are subscribed to this thread.Message ID: @.***>

ee33 commented 1 year ago

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 GitHubhttps://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1662#issuecomment-1490665217, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AERHLSUFKEDTY735JESB3ULW6W6WVANCNFSM6AAAAAAWLPXG34. You are receiving this because you are subscribed to this thread.Message ID: @.***>

lgritz commented 1 year ago

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.