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

added new functionality to use compreface subjects for facial recognition #845

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

tantonj
Copy link
Contributor

@tantonj tantonj commented Nov 30, 2024

Added new compreface face_recognition config option "use_subjects" which defaults to false. When set to true the binary face recognition entities will instead be defined by the subjects compreface. Added a new endpoint /compreface/update_subjects which when called will add to face recognition entities any subject from compreface which is not currently added. This endpoint works regardless of the "use_subjects" config setting. If a user adds a new subject to compreface they will be able to call this endpoint and start receiving face recognition state messages without restarting viseron

…tion

Added new compreface face_recognition config option "use_subjects" which defaults to false. When set to true the binary face recognition entities will instead be defined by the subjects compreface. Added a new endpoint /compreface/update_subjects which when called will add to face recognition entities any subject from compreface which is not currently added. This endpoint works regardless of the "use_subjects" config setting. If a user adds a new subject to compreface they will be able to call this endpoint and start receiving face recognition state messages without restarting viseron
Copy link

netlify bot commented Nov 30, 2024

Deploy Preview for viseron canceled.

Name Link
🔨 Latest commit a203613
🔍 Latest deploy log https://app.netlify.com/sites/viseron/deploys/6750f81909c89f000861bbbf

Copy link
Owner

@roflcoopter roflcoopter left a comment

Choose a reason for hiding this comment

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

Looks good overall but found a few things that needs to change a little!

viseron/__init__.py Outdated Show resolved Hide resolved
vis.add_entity(
component, FaceDetectionBinarySensor(vis, self._camera, face_dir)
)
if not config[CONFIG_USE_SUBJECTS]:
Copy link
Owner

Choose a reason for hiding this comment

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

Need to find a way to move this to the compreface code instead of the base class.
Let me know if you need help with that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this and the part below is because the base class is where entities get created based on the file structure in the face_recognition path This feature stops this from occurring, so the only alternative would be to remove it completely from the base class and implement it in each implementation of it. This was not only less modifications, but seemed more elegant. With the below part, any none-compreface plugin will default to false and implement as usual. The only downside is that a user could turn this option to true for other plugins, and have no entities created.
What do you suggest in order to prevent adding entities at the base class level in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just thought of a way to possibly do this while trying to go to bed, but haven't referenced the code to confirm if it's a good way of doing this.

What if I put an optional boolean parameter that defaults to true in this init method which prevents the entity initialization when false. Then in the compreface implementation pass the value of use_subjects?

You'll end up seeing this before I get a chance to implement this tomorrow, so let me know if this makes sense.

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds like a good solution to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3rd commit addresses this.

@@ -70,6 +73,11 @@
default=DEFAULT_SAVE_FACES,
description=DESC_SAVE_FACES,
): bool,
vol.Optional(
Copy link
Owner

Choose a reason for hiding this comment

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

Since the use_subjects is specific to the compreface integration it should only be included there and not here in the base class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3rd commit addresses this.

@roflcoopter
Copy link
Owner

The pytest CI failure seems to happen due to a new release of types-requests

I have pinned the working version of types-requests and pushed to dev so if you pull dev into your branch that error should hopefully be resolved.

@roflcoopter
Copy link
Owner

And regarding the failure of CI / Check generated docs, since you have added a new config option for Compreface the component docs have to be regenerated (this is sadly not documented in the Developer docs yet)

You need to run python3 -m scripts.gen_docs -c compreface and then commit the changes to config.json

@roflcoopter
Copy link
Owner

Looks good, thank you!

@roflcoopter roflcoopter merged commit a17b53b into roflcoopter:dev Dec 5, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants