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

Added crhook hook style #304

Merged
merged 3 commits into from
Aug 14, 2023
Merged

Added crhook hook style #304

merged 3 commits into from
Aug 14, 2023

Conversation

SpookyScaryDev
Copy link
Contributor

Added crhook hookstyle as described by @inf1niti in #303
At the moment, haste still affects throw speed when using crhook, as that was the original behaviour crhook was removed. I also changed the naming of the speed constants, since the speed for the new hook styles was called CR_THROW_SPEED.

Copy link
Contributor

@inf1niti inf1niti left a comment

Choose a reason for hiding this comment

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

So I've tested this, and it all seems to work.

If you guys have tested it as well, and are OK with the speed - I'm cool with merging this. 👍

My only suggestion (besides the couple of inline comments left below) is that we no longer really need the hook_fast option - it was meant to be an option for people who didn't want to use the enhancements provided by the hook_smooth setting, but this hook is exactly the same (but faster).

My suggestion would be to removing the existing hook_fast implementation entirely, and renaming crhook to hook_fast. The crhook name honestly doesn't make sense, and this hook is faster than the existing fast hook anyways 😄

That way, we can also keep the hook options to 3, which feels slightly more reasonable than 4.

src/grapple.c Outdated
Comment on lines 433 to 436
float throwSpeed = cvar("k_ctf_hookstyle") != 3 ? NEW_THROW_SPEED : THROW_SPEED;

if (cvar("k_ctf_hookstyle") == 4) throwSpeed = CR_THROW_SPEED;

Copy link
Contributor

Choose a reason for hiding this comment

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

might want to convert these to a series of if statements, keeping the ternary doesn't make a ton of sense anymore.

src/commands.c Outdated
@@ -882,6 +884,7 @@ cmd_t cmds[] =
{ "hook_smooth", hooksmooth, 0, CF_PLAYER | CF_MATCHLESS, CD_HOOKSMOOTH },
{ "hook_fast", hookfast, 0, CF_PLAYER | CF_MATCHLESS, CD_HOOKFAST },
{ "hook_classic", hookclassic, 0, CF_PLAYER | CF_MATCHLESS, CD_HOOKCLASSIC },
{ "crhook", hookcrhook, 0, CF_PLAYER | CF_MATCHLESS, CD_HOOKCRHOOK },
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was the original command for this hookstyle, but it makes it more difficult to find the various options if they're not following the same naming convention. Lets stick with the hook_ prefix for this as well.

@cbirkeland
Copy link

@inf1niti
Sounds good! Agree with naming.
Thank you @inf1niti and @SpookyScaryDev .

@tcsabina
Copy link
Collaborator

@SpookyScaryDev,
let us know when ready to merge.

@SpookyScaryDev
Copy link
Contributor Author

@tcsabina Will do. Hope to do testing of the different options with the ctf community this week.

@tcsabina
Copy link
Collaborator

@tcsabina Will do. Hope to do testing of the different options with the ctf community this week.

Hi @SpookyScaryDev,
Have you had a chance to test this? Are we ready to merge?

Let me know...

@tcsabina
Copy link
Collaborator

tcsabina commented Aug 8, 2023

Guys,
Have you had some time to test this? Are we ready to merge?

Let me know...

@cbirkeland
Copy link

@SpookyScaryDev is on vacation until 11th. Hence no reply :) From CTF Admins point of view, we are good to merge if crhook is back like it used to be before it got removed with the values we requested.
There was a night of testing but i think main focus was on the ctf bots that hiipe is working on.
Again, thanks!

@tcsabina
Copy link
Collaborator

Thanks @cbirkeland!
Let's wait a few more days...

@SpookyScaryDev
Copy link
Contributor Author

If the code is ok, this should be ready to push now.

@tcsabina tcsabina merged commit ddd6c62 into QW-Group:master Aug 14, 2023
@tcsabina
Copy link
Collaborator

If the code is ok, this should be ready to push now.

Thanks for the note. Merged...

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.

4 participants