nrkno / sofie-atem-connection

Sofie ATEM Connection: A Part of the Sofie TV Studio Automation System
https://github.com/nrkno/Sofie-TV-automation/
MIT License
134 stars 36 forks source link

feat: support for flying key 'Run to' (to keyframe and to infinite) #102

Closed endreszabo closed 3 years ago

endreszabo commented 3 years ago

Feature added: Flying keys 'Run to' feature implemented (at least it's called this way in original GUI).

There's no support for 'Run to'.

Flying key Run to Keyframe and Run to Infinite (in specific directions) are now supported. Keyframe size and location can't be edited right now.

Be easy on me, I'm not a coder. I made this change against the master tree because the README.md did not instruct me to use another one. Also I'm absolutely not sure if using of this.flag in the send buffer is appropriate in this case. I'd add test and documentation if I'd find a close match to this code. I used tcpdump to capture traffic while using GUI. Also I checked the source of other ATEM control software for actual command parameters.

endreszabo commented 3 years ago

Hmm, surprisingly, there were test cases for this RFlK ATEM command. Discovering them too late, it is not surprising that the argument names don't match my function names.

Julusian commented 3 years ago

Thanks for the contribution

Yes, there are unused test cases for a lot of the commands as most of this library is based on https://github.com/LibAtem/LibAtem, and the testcases are generated from that. It has some tests comparing it to the official sdk, so by proxy that means the testcases should be pretty accurate, but could be a bit outdated

Unfortunately this does mean that you didn't need to dive into it with tcpdump to figure out the commands, I should write some contribution guidelines for future refence

I havent fully read through the code yet, as it has been a while and I need to remind myself about this codebase, but could you define enums in src/enums/index.ts for the keyframe and direction? You can base them on the ones at https://github.com/LibAtem/LibAtem/blob/27e1f7e3b6236eb1929f42b6b3cabd0575ee8652/LibAtem/Common/Id.cs#L110

endreszabo commented 3 years ago

Ok, somewhy the first byte in command must be 0x02, or at least the second bit must be set to make the Run to Infinite work.

Also, I have no idea why all the tests fail. :) In the packet debug output I get the right bytes set and the commands also work on my Mini Pro.

endreszabo commented 3 years ago

Thank you for your feedback. I copied over those two enum definitions. But I guess they should be referenced in the final .ts command file to be checked as valid argument values. Tests started to show up values different than all 0x00s. :)

Julusian commented 3 years ago

@endreszabo I have taken a proper look at this and made a few tweaks to your change. You can follow along with the commits as I kept it in stages to make it clear what I was changing and why.

The tests were failing partly because you had assigned the keyframeId and direction properties on the class, as well as inside the properties object. The tests expect them to be in the properties object. Additionally, as you pointed out, the naming between the test data and the class dont match up. This is fine, but needed the translation map to be updated. Then finally, the test data was bad. You are right in that the first byte needs to be a 2 when doing a run to infinite, so I fixed up that data too (not expected for anyone outside the maintainers to do that)

Finally, I used the enums you added, and changed the api method you added to make it clearer on how to use it correctly (because the direction is only valid when the keyframe is 'infinite')

I am now happy with the changes, so shall merge them soon

Julusian commented 3 years ago

Sorry for the delay, this is published in v2.2.2