Fix backwards bunnyhopping #984

Merged
merged 1 commit into from Apr 21, 2016

Projects

None yet

10 participants

@thegrb93
Contributor
thegrb93 commented Jun 7, 2015

Since move:GetForwardSpeed() returns 0 when the player isn't pushing any keys, but move:GetVelocity() returns the actual speed.

@thegrb93
Contributor
thegrb93 commented Jun 7, 2015

I also noticed this file uses a JUMPING variable which is shared across each Move hook and this assumes that each StartMove and FinishMove is called sequentially for each player. Is this a safe assumption?

@robotboy655
Collaborator

It is.

@thegrb93
Contributor
thegrb93 commented Jun 7, 2015

All right. Looks good then.

@willox
Collaborator
willox commented Jun 7, 2015

Backwards bunny-hopping was purposefully made to work the way it does in Half-Life 2, allowing you to reach silly speeds backwards.

@thegrb93
Contributor
thegrb93 commented Jun 7, 2015

How about a convar? I'm not sure if the flags are right. I don't use convars very often.

@willox
Collaborator
willox commented Jun 7, 2015

You'll need a networked ConVar. Use FCVAR_REPLICATED.

@thegrb93
Contributor
thegrb93 commented Jun 7, 2015

Thanks. I was thinking FCVAR_NOTIFY handled that, but looks like it just does the chat print. I'll make that more clear on the wiki

@thegrb93 thegrb93 changed the title from Fix backwards bunnyhopping. to Add convar for backwards bunnyhopping. Jun 7, 2015
@robotboy655
Collaborator

How about no. Adding an option to toggle everything is not a good idea. You can always remove it manually for your server if you don't like it.

@robotboy655 robotboy655 closed this Jun 7, 2015
@thegrb93
Contributor
thegrb93 commented Jun 7, 2015

There are sbox_weapons, sbox_godmode, sbox_playershurtplayers which are good. You shouldn't expect server owners to have to mod their lua for everything, especially when it is something that can't be modded without editing a file which is in the sandbox gamemode (which can be overwritten by updates). Please reconsider. It is sorely needed.

@willox
Collaborator
willox commented Jun 7, 2015

A feature that lets people almost instantly reach max velocity is a little bit ridiculous, especially when you force people to have at least some understanding of Lua to remove it.

@thegrb93
Contributor
thegrb93 commented Jun 7, 2015

@robotboy655 Please reopen.

@robotboy655 robotboy655 reopened this Jun 7, 2015
@robotboy655
Collaborator

OK FINE

@meepdarknessmeep
Contributor
@Kefta
Contributor
Kefta commented Jun 8, 2015

โค๏ธ

@FPtje
Contributor
FPtje commented Jun 8, 2015

Why the fuck is bunny hopping a feature you want enabled by default? The only existing gamemode that would benefit from this is the one from years ago that let you play HL2 maps in multiplayer. Sandbox wouldn't benefit and DarkRP server owners would hate it, as is the case for most, if not all sandbox derived gamemodes.

People who want this feature should get it by installing a third party addon, not by installing the game and leaving it as it is.

@Kefta
Contributor
Kefta commented Jun 8, 2015

@FPtje Because it's default HL2 behaviour. We already have the HL2 jump boost in Sandbox, and I think that adding another HL2 velocity system is fully in question. If you don't want it in DarkRP, then override the convar, just like you did with sv_allowcslua.

@FPtje
Contributor
FPtje commented Jun 8, 2015

So this should be in Gmod because it was a thing in another source game? Is this a thing in css and tf2 too? If it isn't why arbitrarily cling to hl2 and not tf2/css?

Why don't we scour the web for bugs in hl2, reimplement them in Gmod, enable them by default and make a convar to disable them? Your reasoning would justify this, because all bugs in hl2 are trivially default hl2 behaviour. This is ridiculous.

@thegrb93
Contributor
thegrb93 commented Jun 8, 2015

DarkRP uses it's own player class so it won't be affected. Many players enjoy the accelerated bunnyhop which is fine for purely sandbox servers, but some servers want to have more of a hl2dm environment and the accelerated bunnyhop ruins that. Those servers can decide to turn it off.

@Kefta
Contributor
Kefta commented Jun 8, 2015

Then disable the convar by default, that can go on debate. But the convar should absolutely be implemented

@Bo98
Contributor
Bo98 commented Jun 8, 2015

DarkRP uses it's own player class so it won't be affected.

Only because it uses player_default as a base.

@thegrb93
Contributor
thegrb93 commented Jun 8, 2015

And this applies to player_sandbox which darkrp doesn't use.

@FPtje
Contributor
FPtje commented Jun 8, 2015
@thegrb93
Contributor
thegrb93 commented Jun 8, 2015

I think you misunderstand, the bug was already on by default before. My initial commit removed the bug entirely, but I decided to change it to a convar since it would ruin the fun of many who liked to hop around in sandbox.

@Kefta
Contributor
Kefta commented Jun 8, 2015

If it's a "bug" then why has it not been fixed in HL2? If we're not catering to HL2 physics, then why is jumpboost implemented? I think a convar to toggle Source engine behaviour -- especially one as pertinent as this -- is fully acceptable and provides absolutely no harm

@Bo98
Contributor
Bo98 commented Jun 8, 2015

Source engine behaviour

Worth noting here that it's Orange Box Source engine behaviour specifically. It has been fixed in later versions.

"bug"

Of course it's a bug. Valve didn't add it on purpose. Valve fixed it in later versions, just like they also fixed HL1-style bhopping in Source.

@thegrb93
Contributor
thegrb93 commented Jun 8, 2015

Snip wrong button

@thegrb93 thegrb93 closed this Jun 8, 2015
@thegrb93 thegrb93 reopened this Jun 8, 2015
@thegrb93
Contributor
thegrb93 commented Jun 8, 2015

I could go back to removing the bug entirely, and then I can release this hack to workshop.

if SERVER then

    AddCSLuaFile()
    CreateConVar( "sbox_bunnyhop", "0", { FCVAR_ARCHIVE, FCVAR_NOTIFY, FCVAR_REPLICATED }, "Allow hl2 style bunny hopping?" )

end

local hl2_bunny_hop = GetConVar("sbox_bunnyhop")

hook.Add("Initialize","BHOP",function()
    local player_class = baseclass.Get("player_sandbox")
    function player_class:StartMove( move )
        if bit.band( move:GetButtons(), IN_JUMP ) ~= 0 and bit.band( move:GetOldButtons(), IN_JUMP ) == 0 and self.Player:OnGround() then
            self.JUMPING = true
        end
    end
    function player_class:FinishMove( move )
        if self.JUMPING then
            local forward = move:GetAngles()
            forward.p = 0
            forward = forward:Forward()
            local speedBoostPerc = ( ( not self.Player:Crouching() ) and 0.5 ) or 0.1
            local speedAddition = math.abs( move:GetForwardSpeed() * speedBoostPerc )
            local maxSpeed = move:GetMaxSpeed() * ( 1 + speedBoostPerc )
            local newSpeed = speedAddition + move:GetVelocity():Length2D()
            if newSpeed > maxSpeed then
                speedAddition = speedAddition - (newSpeed - maxSpeed)
            end
            if hl2_bunny_hop:GetBool() then
                if move:GetForwardSpeed() < 0 then
                    speedAddition = -speedAddition          
                end
            else
                if move:GetVelocity():Dot(forward) < 0 then
                    speedAddition = -speedAddition
                end
            end
            move:SetVelocity(forward * speedAddition + move:GetVelocity())
        end
        self.JUMPING = nil
    end
end)
@FPtje
Contributor
FPtje commented Jun 8, 2015
@willox
Collaborator
willox commented Jun 8, 2015

You should just be using StartMove and FinishMove hooks if your code isn't part of a gamemode. You'll probably need to create that ConVar clientside, too.

@thegrb93
Contributor
thegrb93 commented Jun 8, 2015

It's overwriting the player_sandbox move methods. Are you sure?

I reverted the convar additions. I can bring them back if needed.

@thegrb93 thegrb93 changed the title from Add convar for backwards bunnyhopping. to Fix backwards bunnyhopping Jun 8, 2015
@willox
Collaborator
willox commented Jun 8, 2015

Modifying a gamemode's methods (this includes player classes) is, as you said yourself, a hack. It's easy enough to just use two separate hooks to reproduce the same behvaiour.

@thegrb93
Contributor
thegrb93 commented Jun 8, 2015

K I'll do that. I haven't had any problems with the convar or prediction so I think that is working fine.

@willox
Collaborator
willox commented Jun 8, 2015

Have you tested without being on a listen server? If you create a ConVar on a dedicated server, it won't exist on any clients unless created there too.

@thegrb93
Contributor
thegrb93 commented Jun 8, 2015

I'll double check then. Would it be safe just to put the CreateConVar outside the if SERVER block?

I wasn't sure if it behaved similarly to concommand.Add, where you can't use it for both the server and the client.

@awilliamson

This is an engine issue, leave it as such. Writing tweaks and fixes programmatically should definitely be at the discretion of the user and/or server owners. If Garry thought this bug important he could simply update the game engine ( it's been done before right? ).

Releasing this as a 'Backward BHOP fix' under workshop should suffice for most users. A significant portion of the GMod community, outside of sandbox, don't exactly know how to do this.

Custom gamemodes use different player derivations anyway, and can completely bypass standard stuff. Even just putting a player velocity limit would cut the effect of this 'bug' off at the knees. Why is this still being debated?

@Kefta
Contributor
Kefta commented Jun 15, 2015

@awilliamson Late x 1. The fate has already been decided dude

@awilliamson

@LennyPenny Exactly! As a person who flies around backwards for fun in Sandbox, I feel it is iconic!

@Xandaros
Contributor

๐Ÿ‘Ž

@thegrb93
Contributor

@awilliamson @Xandaros Everyone else has agreed that they can install a hl2 bunnyhop mod if they wish. I personally still think it should have a convar, but it's up to the devs.

@robotboy655 robotboy655 merged commit 2eff9b2 into garrynewman:master Apr 21, 2016
@awilliamson

@robotboy655 Care to comment on why almost a year later you decide to merge without re-engaging the community in negotiations? What spurred this sudden merge?

@Kefta
Contributor
Kefta commented Apr 21, 2016

@awilliamson He merges a bunch at a time

@robotboy655
Collaborator

What community negotiations?

@Kefta
Contributor
Kefta commented Apr 21, 2016

@robotboy655 I guess he meant continue the debate over having a convar/which should be the default behaviour.

@awilliamson

@robotboy655 Just generally whether people still consider this an issue, or even want this fix. It has been almost a year - I think it might have been responsible to re-open community dialogue. After all that's what this platform is all about.
@Kefta Is also right, there were still unresolved issues around this PR.

@robotboy655
Collaborator

If people did not want this, it would not be here as a PR.
If someone wants the old behavior for their server/gamemode, they can easily make/use a mod to change it, or change it themselves.

@Kefta
Contributor
Kefta commented Apr 22, 2016

I would debate on the "ease;" the code above seems really hacky for overwriting it. Maybe just call a hook there to return a check whether to invert speedAddition?

@robotboy655
Collaborator

Why do you want an option to enable what's technically a bug?

@Kefta
Contributor
Kefta commented Apr 22, 2016 edited

This game is based on HL2:DM -- why change its physics behaviour?

@robotboy655
Collaborator

Because this is Garry's Mod, and not HL2DM?

@Kefta
Contributor
Kefta commented Apr 22, 2016 edited

Then why was this present from 2004 to 2016 in Garry's Mod? I understand on it being considered a bug, but it's just been around so long that it's more like a feature/engine behaviour now. Doesn't seem too game breaking either. It's fine to remove it by default, but it'd be nice to at the very least have a hook or convar so the fix to add it back doesn't have so redundant and reliant on the current code state

@meepdarknessmeep
Contributor

I could've sworn garry or someone was fighting against people wanting this
changed - it might have been some other dev on the next update thread though
On Apr 21, 2016 8:30 PM, "code_gs" notifications@github.com wrote:

Then why was this present from 2004 to 2016 in Garry's Mod? I understand
on it being considered a bug, but it's just been around so long that it's
more like a feature/engine behaviour now. Doesn't seem too game breaking
either. It's fine to remove it by default, but it'd be nice to at the very
least have a hook or convar so the fix to add it back doesn't have so
redundant and reliant on the current code state?

โ€”
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#984 (comment)

@FPtje
Contributor
FPtje commented Apr 22, 2016

We've been through this. Bugs that exist for a long time are still bugs. They don't become features.
Don't add config options for bugs, that is outrageous, even when the bug has existed for a long time.
The fact that a bug existed in games this game used to be based on is a shitty argument, even more so because a dedicated addon can reimplement the behaviour for the very, very few people that could possibly want this.

See the reasoning of last year for an elaboration on all these points.

@thegrb93 thegrb93 deleted the thegrb93:bunnyhopfix branch May 31, 2016
@Shigbeard

The man responsible for this patch is a monster.

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