Introducing VortexSystem.Settings

[disc0infern0]

This commit looks large, but is essentially just an abstraction of the configuration settings from the VortexSystem into it's own Settings class, with those properties replaced in VortexSystem with a single property; vortexSettings.

The changes are documented in a the updated README. Settings enables an intuitive initialisation based on preset settings, enables autocomplete for each setting (rather than relying on specifying the required parameters of the init, and getting them all in the right order), and removes the need to ever call .makeUniqueCopy.

The change necessitates changes to VortexSystem of course, but also the VortexSystem-Behaviour to ensure that the configuration settings are accessed via vortexSettings. (dynamicLookup is also added to VortexSystem for convenience).

Included here are also changes to four presets, Confetti, Fire, Fireflies and Fireworks, showing how the settings would be used for each. (A new full width Fire preview is also included showing how preset settings can be tweaked). The others remain (at least for now) with the previous VortexSystem method of initialisation.
Changing the remainder of the presets would take a matter of moments, however I did not want to update too many files at once! (In the interim, some deprecation warnings will be generated by the compiler - see below)

Because of the changes, and in particular because secondary systems are now secondary settings, this could have been a breaking change. However in the interests of backward compatibility, the old inits have been preserved, although they are now marked as deprecated.
This will show warnings in the code, but everything will continue to work.

It is evisaged that after a suitable time, agreed by Paul, the deprecated inits could be removed, and some further code cleanup completed. One such change would be modifying the init of the VortexView to accept a Settings struct as an anonymous parameter, so that VortexView(.confetti) would call the VortexSystem.Settings.confetti preset, rather than the current VortexSystem.confetti preset, which would be removed.

[disc0infern0]

Whether position is a configuration setting or not is a debatable point, because it is a property that gets modified by the 'moveTo' method in the proxy. Configuration parameters should arguably be unmodifiable. One could certainly argue that initialPosition could be a valid configuration setting, and it would then require a separate position property to be held and initialised within VortexSystem. No changes have been made in this regard.

[twostraws]

This is an odd shape for an initializer. What does it offer over code like this:

var settings = VortexSystem.Settings(from: .fire)
settings.idleDuration = 3
settings.angle = .whatever

[twostraws]

I think this should just be settings.

[disc0infern0]

agreed

[twostraws]

It's generally not a good idea to mix up logic and layout in the body property, so I think for the README it might be a better idea to pull this settings object out into its own property.

[twostraws]

I think we should probably use _ settings so that we get VortexView(.confetti). I'd be happy to rename the older one and mark it deprecated if it's causing confusion, but I want the newer, correct option to be the simplest.

[disc0infern0]

agreed

[twostraws]

I appreciate the problem this is trying to solve, but I'm not sure it's really that practical – using images rather than shapes has a significant performance impact, and each system uses different variations. For example, .confetti adds a square shape and doesn't use plus lighter, .rain uses 32-point circles without plus lighter, fire adds a blur over 32-point circle, etc.

[disc0infern0]

I will remove prebuilt symbols from the view init, and from any previews.

[twostraws]

I'd be happy to move this whole struct outside VortexSystem and just call it VortexSettings; it's a bit wordy as-is.

[disc0infern0]

agreed.

[twostraws]

Thank you for this! Broadly it looks fine, but I've added various notes and questions around the code.

Changing the remainder of the presets would take a matter of moments, however I did not want to update too many files at once! (In the interim, some deprecation warnings will be generated by the compiler - see below)

There are two independent things in this pull request, which is complicatings things slightly – the move to separate settings, and trying to share tagged particle views. The former seems very good; the latter I'm really not sure about.

Because of the changes, and in particular because secondary systems are now secondary settings, this could have been a breaking change. However in the interests of backward compatibility, the old inits have been preserved, although they are now marked as deprecated. This will show warnings in the code, but everything will continue to work.

Vortex already has a v1 tagged, so we could release a v2 with breaking changes just fine. I'd prefer to remove old code entirely rather than issuing deprecation warnings, or at least make the new approach the easier approach.

Whether position is a configuration setting or not is a debatable point, because it is a property that gets modified by the 'moveTo' method in the proxy. Configuration parameters should arguably be unmodifiable. One could certainly argue that initialPosition could be a valid configuration setting, and it would then require a separate position property to be held and initialised within VortexSystem. No changes have been made in this regard.

Why should configuration parameters be unmodifiable? If someone wants confetti rate to start fast then slow down, wants to make particle bursts grow in size each time a burst is created, or wants to make the angle move constantly like a sweeping motion, all those seem reasonable.

[disc0infern0]

Thankyou for taking the time for such a comprehensive review! :D . I’ll reply to the other comments separately, but will follow your guidance here. The new approach to creating a particle system is very easy; just call the VortexView with a settings struct you previously created, or use a preset. The VortexView then creates the particle systems internally, and only creates secondary systems when the spawn event creates them. With your blessing, I'll remove the old vortex system presets. For clarity, there is no intention to share tagged particle views; only to share presets of type VortexSystem. I did intend to share Previews of each preset for demonstration purposes within Xcode, so the anyone using those presets could see how they would look in their own applications. Would you prefer the Previews to be removed? Should I tag the next PR as a v2, or will you add the tag to it after(if) you merge in the changes? Regarding the comment about position, size and speed being configuration settings that can change over the course of a systems life, I agree with you that it’s ok for these to change, - I just wanted to raise this in case it was a concern. I’m excited about these changes :)

…

-Andrew

On 7 Dec 2024, at 12:11, Paul Hudson ***@***.***> wrote: Thank you for this! Broadly it looks fine, but I've added various notes and questions around the code. Changing the remainder of the presets would take a matter of moments, however I did not want to update too many files at once! (In the interim, some deprecation warnings will be generated by the compiler - see below) There are two independent things in this pull request, which is complicatings things slightly – the move to separate settings, and trying to share tagged particle views. The former seems very good; the latter I'm really not sure about. Because of the changes, and in particular because secondary systems are now secondary settings, this could have been a breaking change. However in the interests of backward compatibility, the old inits have been preserved, although they are now marked as deprecated. This will show warnings in the code, but everything will continue to work. Vortex already has a v1 tagged, so we could release a v2 with breaking changes just fine. I'd prefer to remove old code entirely rather than issuing deprecation warnings, or at least make the new approach the easier approach. Whether position is a configuration setting or not is a debatable point, because it is a property that gets modified by the 'moveTo' method in the proxy. Configuration parameters should arguably be unmodifiable. One could certainly argue that initialPosition could be a valid configuration setting, and it would then require a separate position property to be held and initialised within VortexSystem. No changes have been made in this regard. Why should configuration parameters be unmodifiable? If someone wants confetti rate to start fast then slow down, wants to make particle bursts grow in size each time a burst is created, or wants to make the angle move constantly like a sweeping motion, all those seem reasonable. — Reply to this email directly, view it on GitHub <#29 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBIXUFOOWZFZZKWGWN7T2ELQYNAVCNFSM6AAAAABTFO5WPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRVGA4TANJUGI>. You are receiving this because you authored the thread.

[disc0infern0]

Agreed. I’ll do that.

…

On 7 Dec 2024, at 11:48, Paul Hudson ***@***.***> wrote: @twostraws commented on this pull request. In Sources/Vortex/Settings/Settings.swift <#29 (comment)>: > @@ -0,0 +1,314 @@ +// +// Settings.swift +// Vortex +// https://www.github.com/twostraws/Vortex +// See LICENSE for license information. +// + +import SwiftUI + +extension VortexSystem { I'd be happy to move this whole struct outside VortexSystem and just call it VortexSettings; it's a bit wordy as-is. — Reply to this email directly, view it on GitHub <#29 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBIU7VSZYR3IO7GIEFIT2ELOBVAVCNFSM6AAAAABTFO5WPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWGQ4DKOBZGU>. You are receiving this because you authored the thread.

[disc0infern0]

Ahhh. I see now what you meant about sharing prebuilt views - the symbol views. I’ll think about this some more.

…

On 7 Dec 2024, at 11:42, Paul Hudson ***@***.***> wrote: @twostraws commented on this pull request. In Sources/Vortex/Views/VortexView.swift <#29 (comment)>: > @@ -55,6 +56,32 @@ public struct VortexView<Symbols>: View where Symbols: View { self.symbols = symbols() } + /// Creates a new VortexView from a pre-configured particle system, along with any required SwiftUI + /// views needed to render particles. Sensible defaults will be used if no parameters are passed. + /// - Parameters: + /// - settings: A vortexSettings struct that should be used to generate a particle system. + /// Typically this will be set using a preset static struct, e.g. `.fire`. Defaults to a simple system. + /// - targetFrameRate: The ideal frame rate for updating particles. Defaults to 60 if not specified. (use 120 on Pro model iPhones/iPads ) + /// - symbols: A closure that should return a tagged group of SwiftUI views to use as particles. + /// If not specified, a default group of three views tagged with `.circle`,`.confetti` and `.sparkle` will be used. + public init( + settings: VortexSystem.Settings = .init(), + targetFrameRate: Int = 60, + @ViewBuilder symbols: () -> Symbols = { + Group { + Image.circle I appreciate the problem this is trying to solve, but I'm not sure it's really that practical – using images rather than shapes has a significant performance impact, and each system uses different variations. For example, .confetti adds a square shape and doesn't use plus lighter, .rain uses 32-point circles without plus lighter, fire adds a blur over 32-point circle, etc. — Reply to this email directly, view it on GitHub <#29 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBIXV2W42O257O3PCSMD2ELNK3AVCNFSM6AAAAABTFO5WPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWGQ4DKMBYGE>. You are receiving this because you authored the thread.

[disc0infern0]

Agreed. Will change to: _ settings = VortexSettings(), I’ll remove the old VortexSystem init .

…

On 7 Dec 2024, at 11:31, Paul Hudson ***@***.***> wrote: @twostraws commented on this pull request. In Sources/Vortex/Views/VortexView.swift <#29 (comment)>: > @@ -55,6 +56,32 @@ public struct VortexView<Symbols>: View where Symbols: View { self.symbols = symbols() } + /// Creates a new VortexView from a pre-configured particle system, along with any required SwiftUI + /// views needed to render particles. Sensible defaults will be used if no parameters are passed. + /// - Parameters: + /// - settings: A vortexSettings struct that should be used to generate a particle system. + /// Typically this will be set using a preset static struct, e.g. `.fire`. Defaults to a simple system. + /// - targetFrameRate: The ideal frame rate for updating particles. Defaults to 60 if not specified. (use 120 on Pro model iPhones/iPads ) + /// - symbols: A closure that should return a tagged group of SwiftUI views to use as particles. + /// If not specified, a default group of three views tagged with `.circle`,`.confetti` and `.sparkle` will be used. + public init( + settings: VortexSystem.Settings = .init(), I think we should probably use _ settings so that we get VortexView(.confetti). I'd be happy to rename the older one and mark it deprecated if it's causing confusion, but I want the newer, correct option to be the simplest. — Reply to this email directly, view it on GitHub <#29 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBITJODVAMPC5PPLAUPT2ELMBXAVCNFSM6AAAAABTFO5WPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWGQ4DGNJWGY>. You are receiving this because you authored the thread.

[disc0infern0]

Agreed.

…

On 7 Dec 2024, at 11:29, Paul Hudson ***@***.***> wrote: @twostraws commented on this pull request. In README.md <#29 (comment)>: > ```swift -VortexView(.fire) { - Circle() - .fill(.white) - .blendMode(.plusLighter) - .blur(radius: 3) - .frame(width: 32) - .tag("circle") +struct ContentView: View { + var body: some View { + let fireSettings = VortexSystem.Settings(from: .fire ) { settings in It's generally not a good idea to mix up logic and layout in the body property, so I think for the README it might be a better idea to pull this settings object out into its own property. — Reply to this email directly, view it on GitHub <#29 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBIVEXLUWD7JY4WNM6DD2ELLYTAVCNFSM6AAAAABTFO5WPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWGQ4DGMRXGQ>. You are receiving this because you authored the thread.

[disc0infern0]

Agreed

…

On 7 Dec 2024, at 11:27, Paul Hudson ***@***.***> wrote: @twostraws commented on this pull request. In Sources/Vortex/System/VortexSystem.swift <#29 (comment)>: > - public var angularSpeed: SIMD3<Double> - - /// How much variation to allow in particle spin speed. - public var angularSpeedVariation: SIMD3<Double> - - // These properties determine how particles are drawn. - /// What colors to use for particles made by this system. If `randomRamp` is used - /// then this system picks one possible color ramp to use. - public var colors: ColorMode { - didSet { - if case let .randomRamp(allColors) = colors { - self.selectedColorRamp = Int.random(in: 0..<allColors.count) - } + + /// The configuration settings for a VortexSystem + var vortexSettings: VortexSystem.Settings I think this should just be settings. — Reply to this email directly, view it on GitHub <#29 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBITN7Z6YH32IHG35AYL2ELLRXAVCNFSM6AAAAABTFO5WPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWGQ4DGMBSHE>. You are receiving this because you authored the thread.

[disc0infern0]

It doesn’t offer much, that’s true. The only thing it does is allow the entire settings, including secondary settings, to be created within the closure. Like the example below. It’s a very marginal thing however, so I’m happy to remove it if you don’t like the shape. public static let fireworks = VortexSettings { settings in settings.tags = ["circle"] settings.position = [0.5, 1] settings.birthRate = 2 settings.emissionLimit = 1000 settings.speed = 1.5 settings.speedVariation = 0.75 settings.angleRange = .degrees(60) settings.dampingFactor = 2 settings.size = 0.15 settings.stretchFactor = 4 var sparkles = VortexSystem.Settings { sparkle in sparkle.tags = ["sparkle"] sparkle.spawnOccasion = .onUpdate sparkle.emissionLimit = 1 sparkle.lifespan = 0.5 sparkle.speed = 0.05 sparkle.angleRange = .degrees(180) sparkle.size = 0.05 } var explosions = VortexSystem.Settings { explosion in explosion.tags = ["circle"] explosion.spawnOccasion = .onDeath explosion.position = [0.5, 0.5] explosion.birthRate = 100_000 explosion.emissionLimit = 500 explosion.speed = 0.5 explosion.speedVariation = 1 explosion.angleRange = .degrees(360) explosion.acceleration = [0, 1.5] explosion.dampingFactor = 4 explosion.colors = .randomRamp( [.white, .pink, .pink], [.white, .blue, .blue], [.white, .green, .green], [.white, .orange, .orange], [.white, .cyan, .cyan] ) explosion.size = 0.15 explosion.sizeVariation = 0.1 explosion.sizeMultiplierAtDeath = 0 } settings.secondarySettings = [sparkles, explosions] }

…

On 7 Dec 2024, at 11:26, Paul Hudson ***@***.***> wrote: @twostraws commented on this pull request. In Sources/Vortex/Settings/Settings.swift <#29 (comment)>: > + /// - Parameters: + /// - from: `VortexSettings` + /// The base settings struct to be used as a base. Defaullt settings will be used if not supplied. + /// - : @escaping (inout VortexSettings)->Void + /// An anonymous closure which will modify the settings supplied in the first parameter + /// e.g. + /// ```swift + /// let newFireSettings = VortexSystem.Settings(from: .fire ) { settings in + /// settings.position = [ 0.5, 1.03] + /// settings.shape = .box(width: 1.0, height: 0) + /// settings.birthRate = 600 + /// } + /// ``` + public init( + from base: VortexSystem.Settings = VortexSystem.Settings(), + _ modifiedBy: @escaping (_: inout VortexSystem.Settings) -> Void This is an odd shape for an initializer. What does it offer over code like this: var settings = VortexSystem.Settings(from: .fire) settings.idleDuration = 3 settings.angle = .whatever — Reply to this email directly, view it on GitHub <#29 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBIWI475YXRO2XRRNPCL2ELLO5AVCNFSM6AAAAABTFO5WPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWGQ4DEOJUGY>. You are receiving this because you authored the thread.

[disc0infern0]

Re. Odd shaped initialiser: The difference is highlighted more succinctly in use between these two Previews, which both do the same thing. A closure is required in both cases, but the second one seems neater, and avoids the ` { ..return..}()` pattern which I hoped to avoid. What do you think? versus #Preview("Demonstrates the fire preset with attraction") { /// Here we modify the default fire settings to extend it across, and just slightly below, the bottom of the screen let floorOnFire:VortexSettings = { var settings = VortexSettings(basedOn: .fire) settings.position = [0.5, 1.02] settings.shape = .box(width: 1.0, height: 0) settings.birthRate = 600 return settings }() VortexView(floorOnFire) { Circle() .fill(.white) .frame(width: 32) .blur(radius: 3) .blendMode(.plusLighter) .tag("circle") } } versus #Preview("Demonstrates the fire preset with attraction") { /// Here we modify the default fire settings to extend it across, and just slightly below, the bottom of the screen let floorOnFire = VortexSettings(basedOn: .fire) { settings in settings.position = [0.5, 1.02] settings.shape = .box(width: 1.0, height: 0) settings.birthRate = 600 } VortexView(floorOnFire) { Circle() .fill(.white) .frame(width: 32) .blur(radius: 3) .blendMode(.plusLighter) .tag("circle") } }

…

On 7 Dec 2024, at 15:18, Gn ail ***@***.***> wrote: It doesn’t offer much, that’s true. The only thing it does is allow the entire settings, including secondary settings, to be created within the closure. Like the example below. It’s a very marginal thing however, so I’m happy to remove it if you don’t like the shape. public static let fireworks = VortexSettings { settings in settings.tags = ["circle"] settings.position = [0.5, 1] settings.birthRate = 2 settings.emissionLimit = 1000 settings.speed = 1.5 settings.speedVariation = 0.75 settings.angleRange = .degrees(60) settings.dampingFactor = 2 settings.size = 0.15 settings.stretchFactor = 4 var sparkles = VortexSystem.Settings { sparkle in sparkle.tags = ["sparkle"] sparkle.spawnOccasion = .onUpdate sparkle.emissionLimit = 1 sparkle.lifespan = 0.5 sparkle.speed = 0.05 sparkle.angleRange = .degrees(180) sparkle.size = 0.05 } var explosions = VortexSystem.Settings { explosion in explosion.tags = ["circle"] explosion.spawnOccasion = .onDeath explosion.position = [0.5, 0.5] explosion.birthRate = 100_000 explosion.emissionLimit = 500 explosion.speed = 0.5 explosion.speedVariation = 1 explosion.angleRange = .degrees(360) explosion.acceleration = [0, 1.5] explosion.dampingFactor = 4 explosion.colors = .randomRamp( [.white, .pink, .pink], [.white, .blue, .blue], [.white, .green, .green], [.white, .orange, .orange], [.white, .cyan, .cyan] ) explosion.size = 0.15 explosion.sizeVariation = 0.1 explosion.sizeMultiplierAtDeath = 0 } settings.secondarySettings = [sparkles, explosions] } > On 7 Dec 2024, at 11:26, Paul Hudson ***@***.***> wrote: > > > @twostraws commented on this pull request. > > In Sources/Vortex/Settings/Settings.swift <#29 (comment)>: > > > + /// - Parameters: > + /// - from: `VortexSettings` > + /// The base settings struct to be used as a base. Defaullt settings will be used if not supplied. > + /// - : @escaping (inout VortexSettings)->Void > + /// An anonymous closure which will modify the settings supplied in the first parameter > + /// e.g. > + /// ```swift > + /// let newFireSettings = VortexSystem.Settings(from: .fire ) { settings in > + /// settings.position = [ 0.5, 1.03] > + /// settings.shape = .box(width: 1.0, height: 0) > + /// settings.birthRate = 600 > + /// } > + /// ``` > + public init( > + from base: VortexSystem.Settings = VortexSystem.Settings(), > + _ modifiedBy: @escaping (_: inout VortexSystem.Settings) -> Void > This is an odd shape for an initializer. What does it offer over code like this: > > var settings = VortexSystem.Settings(from: .fire) > settings.idleDuration = 3 > settings.angle = .whatever > — > Reply to this email directly, view it on GitHub <#29 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBIWI475YXRO2XRRNPCL2ELLO5AVCNFSM6AAAAABTFO5WPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWGQ4DEOJUGY>. > You are receiving this because you authored the thread. >