Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@bdero
Copy link
Member

@bdero bdero commented Oct 28, 2022

In support of flutter/flutter#114214.

@bdero bdero self-assigned this Oct 28, 2022
@bdero bdero changed the title [Impeller] Add FlatterFragCoord to impellerc shader lib [Impeller] Add FlutterFragCoord to impellerc shader lib Oct 28, 2022
@chinmaygarde
Copy link
Member

Can we perhaps rewrite this during SKSL generation automatically?

@zanderso
Copy link
Member

Can we perhaps rewrite this during SKSL generation automatically?

Do you mean rewrite FlutterFragCoord() in the SkSL backend, but leave it alone and use the definition in this header otherwise? That would avoid the ifdefs for sure, or were you thinking of a change that would avoid the header entirely?

@bdero
Copy link
Member Author

bdero commented Oct 28, 2022

A couple of constraints to consider here:

  • The sksl generator is actually hijacking gl_FragCoord and supplying a varying input to it. Not doing this will break all existing shaders. I think the consensus is that backwards compatibility isn't a huge deal right now, but is a nice-to-have.
  • The source input shader must be valid GLSL for the compiler to accept it and translate to spirv. I don't think there's a way for us to define our own custom builtins and then rewrite them in spirvcross.

One thing I considered was just removing this gl_FragCoord hijacking in the sksl generator and just having users always specify the varying input -- this would break all existing shaders, of course. One of the reasons I ended up going for this opt-in backwards compatible header shim was that the runtime effect use cases (color source and filters) will only ever support a strictly managed attribute layout anyways, and so it feels like this solution ends up being simpler for users. They just get a function to call and don't have to worry about keeping the varying layout correct. A slight side benefit is that this also allows us to change the vertex layout without breaking users (although I can't imagine we'll need to).

This solution seemed to hit the sweet spot for me, but I'm totally open to other ideas even if it means introducing breakages, etc.

@bdero bdero force-pushed the bdero/shaderlib-runtime-effect branch from 597c022 to dc14a21 Compare October 28, 2022 23:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants