-
Notifications
You must be signed in to change notification settings - Fork 16
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 OSD overlay tool #243
Add OSD overlay tool #243
Conversation
06fb8e8
to
52d1d00
Compare
50044e5
to
7846d02
Compare
Hey @Knifa - thank you first of all for your contribution! |
Let's hang tight for a full review the now --- think I'll try get the UX sorted before I mark it as ready then we can go from there legit! |
Roger that! From what I've seen my biggest "complaint" are the translations. Apart from that everything else is looking nice. Now seeing it, I don't even hate the TS implementation - we can migrate stuff piece by piece to TS at some point. |
@stylesuxx I'll take a review now! The drag/drop component could maybe do with a bit more juice in terms of hover effects, etc. but maybe splitting hairs at this point --- would be good to get it in and make it better from there (or some ideas of what it should do). Let me know what you want from a translations perspective! |
ee57ab6
to
42af374
Compare
@Knifa Regarding translations, I mentioned it in a couple of places. I also see that you have a translation file in place, so if you could just move all your translations there and replace them in the template with the according call to the translation function that would be awesome. You only need to do this for EN, all other languages (as long as the file is present) will be populated by Crowdin anyway. Could you maybe also attach some (short) sample input and expected output files? I'd like to give it a run later on, but might not have my devices available... |
Thanks! I'll go ahead and put the translations placeholders in shortly. I've just added a debug panel as a last little extra there to try diagnose some of the issues we're seeing where the process doesn't complete on some machines --- don't expect to add anything more substantial beyond this. Also, I thought about putting together a bit of a flow diagram if it's helpful? Test pack with inputs + example output: https://drive.google.com/file/d/1bfOlO9alk1ncx8DFv7En8ONrZQ8op50G/view?usp=sharing |
Awesome, thank you. Flow diagram sounds great, the more documentation the better ;-) Feel free to add a section to the readme explaining the process. Also thanks for the test files - greatly appreciated. |
I totally reworked the processing pipeline today after realising some things I hadn't before with the WebCodecs API --- think this should resolve some of the weird problems we've been seeing AND it's much more obvious what the flow is, I think. Still plan to put together some documentation. Everywhere that has text uses the translations now, hopefully it's all OK! |
@Knifa - thank you. I still see some translations that are not in the translations files - just wanted to ask if you missed to push them. It's not a big deal, I can add them myself too. Apart from the documentation - are you happy with the current state right now, because if so, I would give it a final look over and get ready for merging. I would really like to release a new version with all the other features and would like to include the overlay, but I don't want to put you under pressure - if you feel you need more time, then I will simply do an intermediate release and add this feature later on. Just thought it'd be cool to have this awesome feature included as one of the shiny, nice, new things :-) |
@stylesuxx: Sorry, the nav and tiles totally slipped my mind! That's them added now. And yeah, please go ahead and give me the review. I'm happy with how it's put together, generally! |
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.
Alright, generally speaking everything is looking (and working) very well, just some minor nit picks are still left.
I would just ask you to go over the "TODOs" and add them into an issue, could be a collection of all the TODOs, or split up in multiple issues - how you see fit. This way people could take up the task of implementing those TODOs. In my personal experience, if they are only in code, they are left there to die and nobody is ever going to look at them again...
Thanks for your review @stylesuxx! Think that's it all tidied up. I've created an issue to track the remaining TODOs: #278 |
This is looking good - thank you so much! I will give it another test run tomorrow and merge it if I can't find anything else to nit pick :-) In case you are keen to keep some things in separate commits, please squash, otherwise I will squash everything into a single commit. |
Nice, thanks! Happy to have it all squashed into one commit when merged. 👍 |
5c94bf5
to
9aa3a41
Compare
9aa3a41
to
f94a442
Compare
@stylesuxx I've sorted the conflicts that cropped up from the |
@Knifa - thank you for your patience and contribution - finally merged. |
Time to start pulling this together! I think it works well enough that it's worth getting it merged and dealing with it from there.
Tried to keep the TS as separate as possible as suggested. All the main functionality is under
osd-overlay
in TS in it's own little bubble. The UX components are plain JS.All of the processing is done in a web worker to keep it all separate on that front.
Remaining WIP tasks, I think:
The MP4 parsing, etc. needs broken out into it's own library because it's absolutely useful for other things. Bit of a longer term goal for me.