search

AudioKit / AudioKitUI

Controls and Visualization for AudioKit apps
MIT License
187 stars 52 forks source link

MIDITrackView does too much work in body #46

Closed wtholliday closed 5 months ago

wtholliday commented 2 years ago

It creates an AppleSequencer and reads files. Need to create a MIDITrackViewModel to store that.

emurray2 commented 2 years ago

It creates an AppleSequencer and reads files. Need to create a MIDITrackViewModel to store that.

Yeah I made some improvements to the code which is available on my repo emurray2/SimpleMIDI . I can create a model which is injected into the view and only created once. That way there won't be multiple sequencers.

emurray2 commented 2 years ago

@wtholliday What do you think about the new model? If you see any improvements to be made, please let me know. I'm a beginner still when it comes to MVVM, and I appreciate any feedback you have to offer. I'll start working on #47 this weekend when I get home for spring break.

wtholliday commented 2 years ago

Hey @emurray2 , it looks like we could do some more work on it. I'll have to look in greater detail at some point, but it looks like it's still effectively doing the same work, just now here: https://github.com/AudioKit/AudioKitUI/blob/8247c8f772f1917f63579347ec29c54900597aff/Sources/AudioKitUI/Controls/MIDITrackView.swift#L104 instead of in the body. I think updateUIView is going to be called every time body is run. Did you profile it? I should have included some repro steps, because IIRC the Cookbook app was slow.

I also don't think it needs separate per-platform implementations. Should be entirely doable with SwiftUI, especially if we can use Canvas.

emurray2 commented 2 years ago

Hey @emurray2 , it looks like we could do some more work on it. I'll have to look in greater detail at some point, but it looks like it's still effectively doing the same work, just now here:

https://github.com/AudioKit/AudioKitUI/blob/8247c8f772f1917f63579347ec29c54900597aff/Sources/AudioKitUI/Controls/MIDITrackView.swift#L104 instead of in the body. I think updateUIView is going to be called every time body is run. Did you profile it? I should have included some repro steps, because IIRC the Cookbook app was slow.

I also don't think it needs separate per-platform implementations. Should be entirely doable with SwiftUI, especially if we can use Canvas.

Hi Taylor, thank you for taking the time to help me with this. As I mentioned, I really appreciate your feedback, especially since I don't know much about drawing graphics and SwiftUI yet. I see what you mean now by everything being moved to the UIViewRepresentable makeUIView() and updateUIView(). I did some profiling, and the newer version possibly works better than the previous, as the track position is now stored inside MIDITrackViewModel. I believe changing this state before in the UIViewRepresentable before was causing updateView() to be called every frame if I'm remembering right, which made the animations very choppy and slow. Now it will only call the view's update function when the track's file changes, the view is first created, or the width/height change. However, I still agree--this seems like a lot of work drawing out the view/whole file whenever its state changes. And the per-platform implementations are definitely convoluted with duplicate code. I'll explore some more this weekend and see if I can get things working with Canvas (in tandem with the MIDITrackViewModel which currently stores the track position).