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 a Frames option to the Timer node. #131

Closed
wants to merge 8 commits into from

Conversation

CoreTaxxe
Copy link

Adds the option to have timers run with frames instead of time. Works with idle and physics options to choose either physics or normal process. They are used for frame-perfect timings such as in doom-like games or fighting games.

@Eisendroid
Copy link

Nice. Useful feature

Copy link

@shoyguer shoyguer left a comment

Choose a reason for hiding this comment

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

LGTM!

@Veradictus
Copy link
Contributor

I like it

Copy link

@JohnnyThunder2 JohnnyThunder2 left a comment

Choose a reason for hiding this comment

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

Code looks good from my eye.

@Spartan322
Copy link
Member

Spartan322 commented Oct 14, 2024

This needs to be squashed, also does this have a related Godot proposal and/or PR? I would also like to try pushing this upstream before merging, but if you don't wish Godot to have it then we'll investigate merging it in a way to minimize merge conflicts.

@CoreTaxxe
Copy link
Author

This needs to be squashed, also does this have a related Godot proposal and/or PR? I would also like to try pushing this upstream before merging, but if you don't wish Godot to have it then we'll investigate merging it in a way to minimize merge conflicts.

It does. But they were deadset on not adding it so I cba to discuss that there any longer. The only merge conflict that could arise afaik is fro a not-yet-merged pr about bhvr QOL changes. Nothing big, however. We can keep this open until that PR gets merged upstream and I rebase/solve conflicts. Howover I fear that might take some time (waiting for the upstream merge not fixing it :p)

@SkogiB
Copy link
Contributor

SkogiB commented Oct 23, 2024

This needs to be squashed, also does this have a related Godot proposal and/or PR? I would also like to try pushing this upstream before merging, but if you don't wish Godot to have it then we'll investigate merging it in a way to minimize merge conflicts.

It does. But they were deadset on not adding it so I cba to discuss that there any longer. The only merge conflict that could arise afaik is fro a not-yet-merged pr about bhvr QOL changes. Nothing big, however. We can keep this open until that PR gets merged upstream and I rebase/solve conflicts. Howover I fear that might take some time (waiting for the upstream merge not fixing it :p)

Squash the commits and if Godot takes much longer to change their minds, we'll pull the trigger on this one without waiting Godot too long. We don't have a fixed amount of time we wait for upstream to do things, but yeah. Can't have our people waiting 3 months for Godot when the point of things was to bypass that. Get this squashed and we'll be g2g whenever we decide to do it.

@CoreTaxxe
Copy link
Author

CoreTaxxe commented Oct 27, 2024

This got a bit messy with the double fork repo hope rebase went well

Okay I cant figure out why github wont update the PR.

@SkogiB
Copy link
Contributor

SkogiB commented Oct 31, 2024

This got a bit messy with the double fork repo hope rebase went well

Okay I cant figure out why github wont update the PR.

Messy git stuff is fun! I'll see if one of the core guys has time to help you with it, I'm not much of a git wizard

@SkogiB
Copy link
Contributor

SkogiB commented Oct 31, 2024

Spartan says you need to force push it to your remote and it should go in (glorious force push!!!)
The push you're trying to do includes the squash correct?

@CoreTaxxe
Copy link
Author

Spartan says you need to force push it to your remote and it should go in (glorious force push!!!) The push you're trying to do includes the squash correct?

Yessn't I tried to figure out why git was infinitely stuck on processing after I did the force push after rebase. Did not squash yet. I'm gonna try a reset tomorrow 🤷

@CoreTaxxe
Copy link
Author

CoreTaxxe commented Nov 1, 2024

grafik
Branch looks like this. No idea what else I could try. Probably gonna nuke the branch + pr and reopen on new if github doesn't update until tomorrow.

(Squash + rebase + force push)

@Spartan322
Copy link
Member

Spartan322 commented Nov 1, 2024

You need to fetch the upstream then rebase on top of the upstream's master, if you have Redot-Engine as your upstream then you can do:

git fetch upstream
git rebase upstream/master
git rebase -i upstream/master
# squash with the interactive rebase
git push -f origin redot-frame-timer

if you need to set Redot-Engine as your upstream then you need to do this beforehand:

git remote add upstream https://github.com/Redot-Engine/redot-engine

@CoreTaxxe
Copy link
Author

You need to fetch the upstream then rebase on top of the upstream's master, if you have Redot-Engine as your upstream then you can do:

git fetch upstream
git rebase upstream/master
git rebase -i upstream/master
# squash with the interactive rebase
git push -f origin redot-frame-timer

if you need to set Redot-Engine as your upstream then you need to do this beforehand:

git remote add upstream https://github.com/Redot-Engine/redot-engine

This is exactly what I did

@Spartan322
Copy link
Member

Spartan322 commented Nov 1, 2024

Its kinda difficult to help from here then, if you're on the discord I could help better, ping Spartan322 in the redot-discussion channel then.

@CoreTaxxe
Copy link
Author

Closing due to github having lost or cut reference to branch after redot became stand-alone. Branch can no longer be updated. Openend new PR.

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

Successfully merging this pull request may close these issues.