Re: [filter-effects] New syntax proposal for 'custom' filter function

Hi Tab,

Thank you very much for your feedback. I really appreciate your input!

On Nov 15, 2012, at 2:43 PM, Tab Atkins Jr. <jackalmage@gmail.com> wrote:

> Overall, looks good!  I really like how it lets you specify default
> values for arguments, and otherwise just stash information into a
> single location rather than spamming it across your stylesheet.
> 
> GLSL is still a required part of the Filters spec, though, right?  I
> strongly don't support making GLSL optional, even if this proposal
> does make it marginally easier to use two versions.

GLSL stays the recommended shading language as resolved by both working groups. The working draft was published three weeks ago and I am not aware of any exclusion requests.

> 
> Some comments inline:
> 
> On Thu, Nov 15, 2012 at 2:04 PM, Dirk Schulze <dschulze@adobe.com> wrote:
>> The formal syntax for the 'custom' function would be:
>> 
>>        custom([ none | IDENT ]+ [, <params>]*)
>> 
>> * IDENT is the name of a custom-filter at-rule.
>> * <params> is a definition of a shader parameter as defined in the current working draft[3].
> 
> Throughout your examples you've used strings, not idents.  I agree
> with IDENT, but I want to make sure that you actually meant that.

I want to follow CSS Animations and Font-face here, and looked at CSS Animations WD. But sadly the WD used IDENT, which was corrected in the ED. I will correct it as well.

> 
>> custom-filter at-rule
>> ===
>> 
>> To address the needs on the defined goals, we came up with a new custom-filter at-rule. The custom-filter at-rule defines a shader program with the following required properties:
>> 
>>        format: IDENT
> 
> As long as we have control over the format's name, I agree with IDENT
> here as well.  If the name comes from somewhere external, we should
> use <string> instead.
> 
> Once again, all of your examples are using strings, not idents.
> 
> This shouldn't be required.  It should default to "glsl" if not
> specified, since that's the required shading language.

I fear that I made the same mistake here and on your other parts you commented below.

> 
>>        mesh: <integer>{0,2} <box>? [ detached | attached ]?
> 
> I don't see any reason to be strict about ordering here.  The grammar should be:
> 
> mesh: <integer>{1,2} || <box> || [ detached | attached ]
> 
> (Plus, the grammar as written was incorrect - it technically allows
> "mesh:;" as valid.)
> 
> Also, this shouldn't be required.  Apparently they all have defaults,
> so allow it to be omitted and just set those defaults.

That is totally fine for me. This particular declaration definition is still in flux.

> 
>>        mix: <compositing> && <blending>
> 
> Again, no need to make these required.  The obvious defaults are
> "src-over" and "normal".  Given that, the grammar should also be
> changed, to:
> 
> mix: <compositing> || <blending>
> 
> What's the origin of the name "mix"?  Do we think a similar property
> will show up in the Compositing & Blending spec (as a shorthand, I
> guess)?  If not, we shouldn't innovate here - just use separate
> 'alpha-compositing' and 'blend-mode' descriptors.

Have to think about that. This is based on the function name we had in the specification before. I actually don't have anything in mind renaming it if there is a proper name.

> 
>> The UA should take the value of 'format' into account to identify if the shader language is supported. If the author does not specify this property, the UA is recommended to interpret the shader as GLSL shader.
> 
> Okay, so you are requiring the format to be assumed to be GLSL if it's
> not specified.  This means that 'format' is not a required descriptor.
> Also, "recommended" isn't an RFC 2119 keyword.  Do you mean MUST or
> SHOULD?  (The former, please.)

It is, see original mailing list thread linked below, or the thread about "[filter-effects] FPWD publication request" (Don't have the link handy yet). Nothing changed here.

> 
>> Other shading languages may require additional properties like 'geometry' for a geometry shader. This specification explicitly allows the extension of the custom-filter function by new properties, as the shading language requires. The property set must not be extended, if the format is specified as "glsl". A future version of this specification may define further identifiers and properties.
> 
> This can be rephrased - simply state that unknown descriptors do *not*
> invalidate the rule, but that implementations MUST NOT extend the set
> of descriptors accepted by the "glsl" format except as specified by
> this specification or future versions.

Makes sense. Yes.

> 
>> UAs must not accept 'format' identifier if the associated shading language is not supported.
> 
> Rephrase to say that unknown shading language idents make the entire
> rule invalid.

Ok.

> 
>> If the parsing of a custom-filter at-rule fails, the parsing of the referenced shader files fail, or the format identifier is not accepted by the UA, this rule is treated as invalid. The custom function must proceed with the next custom-filter at-rule in the custom-filter at-rule list. Shader files of invalid custom-filter at-rule are not downloaded.
> 
> Be *very* careful how you throw around the term "invalid".  Parsing
> the referenced shader *can not* make the rule invalid, in the sense
> that it's thrown away in the stylesheet.  Define a separate "invalid
> shader" concept, have a failed parse flag the shader as an invalid
> shader, and then have the fallback algorithm pay attention to whether
> it's an "invalid shader" as well as whether the shader itself exists.

Ok.

> 
>> Properties in the custom-filter at-rule are not animatable.
> 
> Things in at-rules are descriptors, not properties, and are never
> animatable in the first place.  No need for this as a special case.

Thanks, I didn't know the proper term for it.

> 
> 
>> Animation of custom function
>> ====
>> 
>> To avoid repetition of values, parameters can be set in the custom-filter at-rule. If a parameter does not need to be animated, authors should define the parameter in the custom-filter at-rule. Otherwise the parameter should be specified in the custom function.
> 
> Just define that the computed value of a custom filter contains all
> the params, with params missing in the specified value filled in from
> the defaults.  You already have a section on how to animate custom()
> and its special new value types, so you're fine.

Awesome. Thanks for the input again.

Greetings,
Dirk

> 
> ~TJ

Received on Thursday, 15 November 2012 23:34:00 UTC