Flat Spectrogram View #82
Conversation
|
Thanks for getting onto that Spectrogram. I'm sorry about the many hound findings, didn't know about SwiftLint. Added it to my project so my future PRs won't suffer from colon violations and the like. |
|
If you want to give me write privileges to your repo, I can fix things up there and this PR will be updated, or else I can fork yours and make a new pr off of it. Best to address these things prior to merging. |
|
Sure, added you as collaborator. |
…ics, just syntax.
|
Hey @aure I have fixed all but one hound linter warning. Will read more about protocol Identifiable, maybe use an lint exception. |
| // Default: 2048 | ||
| var fftSize = 4096/2 | ||
|
|
||
| // how/why can the sample rate be edited? Shouldn't this come from the node/engine? |
There was a problem hiding this comment.
yes full agree sample rate needs to come from engine
| @available(iOS 17.0, *) | ||
| struct SpectrogramSlice: View, Identifiable { | ||
| static var counterSinceStart = 0 | ||
| let id: Int |
There was a problem hiding this comment.
we probably need an edge case for hound, id is a perfectly acceptable variable name
| @available(iOS 17.0, *) | ||
| struct SpectrogramSlice: View, Identifiable { | ||
| static var counterSinceStart = 0 | ||
| let id: Int |
| var frequencyChosen = 0.0 | ||
| var points: [CGPoint] = [] | ||
|
|
||
| for index in 1 ... (fftFloats.count / 2) { |
There was a problem hiding this comment.
this for loop is unsafe, there should be some guards prior to avoid division by zero etc...
| if frequencyForBin > Double(spectrogramMaxFreq) { continue } | ||
| frequencyChosen = frequencyForBin | ||
|
|
||
| if frequencyForBin > 8000 { |
There was a problem hiding this comment.
magic number, should be a constant
|
|
||
| // unused method drawing into a Canvas. Might be useful in the future | ||
| // when doing more energy efficent drawing | ||
| func createSpectrumSlice() -> some View { |
There was a problem hiding this comment.
I haven't run the code, but I don't understand what we gain from representing the spectrogram as a series of UIImage over drawing in Canvas or even CoreGraphics. I imagine that the current UIImage route is expensive and will never look as nice. At the very least I'd like an explanation why this strategy was employed, and its pros/cons, and what help you need to do this directly in Canvas.
| /// a central feature in your app. Furthermore it's not scientificicly correct, when displaying | ||
| /// white noise, it will not show a uniform distribution. | ||
|
|
||
| @available(iOS 17.0, *) |
There was a problem hiding this comment.
Unclear what API requires iOS 17, this should be explained.
| // Choose a higher value when you want to analyze low frequencies, | ||
| // choose a lower value when you want fast response and high frame rate on display. | ||
| // Default: 2048 | ||
| var fftSize = 4096/2 |
There was a problem hiding this comment.
this probably should be an enum and document in the enum the tradeoffs of the different fft sizes
|
|
||
| /// Model for the SpectrogramFlatView. Makes connection to the audio node and receives FFT data | ||
| @available(iOS 17.0, *) | ||
| class SpectrogramFlatModel: ObservableObject { |
There was a problem hiding this comment.
if this code does indeed require iOS 17, then no reason to use newly deprecated ObservableObject and should use @Observable macro
| // The lowest human bass voice in choral music is reaching down to C1 (32.7 Hz). | ||
| // don't go lower than 6.0, it just doesn't make sense and the display gets terribly distorted | ||
| // don't use 0 as it breaks the display because log10(0) is undefined and this error not handled | ||
| var minFreq: CGFloat = 48.0 |
There was a problem hiding this comment.
do these values change? otherwise move to lets.
There was a problem hiding this comment.
Yes, totally. Moved it not only to let but also to SpectrogramFFTMetaData so it can be opened up as public parameter in future. Ideally the minFreq and maxFreq can be configured on runtime. 3868d03
|
@maksutovic thank you so much for the thorough review and the totally legit questions. Will work on it and push it into the fork. |
…Freq and maxFreq to SpectrogramFFTMetaData
I really need to step up in linting and integrate it better into Xcode.
|
Added swiftlint directive to disable the warning from Hound about the var called |
|
@maksutovic I've added more detail at the top of the class describing the dataflow and history of the class. Drawing is done using UIGraphicsImageRenderer which is Core Graphics, with context.fill primitives. These then are cached as UImage and layouted onto the view. I was struggling and couldn't find a good example on how to animate a Canvas from right to left as well as drawing async on it. I did my fair share of Core Animation development with raskinformac.com but I'm pretty new to SwiftUI. So it didn’t took me long to figure out the bottleneck of the existing SpectrogramView using instruments and stuff, but also couldn't implement what I had in mind using ImageRenderer objectwillchange or an animated Canvas. Or all the other possibilites listed. |
|
@aure @maksutovic You may have another look at this PR. I made the suggested changes as well as added some comments to the code and this PR. |
| // static Int instead of a UUID as identifier. While debugging it's practical | ||
| // to see the order and therefore time the slice was created. | ||
| // Furthermore for the sake of premature performance optimisation: | ||
| // Incrementing an Int could be supposedly faster than creating UUID. |
| @@ -97,7 +97,7 @@ class SpectrogramFlatModel: ObservableObject { | |||
| gradientUIColors: SpectrogramFlatView.gradientUIColors, | |||
| sliceWidth: sliceSize.width, | |||
| sliceHeight: sliceSize.height, | |||
| fftReadingsAsTupels: points, | |||
| fftReadingsFrequencyAmplitudePairs: points, | |||
There was a problem hiding this comment.
Love amplitude pairs, thank you!
There was a problem hiding this comment.
Thanks so much @mahal great updates, looking forward to using this, and iterating on it!
No description provided.