Skip to content

Extend image_name_template to allow timestamp - #395

Merged
Breakthrough merged 12 commits into
Breakthrough:developfrom
Veldhoen:394-extend-image-name-template-to-allow-timestamp
May 3, 2024
Merged

Extend image_name_template to allow timestamp#395
Breakthrough merged 12 commits into
Breakthrough:developfrom
Veldhoen:394-extend-image-name-template-to-allow-timestamp

Conversation

@Veldhoen

@VeldhoenVeldhoen commented Apr 26, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#394

@VeldhoenVeldhoen changed the title Added TIMESTAMP_MS to file_path substitutionsExtend image_name_template to allow timestampApr 26, 2024
@BreakthroughBreakthrough linked an issue Apr 27, 2024 that may be closed by this pull request
Extend image_name_template to allow timestamp #394
Closed
@Breakthrough

Breakthrough commented Apr 27, 2024•
edited
Loading

Copy link
Copy Markdown
Owner

I see in your PR you've already used the $TIMESTAMP_MS format as per the discussion in #394. Would you be able to run yapf to format the resulting code so it passes the formatting checks?

Thank you for the PR!

Breakthrough
Breakthrough requested changes Apr 28, 2024
View reviewed changes

@BreakthroughBreakthrough left a comment •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR, please address the following so this can be landed:

  • Rebase ontop of the develop branch
  • Ensure the code is formatted correctly: python -m yapf -i -r scenedetect tests

@Veldhoen

Veldhoen commented Apr 29, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks for replying so fast, even though my PR was still a draft. I'll make sure to address the issues you mentioned, hopefully later today.

@Veldhoen
Veldhoen marked this pull request as ready for review April 30, 2024 08:04
@Veldhoen

Veldhoen commented Apr 30, 2024

Copy link
Copy Markdown
ContributorAuthor

Rebased on develop now, and I think the formatting is in line with your standards now.
Also added $TIMECODE like you suggested in the issue, and added both $TIMECODE and $FRAME_NUMBER to test_save_images.

Documentation still needs to be updated to explain these extensions.

@Veldhoen
Veldhoen changed the base branch from main to developApril 30, 2024 08:29
Breakthrough
Breakthrough requested changes May 1, 2024
View reviewed changes

@BreakthroughBreakthrough left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks like the test_save_images test is failing on Windows, will approve once fixed. Thank you!

Comment threadscenedetect/scene_manager.py Outdated
IMAGE_NUMBER=image_num_format % (j + 1),
FRAME_NUMBER=image_timecode.get_frames(),
TIMESTAMP_MS=int(image_timecode.get_seconds() * 1000),
TIMECODE=image_timecode.get_timecode()),

@BreakthroughBreakthroughMay 1, 2024

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Timecodes need to be formatted specially as not all operating systems support : in paths. Please replace them with ;as is done when splitting videos:

TIMECODE=image_timecode.get_timecode().replace(":", ";")

This is most likely why test_save_images is failing on the Windows build currently.

Comment threadtests/test_scene_manager.py
@Veldhoen

Veldhoen commented May 2, 2024

Copy link
Copy Markdown
ContributorAuthor

I applied the requested changes, and extended docstring. Hope this will do!

@Veldhoen
Veldhoen requested a review from BreakthroughMay 2, 2024 12:32
@Breakthrough
Breakthrough merged commit 7629645 into Breakthrough:developMay 3, 2024
@Breakthrough

Breakthrough commented May 3, 2024

Copy link
Copy Markdown
Owner

Much appreciated, approved.

Thank you for the PR!

@Veldhoen

Veldhoen commented May 6, 2024

Copy link
Copy Markdown
ContributorAuthor

@Breakthrough When do you plan to release 0.6.4? I created a pre-release of my fork to be able to use the functionality right away, but I'd like to use a proper release at some point, of course.

@Breakthrough

Breakthrough commented May 8, 2024

Copy link
Copy Markdown
Owner

I'm hoping to wrap up the next release by the end of this month. If for whatever reason you need this sooner in production, I can consider pushing a pre-release to PyPI.

@Veldhoen

Veldhoen commented May 14, 2024

Copy link
Copy Markdown
ContributorAuthor

If you can make it by the end of this month, that would be great. Until then we'll be fine with my ad hoc pre-release. Thanks!

@Veldhoen

Veldhoen commented Jun 7, 2024

Copy link
Copy Markdown
ContributorAuthor

@Breakthrough do you have any updates concerning the release of 0.6.4? Would be much appreciated :)

@Breakthrough

Breakthrough commented Jun 10, 2024•
edited
Loading

Copy link
Copy Markdown
Owner

All feature work is complete, just finishing up testing and preparing the release. The new version should be out within a day or two.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BreakthroughBreakthroughBreakthrough approved these changes

Assignees

No one assigned

Labels

improvement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Extend image_name_template to allow timestamp

2 participants

@Veldhoen@Breakthrough