-
Notifications
You must be signed in to change notification settings - Fork 33
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
[SDK-607] Add selfie element #166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good but some suggestions and discussion points in the comments
var webCamDevice = GetWebCamDevice(); | ||
|
||
SetupPhotoBoothTexture(webCamDevice?.name); | ||
StartCamera(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually not relevant to this PR, something I kinda missed in Max's PR. But perhaps we shouldn't force them to start the camera on awake (this InitializeCamera function) is automatically called on awake, starrting the camera is also kinda a hidden "side effect" of initializing. I also wonder if it the automatic GetCamDeveice() on start might be annoying for some as it basically requests access regardless of whether the element is actually used? 🤔 Maybe I'm just overthinking this part though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see the positives for moving out the StartCamera() method to be invoked outside of awake (maybe in a from the editor itself in the element prefab), but then you'd maybe want the whole initialize method to not be on awake right?
Although since this is so easy to change between, maybe it's one of those ones where it's a case of us getting it out there and seeing what partners say. If we get reports that it is annoying then it's easy for us to change it and to respond to feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean about the body type thing for avatar manager, kinda annoying we need to add to the function params. But I do think it's still better then having the multiple places to set body type. Maybe in future there might be a way to improve it even further somehow.
I mention one small thing in the comments but it's not specific to the changes in this PR and is kinda minor.
But overall great work ! And thanks for making the changes 🙏
4c06e5c
to
e55a1ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Cool stuff
SDK-607
Description
How to Test
Checklist