twostraws / Inferno

Metal shaders for SwiftUI.
Other
2.46k stars 119 forks source link

Add SPM Support #19

Closed john-flanagan closed 12 months ago

john-flanagan commented 1 year ago

This is an initial pass at adding SPM support to address issue #17

Open questions

Is the split of whats in the package vs. whats in the sandbox app correct?

What's the best way to handle accessing the correct ShaderLibrary from consumer code?

Should the different types of shaders be different package?

i.e. InfernoBlurShaders, InfernoGenerationShaders, ... There could still be one higher level Inferno package that re-exports all of the other modules

daprice commented 1 year ago

What's the best way to handle accessing the correct ShaderLibrary from consumer code?

  • Stick with InfernoShaderLibrary implementation as implemented in the PR
  • Hide away all access to ShaderLibrary behind an abstraction layer for consumers.

I think the current way is probably the way to go. Adding the extra layer like your example does provide the benefit of having named parameters, but it hides the important detail of whether each shader is suitable for colorEffect, distortionEffect, layerEffect, ShapeStyle etc which require different Metal function signatures. That could be solved by defining protocols like colorEffectCompatible, ShapeStyleCompatible and so on, but that seems like overkill when SwiftUI itself doesn’t provide that level of abstraction.

Adding View or VisualEffect extensions as suggested in #15 seems like the better place to put that type of abstraction IMO.


Should the different types of shaders be different package?

i.e. InfernoBlurShaders, InfernoGenerationShaders, ... There could still be one higher level Inferno package that re-exports all of the other modules

@twostraws thoughts on this one? Having one package and one .metallib does mean that consumers end up with all of the shaders in their final build, not just the ones they use. I don’t see an obvious performance/memory impact – I’m guessing it only does its runtime processing on the functions you actually look up from the library? For now (this will grow if Inferno gets more shaders) including the entire metallib adds a few hundred kb to the final build, which doesn’t seem like much, but it might matter to someone building an App Clip with a 15mb limit.

john-flanagan commented 1 year ago

@daprice Thanks for the review and catching the issue with the absolute vs. relative path.

For the InfernoShaderLibrary, assuming we stick with that approach, do you like the dynamicMemberLookup approach to mirror the behavior of SwiftUI.ShaderLibrary to be consistent, or would it be better to define static properties like this to enable auto-complete:

public enum InfernoShaderLibrary {
    public static var circleWaveTransition: ShaderFunction {
        ShaderLibrary.bundle(.inferno).circleWaveTransition
    }

    /// A bunch more...
}
twostraws commented 12 months ago

Dynamic member lookup is great for the way SwiftUI finds Metal shaders, but these are examples we've built ahead of time so I think the best approach is to expose them as fixed values, properties or methods depending on whether they are customization points. As @daprice says, #15 outlines what I consider the ideal approach here.

As for putting all the shaders in one file, that's fine by me – I would imagine(!) that including one shader already requires the metallib, and adding more has a limited effect.

It would be great to reduce the friction to be as low as possible.

twostraws commented 12 months ago

I'm going to merge this in now, but there's still lots of time to adjust until we make a tag in the future.