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

Updates to Timestamp Trade, Attempt 2 #428

Closed
wants to merge 2 commits into from
Closed

Conversation

vamiceDev
Copy link

I'm not that great at Git, a skill that's on my list to learn. So I just made a new repo with the same name and a single commit.

@feederbox826 Thanks for the heads-up.

Updates TimestampTrade to allow the user to add a tag/update the title of created timestamps.

@feederbox826
Copy link
Collaborator

your commits were not under vamiceDev, figured I would give you a heads up. tips for splitting identities

also cc @Tweeticoats

@vamiceDev
Copy link
Author

Yes, I saw right after I submitted, I appreciate it.

Since mp.import_scene_markers ignores any markers too close to other markers, this code isn't needed.
@Tweeticoats
Copy link
Contributor

runOnScenesWithMarkers is not a flag that I want because it's already covered by the setting createMarkers.
The plugin should run on the scenes as there are other functions that the user may want so the user could disable creating markers but still have the plugin to create movies.

I'm not against adding a tag and adding a prefix to markers.
For consistency I would use [Timestamp] or similar as the plugin uses [Timestamp: Skip Submit], [Timestamp: Skip Sync], [Timestamp: Auto Gallery] so it would be more consistent.

I'm working on my own pull request so I'll add the changes and submit that instead if that is ok.

@vamiceDev
Copy link
Author

runOnScenesWithMarkers is not a flag that I want because it's already covered by the setting createMarkers.

I don't see how. The query run when "processScene" == PLUGIN_ARGS uses "has_markers": "false". Also, there's a check in the processSceneTimestamTrade function that checks if len(s["scene_markers"]) == 0.

The use case I'm targeting here is a user who made a custom marker for a scene and now wants to add extra markers from timestamp.trade. As it's currently written, that scene will be skipped.

For consistency I would use [Timestamp] or similar as the plugin

Sure that's fine.

I'm working on my own pull request so I'll add the changes and submit that instead if that is ok.

Yeah, that's fine, but I would like to see the functionality from runOnScenesWithMarkers added.

@Tweeticoats
Copy link
Contributor

I have multiple tasks:

  • Sync: just run on scenes without markers
  • Re-process Scene: look for scenes with a timestamp.trade/scene/uuid url to re-process scenes that have been processed before
  • Re-process All: run on all scenes with a stashid and this is what I would recommend you run most of the time

If a scene has markers it will skip the scene unless you enable overwriting markers.
I'm happy to have a merge setting if you would find it useful.

@vamiceDev
Copy link
Author

I see my mistake now, missed the Reprocess All hook, so that's fine.

Suggestion: Calling it Re-Process All implies it only runs on previously processed scenes.

Having an option to merge would be preferable, and I'd argue that it should be on by default.

Also, overwriteMarkers is not in the timestampTrade.yml file, so it's not in the Stash UI.

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

Successfully merging this pull request may close these issues.

3 participants