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

Fix freeze frame at end instead using the first frame #1334

Closed
wants to merge 9 commits into from

Conversation

jkolbly
Copy link
Contributor

@jkolbly jkolbly commented Oct 7, 2020

This fixes issue #1307 by making the freeze function take an ImageClip from slightly before the end of the clip (effectively the end of the clip).

  • I have provided code that clearly demonstrates the bug and that only works correctly when applying this fix
  • I have added suitable tests demonstrating a fixed bug or new/changed feature to the test suite in tests/
  • I have properly documented new or changed features in the documentation or in the docstrings
  • I have formatted my code using black -t py36

@tburrows13
Copy link
Collaborator

Thanks for the PR. I've not looked properly into this one yet, but it looks like quite a 'hacky' solution? If you are just trying to go back one frame, I think it would be better to do that by subtracting 1 / fps. However given that your change here means that it is failing test_freeze, you'll have to convince me that the tests are incorrect before I merge :)

@jkolbly
Copy link
Contributor Author

jkolbly commented Oct 7, 2020

Agreed that it's a bit hacky. The problem is the input clip doesn't always have an fps, but this seems to be working just as well. Definitely hacky, though, so I'll try to dig deeper and hopefully find a root cause of the issue.

@coveralls
Copy link

coveralls commented Oct 7, 2020

Coverage Status

Coverage increased (+0.04%) to 67.504% when pulling b77fd6a on ToxicOverload:master into fe5c782 on Zulko:master.

@tburrows13 tburrows13 added the bug-fix For PRs and issues solving bugs. label Oct 8, 2020
@tburrows13 tburrows13 changed the title Fix #1307 Fix freeze frame at end instead using the first frame Oct 8, 2020
@keikoro
Copy link
Collaborator

keikoro commented Oct 10, 2020

@ToxicOverload To update your feature branch so that it always "starts" with the latest commit on master, you'd rebase master, not merge it. So basically you are "pulling" the latest commits in master "under" what you've been working on.

@keikoro
Copy link
Collaborator

keikoro commented Oct 10, 2020

See e.g. this Atlassian tutorial.

@keikoro
Copy link
Collaborator

keikoro commented Oct 11, 2020

@ToxicOverload What you want to do is reset those last (merge) commits until you are back where you finished your last code change.

Then switch to a new branch that you use for your code – as per CONTRIBUTING.md (because right now it looks like you are working on master directly). Then update your local master branch so it has the latest changes from this repo, then rebase your local master while you are checked out in your own branch. Then you push your fix_ branch to your fork and create a PR from that.

Can explain in a bit more detail in a bit.

@jkolbly
Copy link
Contributor Author

jkolbly commented Oct 11, 2020

Will do, thanks! I don't think I'll get a chance until tomorrow morning, but thanks for helping out :) I'm pretty new to gitflow, so thanks for the tips

@jkolbly
Copy link
Contributor Author

jkolbly commented Oct 11, 2020

Does that mean I should close this PR?

@keikoro
Copy link
Collaborator

keikoro commented Oct 11, 2020

Step-by-step:

(Re)Familiarise yourself with the contents of CONTRIBUTING.md, particularly the last section re: workflow.

Make sure you don't have any uncommitted changes in your repo:

$ git status

If you do, either stash them for now with $ git stash (if they are important/needed) or discard them. Git will tell you how to do the latter.

Drop those merge commits by resetting to the last "good" commit (c48b9f8, by the looks of it) :

$ git reset COMMIT_HASH

Like above: check for the status of your files, but now definitely discard any changes. (They are the remains of those merge commits.)

Next, create a new branch named something along the lines of fix_abc, where abc is a 1-2 word descriptor of what you are working on (just meant to make your branch recognisable among the others). For a new feature, you'd prefix it with something like feat_ to differentiate it from a bug fix.

$ git checkout --branch YOUR_BRANCH_NAME

Switch back to master and get rid of all those commits you made (you have an exact copy of them now in your fix branch). Like above: find the ID of the last commit before the first you made:

$ git reset COMMIT_HASH

You'll now have to clean up the remains of your changes on master. Like above, check with $ git status what's going on, and discard the changes. (There's a shortcut to all, but it's destructive, so I'll leave it off for now). Normally, this would have been the point where you branched off into your own feature/fix branch as a first step (see CONTRIBUTING workflow). Once you are done with cleaning up, update your local master so it mirrors this repo's master again (this assumes you named the remote for this repo upstream):

$ git pull upstream master

Now switch back to your own fix branch again and rebase your updated master branch:

$ git checkout YOUR_BRANCH_NAME
$ git rebase master

If there weren't any conflicts: hooray! You can now push your branch to your fork here on GitHub (the assumption is, you originally cloned your fork to your HD, in which case your fork will have got the default name for remotes, origin):

$ git push origin YOUR_BRANCH_NAME

At this point I'm not sure if you can update this existing PR, or if you have to create a new one. Don't worry if the latter is the case, we can close this one and link it to your new one.

Misc notes:

  • use git status generously to learn about the status of your files/changes
  • use git log to figure out commit IDs
  • use git config --list to figure out the names and URLs of your remotes

@keikoro
Copy link
Collaborator

keikoro commented Oct 11, 2020

Will do, thanks! I don't think I'll get a chance until tomorrow morning, but thanks for helping out :) I'm pretty new to gitflow, so thanks for the tips

No worries!

Does that mean I should close this PR?

Leave it open for now, as said above, it's possible you can reuse it (I'm not sure myself).

@keikoro
Copy link
Collaborator

keikoro commented Oct 11, 2020

Updated/added to the steps because I realised you'll have to clean up a bit sooner. ^^

@jkolbly
Copy link
Contributor Author

jkolbly commented Oct 11, 2020

This is the new PR: #1348

@keikoro
Copy link
Collaborator

keikoro commented Oct 11, 2020

Closing this PR because it "moved" to #1348,

@keikoro keikoro closed this Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix For PRs and issues solving bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants