-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Creating of VortexSystem.Settings #21
base: main
Are you sure you want to change the base?
Conversation
…se;- * Codable: Removed manual codable settings in favour of automated conformance, made possible by adding Angle conformance to Codable. * Initialisers: To avoid ever having to use an overlong initialiser, default settings have been added throughout. * Seperation of vortex configuraton settings to a new Struct; VortexSystem.Settings, and removal of secondarySystems in favour of secondarySettings. * Use of VortexSystem is mostly unchanged through use of dynamicLookup * New presets for Settings are created, which can easily be modified by a custom init for Settings that takes a closure to modify a given Settings struct. * New SwiftUI previews are created for each of the presets. (some previews are macOS only) * Updated the Fireflies preset to allow both attraction and replusion through use of the Option key modifier on drag * Eliminated the need to ever use .makeUniqueCopy() on a Vortex System. It now returns self. * Added default symbols to the VortexView initialiser, so that circle, triangle and sparkle can now be used without explicitly adding a trailing closure of symbols. * Added automatic conformance to Equatable and Hashable for any type conforming to Identifiable.
Thank you for this great piece of work! It looks really useful, and I'm keen to get these changes merged in so everyone can benefit from them. However, I'm not in a position to review a pull request this big – you've changed practically every file 😅 Is there any way for us to work through these in smaller, discrete chunks – if you submit a PR for one of the changes, we review it, merge it in, then repeat for another chunk? It would ensure all this functionality gets reviewed thoroughly and with all the attention it deserves. It would be easier for both of us, and I think would yield a better result for the code – if it's possible, of course. Thank you! |
Many thanks for replying so quickly Paul, and especially for your encouragement.
Looks like we are in for a journey together on this :) I'll create a new branch/baseline and re-do the changes.
Where to start is the question.
Do you have a preference as whether I start small, perhaps with default Symbols, then Automatic codable conformance, then the attraction/repulsion change to Fireflies previews, and work up to the extraction of Settings from VortexSystem?
Or the other way around, starting with the Settings change?
As an aside, for the future I’d like to experiment with creating an ‘onClip’ event that would fire when a particle was removed when going out of bounds. - The idea is that we could then merge the rain and splash presets - making a splash where a rain particle clipped the lower edge. It’s perhaps gilding the lily really since the effect you already have is good, but I’m turning the idea over :).
I admit to being rather prone to gilding - if you have time, do have a look at another package on my GitHub page - Slider - it’s a dupe of the Apple Slider but extremely stylable, even to a radial presentation. - I started looking at Vortex because I thought it might be nice to leave a sparkly trail when moving the thumb button (that’s in progress locally)
Best wishes,
Andrew
… On 3 Dec 2024, at 10:52, Paul Hudson ***@***.***> wrote:
Thank you for this great piece of work! It looks really useful, and I'm keen to get these changes merged in so everyone can benefit from them. However, I'm not in a position to review a pull request this big – you've changed practically every file 😅
Is there any way for us to work through these in smaller, discrete chunks – if you submit a PR for one of the changes, we review it, merge it in, then repeat for another chunk? It would ensure all this functionality gets reviewed thoroughly and with all the attention it deserves. It would be easier for both of us, and I think would yield a better result for the code – if it's possible, of course.
Thank you!
—
Reply to this email directly, view it on GitHub <#21 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JBIQVQDF7LU4OVQES6NT2DWEOZAVCNFSM6AAAAABS5OPHOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJUGIYDKOJWGE>.
You are receiving this because you authored the thread.
|
Thank you! That sounds very reasonable – it sounds more or less like smallest to largest, which is definitely preferable. For each step, make sure to add a little context on why a change works well, perhaps with a before/after code snippet if you can. For example, default symbols seems like it would make a lot of code easier, but does it work well for someone importing the project through SPM?
Definitely one for a later PR 😅
Adding interesting features and functionality is usually great, as long as we find the balance between "making things better" and "this breaks the framework for folks who are using it." |
VortexSystem.Settings
[ ] Removed the configuration settings from the VortexSystem, so that the system class is now solely responsible for drawing.
[ ] Eliminated the need to ever use .makeUniqueCopy() on a Vortex System. It now returns
self
for backward compatibility[ ] Settings can easily be created and modified outside of the package through use of a custom init for Settings that takes a closure to modify a given Settings struct. All existing VortexSystem presets have been recreated as Settings presets.
[ ] Initialisers: To avoid ever having to use an overlong initialiser, default settings have been added throughout. Commenting of variables has been supplemented with the default values to be used.
[ ] Removal of secondarySystems in favour of secondarySettings.
[ ] Use of VortexSystem is mostly unchanged through use of dynamicLookup ( excepting storage of secondarySystems)
[ ] New SwiftUI previews are created for each of the Settings presets. (some previews are macOS only)
[ ] VortexSystem's are only created by the VortexView or when initialising secondary systems.
[ ] The Vortex View now directly takes a Settings struct, and constructs the particle system in the init
[ ] Added automatic conformance to Equatable and Hashable for any type conforming to Identifiable.
Misc:
(This is only the second pull request I have ever created, so if there are mistakes, or breaches in etiquette, please let me know as I'm still learning how all of this works! Thankyou. )