-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(web/server): merge faces #3121
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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 haven't had the time to take a proper look at this, sorry. It looks mostly good, though there are a lot of individual SQL operations happening. I have a feeling a majority of this could be consolidated and would be both more performant and more clear.
} | ||
} | ||
|
||
private async getPersonWithMostAssets(ids: string[]): Promise<string> { |
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 feel like this could probably be just one SQL query.
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.
Personally I think the client can indicate the primary person. People are sorted by count so the web could always use the first named person to achieve the same thing. The server code would be a lot cleaner if it worked like that.
Apologies for my unsolicited opinion, but I personally think there could be value in allowing users to customize the order of people in their people page - hence, I think abstracting away the merging logic to the server could have its benefits. |
The only difference between merging Person A into Person B opposed to merging Person B into Person A is:
I don't see any advantage/benefit to adding a lot of complicated code in the server in order to automatically infer who's name and thumbnail you want to keep. Specifically by looking at asset counts. IMO it should be specified in the API request, by the client, and I think it would work fine if the web just used the first selected person or the left-most person with a name as the algorithm for that. The complexity server-side would be considerably simpler. (even in the case the user got it wrong, the name and thumbnail can be easily changed after the merge too) |
It seems I may have slightly misunderstood your intent in your previous comment, my apologies. I agree with not using asset counts to determine the 'original' person - it does seem like a lot of work for what could be, to be frank, a coin toss (determinism is always better though); it might as well be specified by the client. |
Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>
Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>
This PR adds the mechanism to merge faces and assets belonging to the faces for the Facial Recognition feature.
Screencast.from.2023-07-09.20-23-16.webm