make it usable for macOS and iOS #2
Conversation
There was a problem hiding this comment.
Hi @sakrist, this generally looks great, thank you!
I've got a few changes I'd prefer you make before merging it, to keep things clean, docs accurate, etc. And in a few places, I've asked for details so that I can back-fill in documentation on the added RealityView. I don't expect you to update all the docs, but I would like details about what all the pieces are and their intentions so that I can do so after merging this addition to support iOS as well as macOS.
| var cancellables = [Cancellable]() | ||
|
|
||
| public struct RealityKitView: View { | ||
| public typealias UpdateBlock = () -> Void |
There was a problem hiding this comment.
Since this (UpdateBlock) is public, give me some detail of what it's being used for, and why a developer would include an updateBlock closure in their RealityKitView initializer. (Ideally, this would have documentation to explain it - but I'm happy to write it up if you can detail what the intent is for me)
There was a problem hiding this comment.
I'm going to remove alias :)
| self.update = update | ||
|
|
||
| if let update = self.update { | ||
| let updateCa = arContainer.arView.scene.subscribe(to: SceneEvents.Update.self) { event in |
There was a problem hiding this comment.
can we name updateCa a more descriptive variable? I'm a little confused on this point - it makes sense to get the various SceneEvents as they're triggered, but you're not passing them down into the closure, so I'm not understanding the purpose of this all.
There was a problem hiding this comment.
I will rename updateCa to updateCancellable probably forget to finish typing :)
Idea with update is that we can do something each frame. Update is optional argument.
|
@heckj should we rename RealityKitView to RealityView? 😉 |
|
As a lower level component, I think keeping it named for the renderer that it presents (and not shortening it) makes a lot more sense. |
|
I might redesign it and will make proposal request. This actually inspired me at the time :) |
|
Sounds great - it's an interesting design. And all the more reason (IMO) to not call it RealityView to keep from confusing it with a (new) extant Apple class for SwiftUI |
I've added SwiftUI view. Can be used for prototyping on macOS.
