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 35mm 3perf & 16mm Foot+Frames Support #8

Merged
merged 59 commits into from
Jul 23, 2023
Merged

Conversation

iluvcapra
Copy link
Contributor

Hello! This is PR #5 now based on dev, I will add some more tests here.

@iluvcapra iluvcapra mentioned this pull request Mar 7, 2023
@peake100
Copy link
Contributor

peake100 commented Mar 7, 2023

Sweet. Just ping me when you want a review.

It's been a bit since I updated this repo and it looks like I need to update some of my CI pipeline to get it working again anyway. Will probably need to wait to do that until the weekend.

Just out of curiosity, how is the lib working out for you? This was, first and foremost, a project for me to learn Rust. Whenever I'm learning a new language I implement this library to wrap my head around both the actual syntax and the supporting toolchain. I haven't gotten the chance to write much Rust since putting this together, but I would like to some day as writing this lib was a good experience and I really enjoyed Rust's philosophy of letting the compiler help you by doing all of the static analysis.

If you have any feedback I'd love to hear it.

@peake100 peake100 self-requested a review March 7, 2023 23:14
@peake100 peake100 changed the title 3perf Implementation Add 35mm 3perf & 16mm Foot+Frames Support Mar 7, 2023
@peake100 peake100 self-assigned this Mar 7, 2023
@peake100 peake100 added the feature New feature or request label Mar 7, 2023
@peake100
Copy link
Contributor

peake100 commented Mar 7, 2023

One other request: can you supply me with an example cutlist or changelist using these formats? I was on the Picture Editorial side of things, so audio turnovers were my only interaction with the Feet+Frames convention, and the projects I worked on were digital and only used the 35mm, 4-perf format.

I don't doubt what you've written is correct, but the Feet+Frames.fractional format is new to me, so I'd just like to see it in action to wrap my head around it. Ideally I'd like to take the logic you've written here and adapt it to the other vtc libs for Python, Elixir, and Go when I get the chance.

Thanks!

@iluvcapra
Copy link
Contributor Author

I'm not sure why the build is failing the checks on Azure, it seems to be in the prebuild somewhere?

I haven't been using the library myself, I pulled it when I was researching an AAF library for rust and it's just the best one out there, the most thorough. That project is kinda on the back burner for now.

I sent out a tweet 😇 to see if anyone can output a 3-perf pull list, I haven't seen one myself in about 15 years since the format is pretty historical now. I wrote the code as an exercise because it's usually one of the first things I do when I learn a new language.

The period in the 3-perf representation isn't a fraction or decimal; in many cases the vendors would use a bullet or a degree symbol. What that is is the module index of the current foot. Since the lcm of 96 and 3 is 288 (96*3), each foot starts on a different perf relative to the frame line, in a cycle that repeats every 3 feet. Frame .0 indicates the first foot (frame 0 starting on the frame line) .1 indicates the second foot in the module (frame 1 of the 2nd foot, starting on perf 1 of frame 22), etc.

@iluvcapra
Copy link
Contributor Author

(In sound we would NEVER request 3 perf footage, we'd actually usually just request 4 perf numbers. The footage codes were used in telecine and that's about it. No negative cutter in LA ever cut a 3-perf show, legend has it Mo Henry attempted it once and declared it impossible.)

@peake100
Copy link
Contributor

peake100 commented Mar 8, 2023

I haven't been using the library myself, I pulled it when I was researching an AAF library for rust and it's just the best one out there, the most thorough. That project is kinda on the back burner for now.

Makes sense, glad to hear it caught your eye. I don't envy you implementing AAF, I had to do some very light AAF parsing in python once and it made my brain leak out of my ears.

If you ever get going on that again and need anything more from the lib, feel free to reach out.

I sent out a tweet 😇 to see if anyone can output a 3-perf pull list, I haven't seen one myself in about 15 years since the format is pretty historical now. I wrote the code as an exercise because it's usually one of the first things I do when I learn a new language.

I appreciate it, though with that additional context, no need on the 3-perf side (though if you can wrangle one for a 16mm show I would still like to glance at that, just to have real-world data to validate with).

If 3-perf is that mythical I am happy to have the theory of it implemented and if someone somewhere decides to prove everyone wrong and runs into a bug while doing it, they can file an issue.

@peake100
Copy link
Contributor

peake100 commented Mar 8, 2023

Oh and the build is failing because Python changed some stuff in how setuptools works and I need to add a setup.py to the project for my build script dependencies to be pulled properly.

That's on my end and not something you are doing. I'll make that update this weekend and then you'll need to rebase on top of that. After that's done CI should work for this branch.

@peake100
Copy link
Contributor

peake100 commented Mar 8, 2023

One last note. Can you take this:

The period in the 3-perf representation isn't a fraction or decimal; in many cases the vendors would use a bullet or a degree symbol. What that is is the module index of the current foot. Since the lcm of 96 and 3 is 288 (96*3), each foot starts on a different perf relative to the frame line, in a cycle that repeats every 3 feet. Frame .0 indicates the first foot (frame 0 starting on the frame line) .1 indicates the second foot in the module (frame 1 of the 2nd foot, starting on perf 1 of frame 22), etc.

And throw it in the doc string? That's great information and I don't mind the docs getting verbose. One of my goals for this lib was also to have it serve as a natural primer to engineers who might not be super familiar with the film industry and timecode, and educate people making stuff with it.

Feel free to throw in the anecdote about Mo Henry as well, that's hilarious.

@peake100 peake100 mentioned this pull request Mar 11, 2023
@peake100
Copy link
Contributor

Hello!

I've fixed the CI pipeline with #9. If you rebase off of the latest dev the pipeline should now run correctly. Go ahead and ping me whenever your PR is ready for merge.

I also updated the lib to fix some Clippy warnings. Looks like I causes a merge conflict with that. Sorry! Let me know if there is anything else you need.

Thanks!

@peake100
Copy link
Contributor

Just checking up on this. Thanks!

@iluvcapra
Copy link
Contributor Author

Hey sorry I'll get to this soon.

src/timecode.rs Outdated Show resolved Hide resolved
src/timecode.rs Outdated Show resolved Hide resolved
src/timecode.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@peake100 peake100 left a comment

Choose a reason for hiding this comment

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

This is coming together! Some thoughts on the API. Apologies on move goal posts a bit.

Let me know if you have the time to address it. If not I can pull the branch and shuffle some code around myself. I want to be mindful that I'm not asking more than you can dedicate to this. Thank you for all the work so far!

src/timecode.rs Outdated Show resolved Hide resolved
@iluvcapra
Copy link
Contributor Author

I should have mentioned, but you can run make lint from the root of the project to run all the linters locally so you don't have to wait for CI to get the results.

This is actually super helpful thanks!

@iluvcapra
Copy link
Contributor Author

Do you know where the misspell tool can be gotten? I'm on macOS and brew doesn't seem to have a tool by that name in the registry.

@peake100
Copy link
Contributor

peake100 commented May 7, 2023

Do you know where the misspell tool can be gotten? I'm on macOS and brew doesn't seem to have a tool by that name in the registry.

Here's the repo for the project: https://github.com/client9/misspell

I personally use the go toolchain to build and install it, but supposedly prebuilt binaries are available too.

@iluvcapra
Copy link
Contributor Author

Just checking in, I’m still working on this it’s pretty close.

@peake100
Copy link
Contributor

peake100 commented Jun 2, 2023

Just wanted to check back and see how it's going. No rush, but if there's anything I can do to help, let me know!

@iluvcapra
Copy link
Contributor Author

I'm literally noodling on it right now lol, it haunts me like a sore tooth.

@iluvcapra
Copy link
Contributor Author

I'm not sure why the test run is failing here.

Copy link
Contributor Author

@iluvcapra iluvcapra left a comment

Choose a reason for hiding this comment

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

Looking good to me.

@iluvcapra
Copy link
Contributor Author

All of the tests were completing last time I looked at this, I can't remember what was failing in Azure and it's lost the report.

@peake100
Copy link
Contributor

I think I need to rotate the credentials on this pipeline. Will take a look tomorrow. Excited to take a look!

@peake100
Copy link
Contributor

It looks like the coverage report is failing to update due to a change in nightly flags.

I'll dig into it soon, but I just looked over the PR and it looks great!

Sorry for the long delay. Work has been a bit crazy.

@peake100
Copy link
Contributor

peake100 commented Jul 23, 2023

Hello!

Finally had the bandwidth to figure this out. Apologies, work has been bananas lately and has been eating up all my time.

You'll need to rebase off the latest dev to get this change. Once you do that and CI passes, this looks good to go!

Edit: Looks like Github lets me merge the latest changes in myself.

@iluvcapra
Copy link
Contributor Author

Okay just let me know what you'd like me to do.

Copy link
Contributor

@peake100 peake100 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! Merging.

Thanks again for all the hard work. Glad to have this in the repo.

@peake100 peake100 merged commit b08c53f into opencinemac:dev Jul 23, 2023
1 check passed
@peake100
Copy link
Contributor

@iluvcapra
Copy link
Contributor Author

Coolbeans, now on to the next thing! Glad you're busy, we're all shut down by the strike!

@peake100
Copy link
Contributor

Coolbeans, now on to the next thing! Glad you're busy, we're all shut down by the strike!

Ah man, I'm sorry to hear that. I'm a developer full time now but have a lot of friends in Hollywood affected by the strike. Hope that the studios stop being greedy and you're able to get back to work soon.

On another note: I've been thinking about doing some pretty heavy refactors in this lib to bring it inline with the changes I've been making to the Elixir version. Would you be open to me tagging you for code review?

@iluvcapra
Copy link
Contributor Author

On another note: I've been thinking about doing some pretty heavy refactors in this lib to bring it inline with the changes I've been making to the Elixir version. Would you be open to me tagging you for code review?

Would love to! Please email me because it seems GitHub doesn't have messaging anymore.

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

Successfully merging this pull request may close these issues.

None yet

2 participants