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

Add native PTZ element #807

Merged
merged 4 commits into from
Sep 3, 2022
Merged

Add native PTZ element #807

merged 4 commits into from
Sep 3, 2022

Conversation

dermotduffy
Copy link
Owner

@dermotduffy dermotduffy commented Aug 17, 2022

elements:
  - type: custom:frigate-card-conditional
    conditions:
      mediaLoaded: true
      view:
        - live
    elements:
      - type: custom:frigate-card-ptz
        orientation: horizontal
        service: homeassistant.toggle
        style:
          transform: none
          right: 20px
          top: 180px
        data_left:
          entity_id: light.office_main_lights

@dermotduffy
Copy link
Owner Author

@felipecrs Give this a shot? I'm not sure if this should be a custom element, or 'builtin' but went with element for now. Let me know what you think. You can set orientation to horizontal or vertical.

@felipecrs
Copy link
Contributor

I'll surely try ASAP.

@felipecrs
Copy link
Contributor

It looks great, but I think you should allow the user to select some preset position instead of asking them to make it pixel-based (which is one of the problems I reported: pixel-based locations do not work well when in different DPIs).

chrome_usoqQD8A2x.mp4

How about a new attribute like position, which can be: left, center, right and another attribute called alignment which could be top, center, bottom. So that I could set the custom element to be positioned at bottom-right.

And I think it's a good decision keeping it as a custom element.

@dermotduffy
Copy link
Owner Author

dermotduffy commented Aug 20, 2022

(which is one of the problems I reported: pixel-based locations do not work well when in different DPIs).

I'm not following why you think this will be different in code, unless you want active measurement of some kind. i.e. why can't you just put the positions in %s so that they scale with the card viewport, e.g. have you tried:

      - type: custom:frigate-card-ptz
        orientation: horizontal
        service: homeassistant.toggle
        style:
          transform: none
          right: 5%
          top: 50%
[...]

PS: If you have a specific test scenario / config let me know.

How about a new attribute like position, which can be: left, center, right and another attribute called alignment which could be top, center, bottom. So that I could set the custom element to be positioned at bottom-right.

I definitely could do this, although it felt like a fairly personal choice that presets may just get in the way of. Also I could imagine you need different values depending on where you have the menu, whether you have thumbnails on/off, whether you have the next/previous controls, etc -- much easier to just the user put it wherever they want.

@felipecrs
Copy link
Contributor

I'm not following why you think this will be different in code, unless you want active measurement of some kind. i.e. why can't you just put the positions in %s so that they scale with the card viewport, e.g. have you tried:

      - type: custom:frigate-card-ptz
        orientation: horizontal
        service: homeassistant.toggle
        style:
          transform: none
          right: 5%
          top: 50%
[...]

It works like a charm. I would only suggest to add this to the README, as I think it should work in more cases than what is there currently:

style:
  transform: none
  right: 20px
  top: 180px

I definitely could do this, although it felt like a fairly personal choice that presets may just get in the way of. Also I could imagine you need different values depending on where you have the menu, whether you have thumbnails on/off, whether you have the next/previous controls, etc -- much easier to just the user put it wherever they want.

Sorry, I didn't realize I could do what you suggested earlier, and that covers all my needs.

I would say this PR is ready, and I like it a lot!

@felipecrs
Copy link
Contributor

Another feedback: even when using mediaLoaded: true, the PTZ buttons shows on top of the reconnecting message. I also noticed it being displayed on top of the Frigate splash image (the bird) when it quickly displayed.

Screenshot_20220830-170832_Home Assistant

@felipecrs
Copy link
Contributor

felipecrs commented Aug 30, 2022

Another feedback: even when using mediaLoaded: true, the PTZ buttons shows on top of the reconnecting message. I also noticed it being displayed on top of the Frigate splash image (the bird) when it quickly displayed.

Filtering the issue, it seems to only happen after it loaded once:

chrome_gEn527s1Hc.mp4

@dermotduffy dermotduffy merged commit 8cac46f into release-4.0.0 Sep 3, 2022
@dermotduffy dermotduffy deleted the native-ptz-wo-hover branch September 3, 2022 22:34
@dermotduffy
Copy link
Owner Author

@felipecrs I believe I have fixed those issues. I still found I can very occasionally get it to happen (the PTZ rendering on top of the loading spinner) but that is because JSMPEG appears to be actually reporting the canvas as having been loaded when it is not. I haven't tracked that down further, as I think this is good enough for now.

@felipecrs
Copy link
Contributor

Awesome! :D

@felipecrs felipecrs mentioned this pull request Sep 5, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants