Skip to content
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): manual face tagging and deletion #16062

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Conversation

alextran1502
Copy link
Contributor

@alextran1502 alextran1502 commented Feb 12, 2025

This PR adds the ability to tag a face area and delete identified faces from the asset

Tagging interface

image

Removal option

image

server/src/controllers/person.controller.ts Outdated Show resolved Hide resolved
server/src/db.d.ts Outdated Show resolved Hide resolved
server/src/repositories/person.repository.ts Outdated Show resolved Hide resolved
@@ -717,4 +719,25 @@ export class PersonService extends BaseService {
height: newHalfSize * 2,
};
}

async tagFace(auth: AuthDto, dto: TagFaceDto): Promise<void> {
await this.requireAccess({ auth, permission: Permission.PERSON_READ, ids: [dto.personId] });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could argue this should require PERSON_WRITE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PERSON_WRITE or PERSON_CREATE is to create a new person in the person table. Here you need to be able to read the person list in order to create a new entry in the asset_faces table

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should be a new permission like Permission.FACE_CREATE

@bo0tzz bo0tzz added the preview label Feb 12, 2025
@bo0tzz
Copy link
Member

bo0tzz commented Feb 12, 2025

Awesome!

Instead of "delete face" and a bin icon, I would suggest "unlink face" and something like https://pictogrammers.com/library/mdi/icon/link-off/.

I think the list of names in the editor is broken on dark mode.

server/src/controllers/face.controller.ts Outdated Show resolved Hide resolved
server/src/controllers/face.controller.ts Outdated Show resolved Hide resolved
Comment on lines +191 to +199
@ApiProperty({ type: 'integer' })
@IsNotEmpty()
@IsNumber()
imageWidth!: number;

@ApiProperty({ type: 'integer' })
@IsNotEmpty()
@IsNumber()
imageHeight!: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be preferable to move this server-side and infer it from the file. Either by getting metadata from sharp or by just storing the preview dimensions in the database in the asset_files table.

server/src/dtos/person.dto.ts Outdated Show resolved Hide resolved
Comment on lines 222 to 228
export class DeleteFaceDto {
@ValidateUUID()
assetFaceId!: string;

@ValidateUUID()
personId!: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't include personId and the access check can infer the person from the assetFaceId

server/src/repositories/person.repository.ts Outdated Show resolved Hide resolved
server/src/services/person.service.ts Outdated Show resolved Hide resolved
@@ -717,4 +719,25 @@ export class PersonService extends BaseService {
height: newHalfSize * 2,
};
}

async tagFace(auth: AuthDto, dto: TagFaceDto): Promise<void> {
await this.requireAccess({ auth, permission: Permission.PERSON_READ, ids: [dto.personId] });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should be a new permission like Permission.FACE_CREATE

@alextran1502
Copy link
Contributor Author

@jrasm91 @mertalev Added soft delete mechanism to asset_faces

@bo0tzz bo0tzz added preview and removed preview labels Feb 13, 2025
Copy link
Contributor

Deploying preview environment to https://pr-16062.preview.internal.immich.cloud/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants