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

Correcting +bxt_ch_hook request #456

Closed
SmileyAG opened this issue Oct 24, 2023 · 5 comments · Fixed by #458
Closed

Correcting +bxt_ch_hook request #456

SmileyAG opened this issue Oct 24, 2023 · 5 comments · Fixed by #458

Comments

@SmileyAG
Copy link
Collaborator

That a good addition, but I haven't looked to the request itself before, so let's see:

pl->v.health = 6969.f;

What is that random stuff does here? Please, don't add some wacky kind of that hardcoded stuff that ain't supposed to be in it, let player to set HP on their own with bxt_ch_set_health or using the god command

const auto HOOK_VEL = 1337.f;

Don't make the speed is hardcoded, better to create the specified cvar for that initially, or otherwise soon or later you still would to hear from others for making that addition

Other than that, additionally, might to consider of adding draw of trace line with power of pTriAPI
Or rather re-using beam sprites from HL1 (e.g. from Gauss gun) and pEfxAPI/pEventAPI/pTriAPI/SPR_ functions to draw it

@khanghugo
Copy link
Contributor

The health thing is so player won't die when going down. These parameters are hard coded so there is no need to do a lot of things just to get it to work. I agree that having another cvar to control it is nice but I just don't feel like adding some more cvars.

As for the line drawing, I did not know how to use the thing so I just skip it.

@SmileyAG
Copy link
Collaborator Author

SmileyAG commented Oct 25, 2023

The health thing is so player won't die when going down. These parameters are hard coded so there is no need to do a lot of things just to get it to work.

Do you want to know what main issue with the current code?
Well, then if there some keys with bound + cmds/aliases on them (e.g. bind / +bxt_ch_hook), then each time you spawning/going to menu/loading save = those - cmds will be executed (e.g. -bxt_ch_hook)

Now guess what would happen when you launching game and start map with bind for +bxt_ch_hook?
Right, you spawning as dead, since the ch_hook_hp_before is 0 by default

Instead of applying some value at init, I rather suggest to remove that health thing, since is really not supposed to be here, I'm still on a side that player should to decide himself if he want to set HP or not

And if someone think that adding a new variable for controlling speed isn't sound as something that would clutter or be unpopular, just remember that _offset and _anchor commands is exist, so it wouldn't be worse anyway

Don't think that I'm kind of angry or smth, i'm really pleasant for everyone who putting effort in it, but I really don't like when there is hardcoding that doesn't have a real purpose for it and half ppls would easily to prefer to edit to their own preferences, so you need to think about everyone

@YaLTeR
Copy link
Owner

YaLTeR commented Oct 25, 2023

FWIW I was also on the edge about hardcoded HP but figured it's a specific enough functionality that it doesn't really matter. The -command issue should be fixed though.

As for speed, I see even less of a reason for it to be customizable tbh.

@khanghugo
Copy link
Contributor

Thanks for pointing out the issues. I will try and fix it and see.

@SmileyAG
Copy link
Collaborator Author

SmileyAG commented Oct 26, 2023

As for speed, I see even less of a reason for it to be customizable tbh.

Well, if you want to know my opinion, I prefer to have hook speed to get more lower than the current one (like 850 units)

But at the same time I don't want to force for change value due of my preferences, instead I rather prefer to have a separate cvar for that
Like that's fine what value you want to have by default, just please let others to change to their own preferences

For a long time the BXT custom triggers was hardcoded to orange color and more than half opacity
And when you see that color for a hundred times (that might not even fit to HUD color), and specifically, when map textures / lights is too bright and have contain most of that orange color, it makes literally eyes are bleeding while trying to look through that trigger
So I had to make a separate cvar (bxt_triggers_color) for it to being satisfying with it

One of valid reason for hardcoding speed can be accepted in case if the code has some limitations that makes it only stable works with specific value / range of values, but that's a not case as you can see

Other valid reason is when you implementing some of game feature from one game to another, so you might to use some hardcoded values for similar behaviour

Example:

HOOK_DEF_0(ServerDLL, void, __cdecl, PM_Duck)
{
ORIG_PM_Duck();
if (ppmove && CVars::bxt_cof_slowdown_if_use_on_ground.GetBool()) {
auto pmove = reinterpret_cast<uintptr_t>(*ppmove);
int *onground = reinterpret_cast<int*>(pmove + offOnground);
usercmd_t *cmd = reinterpret_cast<usercmd_t*>(pmove + offCmd);
float *velocity = reinterpret_cast<float*>(pmove + offVelocity);
if ((*onground != -1) && (cmd->buttons & IN_USE)) {
velocity[0] *= 0.3f;
velocity[1] *= 0.3f;
velocity[2] *= 0.3f;
}
}
}

That 0.3 value is not a some of random, it's straight brought from pm_shared code of HL1: https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/pm_shared/pm_shared.c#L3028

Each time you think that some of cvar that changing only cosmetic stuff will be used only by few peoples, that completely not true
There some of stuff that I added before that ppls to use it this day, but most of them never talked about adding that (some of them afraid to bother others about that misc stuff, others are just kinda fine with what they got, but still would be cool for them to have a option for changing it in future)

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 a pull request may close this issue.

3 participants